Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[ray_client] close ray connection upon client deactivation #13919
[ray_client] close ray connection upon client deactivation #13919
Changes from 2 commits
b679a10
1373f48
0410c37
f3ad8a8
e517bb4
682ed30
3e6ce82
321751d
dff6906
3120440
ef24c44
1162160
f4b5e8e
c302816
1e47947
a78c3c7
7061aa3
dc12804
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe make this top-level and pass in redis_address/redis_password?
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.
Hmm what problem was this causing? Shouldn't is_initialized still work as before after the client connects?
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.
Oh I see, the data connection isn't added until later. How about we add a new dummy ping operation here then instead of is_initialized()? It can be anything really, is_initialized was just used as a convenient no_op.
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.
is_initialized()
also had the advantage of checking if ray was initialized, which is kinda useful here. What you might want to do though is do the data connection earlier. you might need to wait for /it/ to return ready (meaning the send/receive loop thread pair is kicked off and sets a flag, or even better, returns its client_id) -- but in general, yeah, some op here is useful.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.
OK i did something, take a look! happy to hear any suggestions
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.
@barakmich I just moved this into the try-except