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

[core][client] Remove protocol version, instead just compare Ray and Python versions. #42760

Merged
merged 7 commits into from Feb 5, 2024

Conversation

rynewang
Copy link
Contributor

@rynewang rynewang commented Jan 27, 2024

In a regular Ray worker init, (python version, ray version) is compared against the cluster. In Ray Client, however, a (python version, protocol_version) is compared. This makes it a looser requirement for the client connection than for the workers.

This commit removes that and unifies version check between both sides to (python version, ray version). This helps avoiding any confusion in version mismatch.

Fixes #42356

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@ckw017
Copy link
Member

ckw017 commented Jan 29, 2024

Were you able to find out why this is there in the first place? Shouldn't this constant be the same on a given ray version anyway?

@rynewang
Copy link
Contributor Author

Eric explained it was safe to remove it. Did not track to its genesis though.

@ckw017
Copy link
Member

ckw017 commented Jan 29, 2024

Okay, sounds good. If you want to track something to it's genesis, you can use git blame. In this case it looks like it was added here, but there's not really an explanation on the PR: #13846

Shouldn't this constant be the same on a given ray version anyway?

^I'm still wondering about this. I'm also not sure if this fixes anything with #42356, isn't this removing a layer of version checking?

+ f" client is {version_str},"
+ f" server is {conn_info['python_version']}"
)
if ignore_version or "RAY_IGNORE_VERSION_MISMATCH" in os.environ:
Copy link
Member

Choose a reason for hiding this comment

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

Are we removing behavior for RAY_IGNORE_VERSION_MISMATCH intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back. Thanks for spotting it!

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
…e-protocol-version

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
@rynewang
Copy link
Contributor Author

@ericl tests passed. please take a look! Thanks.

@rynewang
Copy link
Contributor Author

rynewang commented Feb 5, 2024

@ericl friendly ping

@jjyao jjyao merged commit 8ed5465 into ray-project:master Feb 5, 2024
9 checks passed
@rynewang rynewang deleted the remove-protocol-version branch February 5, 2024 18:49
tterrysun pushed a commit to tterrysun/ray that referenced this pull request Feb 14, 2024
…Python versions. (ray-project#42760)

In a regular Ray worker init, (python version, ray version) is compared against the cluster. In Ray Client, however, a (python version, protocol_version) is compared. This makes it a looser requirement for the client connection than for the workers.

This commit removes that and unifies version check between both sides to (python version, ray version). This helps avoiding any confusion in version mismatch.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
Signed-off-by: tterrysun <terry@anyscale.com>
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.

[client] Better version-compatibility validation
4 participants