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] update protobuf to 3.19.4 #25648

Merged
merged 15 commits into from
Jun 18, 2022
Merged

Conversation

scv119
Copy link
Contributor

@scv119 scv119 commented Jun 10, 2022

Why are these changes needed?

The error message in #25638 indicates we should use protobuf>3.19.0 to generated code so that we can work with python protobuf >= 4.21.1. Try generating wheels to see if this works.

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • Manually test
    • This PR is not tested :(

@scv119 scv119 linked an issue Jun 10, 2022 that may be closed by this pull request
@scv119 scv119 marked this pull request as ready for review June 10, 2022 17:25
@scv119 scv119 changed the title [Core][WIP] update protobuf to 3.19.4 [Core] update protobuf to 3.19.4 Jun 10, 2022
@scv119 scv119 added do-not-merge Do not merge this PR! and removed do-not-merge Do not merge this PR! labels Jun 10, 2022
@scv119
Copy link
Contributor Author

scv119 commented Jun 10, 2022

Manually tested with this we can work with both protobuf == 4.21.1 and protobuf == 3.20.1


# verfiy this no longer crashes. see
# https://github.com/ray-project/ray/issues/25282
from ray import tune # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

To simplify this even more, this should fail with just import ray right?

@pcmoritz
Copy link
Contributor

Thanks a ton for fixing this, this is great! Do we need the upper bound on 4.21.1 in the requirements.txt? It would be great to not constrain it so the CI will detect if there is other versions in the future that cause problems :)

Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

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

LGMT if tests pass!

python/requirements.txt Outdated Show resolved Hide resolved
python/requirements.txt Outdated Show resolved Hide resolved
@fishbone
Copy link
Contributor

test_protobuf_compatibility failed

@fishbone fishbone added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jun 13, 2022
@scv119 scv119 removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jun 14, 2022
@scv119
Copy link
Contributor Author

scv119 commented Jun 15, 2022

well, we are hitting a windows bug: protocolbuffers/protobuf#10075

@scv119 scv119 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jun 15, 2022
@scv119
Copy link
Contributor Author

scv119 commented Jun 15, 2022

@pcmoritz @rkooo567 so basically this works for linux and mac... except for Windows.
Turns out there is an open issue protocolbuffers/protobuf#10075 that python protobuf-4.21.1 package doesn't work with C++ protobuf-3.19.4 generated python file, on Windows.

What's our option here?

  1. we upgrade C++ protobuf to 3.19.4, change the pip requirement to protobuf >= 3.12.0, < 4.0.0. In this way:
  • by default, all user should install a protobuf version that's compatible (between 3.12.0 to 4).
  • linux/mac user with protobuf between 3.12.0 to 4.21.1 (latest) will work.
  • windows user, if their installation didn't respect our requirement, with protobuf 4.21.1 doesn't work.
  1. we upgrade C++ protobuf to 3.19.4, change the pip requirement to **protobuf >= 3.12.0 **. In this way:
  • linux/mac user with protobuf between 3.12.0 to 4.21.1 (latest) will work.
  • windows user with latest protobuf 4.21.1, ray doesn't work.
  1. do nothing?

@pcmoritz
Copy link
Contributor

There is also 4., which is upgrade C++ protobuf to 3.19.4 only on non-Windows (and also unpin), and keep the old one on Windows until protocolbuffers/protobuf#10075 is fixed :)

That one actually seems optimal given the situation, what do you think? We could even keep pinning on Windows so stuff works there too unless the pin is ignored.

@scv119 scv119 added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Jun 18, 2022
@scv119 scv119 merged commit 97582a8 into ray-project:master Jun 18, 2022
scv119 added a commit that referenced this pull request Oct 17, 2022
This upper bound was introduced in #25211 but the root cause is supposed be fixed by #25648.
scv119 added a commit to scv119/ray that referenced this pull request Oct 17, 2022
This upper bound was introduced in ray-project#25211 but the root cause is supposed be fixed by ray-project#25648.
rickyyx pushed a commit that referenced this pull request Oct 17, 2022
This upper bound was introduced in #25211 but the root cause is supposed be fixed by #25648.
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
This upper bound was introduced in ray-project#25211 but the root cause is supposed be fixed by ray-project#25648.

Signed-off-by: Weichen Xu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests-ok The tagger certifies test failures are unrelated and assumes personal liability.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core] Ray broken when the latest protobuf is used (>= 4.21.1)
5 participants