-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[core] Add opt-in flag for Windows and OSX clusters, update ray start
output to match docs
#31166
[core] Add opt-in flag for Windows and OSX clusters, update ray start
output to match docs
#31166
Conversation
ray start
output to match docs and de-emphasize Ray Clientray start
output to match docs
python/ray/scripts/scripts.py
Outdated
if dashboard_url: | ||
cli_logger.print( | ||
cf.bold( | ||
" RAY_ADDRESS='http://<dashboard URL>' ray job submit " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" RAY_ADDRESS='http://<dashboard URL>' ray job submit " | |
" RAY_ADDRESS='http://<dashboard URL>:8265' ray job submit " |
Not sure if 8265
is technically/customarily part of the URL, but I could see a lot of users leaving it out if we don't include it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or since we're in the if dashboard_url
, could we use {dashboard_url}
itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks for the heads up! Yeah actually I tried dashboard_url
at first, but it defaults to localhost, which is a bit confusing since this part is supposed to be about connecting from a remote client. The {dashboard_url}
version of the message won't work out of the box in that scenario.
I can also add the actual dashboard port that was used to this message, by the way.
Co-authored-by: Archit Kulkarni <architkulkarni@users.noreply.github.com> Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu>
Thanks for doing this ❤️ Just a small nit: At the moment we have an unholy mix of sometimes It seems at the moment the 0 and 1 convention is more common https://docs.ray.io/en/latest/tune/api_docs/env.html and the other variables in the ray_constants.py file, should we try to standardize around that for now for new environment variables? The 0 / 1 convention feels a little nicer since there is no problem to decide between "True" and "true" (also I feel like it is the more common convention, but I'm not sure about that). |
Is there a good reason to document this flag? It seems preferable to raise an exception and just say we do not support this. |
People should be allowed to live dangerously (with a warning of course). Also.. possibly someone could come along and help make this work for OSX / Windows at some point? |
Yes, this is the reason. We've also had at least two users ask about this on discuss.ray.io, and it seems their only real blocker is #30770. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the messages again, I think there is some potential for confusion about what a cluster is. Could we clarify the message to say "Multi-node Ray clusters"?
Also:
- I don't think we should print any warning on ray.init()--- this is spammy and probably not actionable if your cluster is already started.
- I think we should raise an error when trying to start a worker node on OSX/Windows without the flag set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot we override the node address so driver doesn't listen to the localhost by default, similar to how we override address with ray start ray start --address=xxx
on mac and windows?
I think this breaks master #32389 |
…ray start` output to match docs (ray-project#31166)" This reverts commit 90f8511.
…update `ray start` output to match docs (ray-project#31166)"" This reverts commit 0566e84.
… output to match docs (#32409) Un-revert #31166. This PR cleans up a few usability issues around Ray clusters: - Makes some cleanups to the ray start log output to match the new documentation on Ray clusters. Mainly, de-emphasize Ray Client and recommend jobs instead. - Add an opt-in flag for enabling multi-node clusters for OSX and Windows. Previously, it was possible to start a multi-node cluster, but then any Ray programs would fail mysteriously after connecting to the cluster. Now, it will warn the user with an error message if the opt-in flag is not set. - Document multi-node support for OSX and Windows. Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu> Co-authored-by: Archit Kulkarni <architkulkarni@users.noreply.github.com>
…t` output to match docs (ray-project#31166) This PR cleans up a few usability issues around Ray clusters: Makes some cleanups to the ray start log output to match the new documentation on Ray clusters. Mainly, de-emphasize Ray Client and recommend jobs instead. Add an opt-in flag for enabling multi-node clusters for OSX and Windows. Previously, it was possible to start a multi-node cluster, but then any Ray programs would fail mysteriously after connecting to the cluster. Now, it will warn the user with an error message if the opt-in flag is not set. Document multi-node support for OSX and Windows. Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu> Co-authored-by: Archit Kulkarni <architkulkarni@users.noreply.github.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
…ray start` output to match docs (ray-project#31166)" (ray-project#32403) This reverts commit 90f8511. Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
… output to match docs (ray-project#32409) Un-revert ray-project#31166. This PR cleans up a few usability issues around Ray clusters: - Makes some cleanups to the ray start log output to match the new documentation on Ray clusters. Mainly, de-emphasize Ray Client and recommend jobs instead. - Add an opt-in flag for enabling multi-node clusters for OSX and Windows. Previously, it was possible to start a multi-node cluster, but then any Ray programs would fail mysteriously after connecting to the cluster. Now, it will warn the user with an error message if the opt-in flag is not set. - Document multi-node support for OSX and Windows. Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu> Co-authored-by: Archit Kulkarni <architkulkarni@users.noreply.github.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
… output to match docs (ray-project#32409) Un-revert ray-project#31166. This PR cleans up a few usability issues around Ray clusters: - Makes some cleanups to the ray start log output to match the new documentation on Ray clusters. Mainly, de-emphasize Ray Client and recommend jobs instead. - Add an opt-in flag for enabling multi-node clusters for OSX and Windows. Previously, it was possible to start a multi-node cluster, but then any Ray programs would fail mysteriously after connecting to the cluster. Now, it will warn the user with an error message if the opt-in flag is not set. - Document multi-node support for OSX and Windows. Signed-off-by: Stephanie Wang <swang@cs.berkeley.edu> Co-authored-by: Archit Kulkarni <architkulkarni@users.noreply.github.com> Signed-off-by: elliottower <elliot@elliottower.com>
Why are these changes needed?
This PR cleans up a few usability issues around Ray clusters:
ray start
log output to match the new documentation on Ray clusters. Mainly, de-emphasize Ray Client and recommend jobs instead.ray start --head
output before this PR:After:
If on OSX or Windows and RAY_ENABLE_WINDOWS_OR_OSX_CLUSTER is not set:
Related issue number
Closes #30770.
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.