-
Notifications
You must be signed in to change notification settings - Fork 7.1k
feat: add --ip option in attach #59931
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
feat: add --ip option in attach #59931
Conversation
Signed-off-by: machichima <nary12321@gmail.com>
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.
Code Review
This pull request adds a useful --ip option to ray attach, allowing users to connect to a specific node in the cluster by its IP address. The implementation is solid, and the changes are well-contained. I've provided a couple of suggestions to enhance code quality and efficiency by reusing an existing object and tidying up some redundant code. Overall, this is a great addition.
Signed-off-by: machichima <nary12321@gmail.com>
…rker-node Signed-off-by: machichima <nary12321@gmail.com>
Signed-off-by: machichima <nary12321@gmail.com>
Signed-off-by: machichima <nary12321@gmail.com>
Signed-off-by: machichima <nary12321@gmail.com>
Signed-off-by: machichima <nary12321@gmail.com>
b90cd98 to
a5bf5a5
Compare
Signed-off-by: machichima <nary12321@gmail.com>
|
@edoakes PTAL. Thank you! |
edoakes
left a comment
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.
Roughly looks good to me. nit: let's call the arg --node-ip to make it a little more clear what it is specifying
…rker-node Signed-off-by: machichima <nary12321@gmail.com>
Thank you! Fixed in 475940f. Also rename variables to |
Signed-off-by: machichima <nary12321@gmail.com>
3f77818 to
475940f
Compare
Signed-off-by: machichima <nary12321@gmail.com>
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Signed-off-by: machichima <nary12321@gmail.com>
Signed-off-by: machichima <nary12321@gmail.com>
edoakes
left a comment
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.
LGTM

Description
Currently ray attach only allows opening an SSH session on the head node. It could be useful to allow attaching to worker nodes to check what state the execution environment and file system are in (e.g. running conda list, examining config files such as ~/.keras/keras.json).
Related issues
Closes #7064
Additional information
This PR add
--node-ipargs toray attachto specify the node IP to attach to. Usage:ray attach cluster.yaml --node-ip <node ip>. Default to head node if the--node-ipis not provided.Add unit test and tested on GCP (see #59931 (comment))