Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove misleading address field from Ray Task #2870

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sovietaced
Copy link
Contributor

@Sovietaced Sovietaced commented Oct 27, 2024

Tracking issue

Closes flyteorg/flyte#5877

Why are the changes needed?

The documentation implies that you can run a ray job on an existing cluster by configuring the address but this is not true since the task still requires users to configure details for a new and dynamically created Ray Cluster. And ray jobs will run on the dynamically created Ray Cluster even if an address is configured.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

flyteorg/flytesnacks#1765

Signed-off-by: Jason Parraga <[email protected]>
Copy link

welcome bot commented Oct 27, 2024

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Copy link

codecov bot commented Oct 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.37%. Comparing base (3fc51af) to head (b2654c4).
Report is 13 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2870       +/-   ##
===========================================
+ Coverage   45.53%   92.37%   +46.83%     
===========================================
  Files         196      126       -70     
  Lines       20418     5417    -15001     
  Branches     2647        0     -2647     
===========================================
- Hits         9298     5004     -4294     
+ Misses      10658      413    -10245     
+ Partials      462        0      -462     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is backwards incompatible right?

@Sovietaced
Copy link
Contributor Author

This is backwards incompatible right?

Correct. If we want to keep backwards compatibility I can close this. I was't sure how stable these plugins are meant to be, especially this one.

@@ -65,7 +64,7 @@ def __init__(self, task_config: RayJobConfig, task_function: Callable, **kwargs)
self._task_config = task_config

def pre_execute(self, user_params: ExecutionParameters) -> ExecutionParameters:
init_params = {"address": self._task_config.address}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. I think this only works for local execution. For those people who still want to use it, they can use RAY_ADDRESS env.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Cannot submit a Ray job to an existing cluster
3 participants