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.

Sign up for GitHub

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

Jump to bottom

Update to protobuf>=4.21.6,<4.22. #12864

Merged
merged 7 commits into from
Mar 6, 2023
Merged

Update to protobuf>=4.21.6,<4.22. #12864

merged 7 commits into from
Mar 6, 2023

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Feb 28, 2023 •
edited
Loading

Description

This updates cudf's protobuf version to resolve #12830 and a downstream conflict with ortools (a dependency of cuopt), which requires protobuf>=4.21.5.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added conda Python Affects Python cuDF API. labels Feb 28, 2023
@bdice bdice added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed Python Affects Python cuDF API. conda labels Feb 28, 2023
@bdice
Copy link
Contributor Author

bdice commented Feb 28, 2023

See also: https://protobuf.dev/support/cross-version-runtime-guarantee/

cc: @rgsl888prabhu

@bdice bdice self-assigned this Feb 28, 2023
@bdice bdice added Python Affects Python cuDF API. conda labels Feb 28, 2023
@bdice
Copy link
Contributor Author

bdice commented Feb 28, 2023

@galipremsagar and I discussed offline and decided to pin to >=4.22,<4.23 but there is no conda package for 4.22 yet (released 2 weeks ago). I will revert 563909c. I filed this PR to get version 4.22.0 on conda-forge: conda-forge/protobuf-feedstock#180

@galipremsagar galipremsagar mentioned this pull request Feb 28, 2023
Merged
@bdice bdice marked this pull request as ready for review February 28, 2023 21:07
@bdice bdice requested review from a team as code owners February 28, 2023 21:07
@bdice bdice requested review from vyasr and charlesbluca February 28, 2023 21:07
@beckernick
Copy link
Member

beckernick commented Mar 1, 2023 •
edited
Loading

Updating our protobuf to allow 4.22 would likely allow more easily mixing cuDF and Tensorflow pip packages. Tensorflow currently pins to 3.20.x, but the tensorflow 2.12 release candidate relaxes the constraint and appears to be consistent with the security alert and tries to install 4.22 by default. 4.21.6 would work, but the pip resolver doesn't select it if we've already installed 4.21.0 via cuDF (even though 4.21.6 should in theory be a permissible upgrade -- it naturally wants to go to 4.22).

This could be a great UX improvement for our pip packages.

@bdice
Copy link
Contributor Author

bdice commented Mar 1, 2023

@beckernick Conda packages aren't available yet for 4.22. To clarify, you're saying we should permit a more extensive range to ensure compatibility with TensorFlow? Should we pin to something like >=4.21.6,<5?

@beckernick
Copy link
Member

beckernick commented Mar 1, 2023 •
edited
Loading

Today, installing cuDF and the Tensorflow release candidate pip packages is challenging. Tensorflow naturally wants to bring in 4.22 (though could bring in 4.21.6 if needed), and I believe we must bring in 4.21.0 because of the version matching clause of protobuf==4.21. A similar problem exists for the current Tensorflow release with earlier versions of protobuf.

If our pip packages (today) were using protobuf>=4.21.6,<4.22, we should be compatible with the Tensorflow release candidate, which is great. When the conda packges are ready, if we then update again to pin to >=4.22,<5 (or anything with 4.22) it should be similarly compatible (or if we wait until then to update). Either of these scenarios should put us in a good position for our 23.04 packages.

@bdice
Copy link
Contributor Author

bdice commented Mar 2, 2023

@beckernick Fantastic explanation, thank you for the detailed response. I agree! For now we'll pin to a version that we can get with both pip and conda: >=4.21.6,<4.22 and we'll update again at a later time.

@bdice bdice removed request for vyasr and charlesbluca March 2, 2023 00:21
@bdice
Copy link
Contributor Author

bdice commented Mar 6, 2023

/merge

@rapids-bot rapids-bot bot merged commit 98b92a5 into rapidsai:branch-23.04 Mar 6, 2023
@beckernick
Copy link
Member

This should also hopefully enable cuDF packages to work more cleanly with Weights and Biases

https://twitter.com/bilzrd/status/1634363707336847361

cudfがprotobufのバージョンを4.21に固定しようとするのでwandbを一緒にインストールできない（wandbはprotobuf==4.21だと動かない）。
こういう時どうするのが正しいのだろう...？
Translated from Japanese by
I can't install wandb with cudf because it tries to lock the protobuf version to 4.21 (wandb doesn't work with protobuf==4.21).
What's the right thing to do at times like this...?

https://github.com/wandb/wandb/blob/436788097eee21cae61c7e98c9c4e8705b011848/requirements.txt#L7-L10

@bdice bdice mentioned this pull request Jul 26, 2023
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Jul 27, 2023
@bdice
Relax protobuf pinnings. (#13770)
5600f19 
This PR relaxes cudf's protobuf pinnings to help with compatibility issues. `cudf` uses `protobuf` in two places.

The first place `protobuf` is used is at build time, to generate a Python module from a `.proto` file in `python/cudf/cmake/Modules/ProtobufHelpers.cmake`: https://github.com/rapidsai/cudf/blob/f8e5a89e983065e1202f1151dd499bea3102a537/python/cudf/cmake/Modules/ProtobufHelpers.cmake#L16-L17

The second place `protobuf` is used is in the generated file `python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py` which is [imported here](https://github.com/rapidsai/cudf/blob/f8e5a89e983065e1202f1151dd499bea3102a537/python/cudf/cudf/io/orc.py#L14-L16).

The generated Python module used at runtime should be compatible with newer versions of `protobuf` than the version used to build the Python module, from my understanding of https://protobuf.dev/support/cross-version-runtime-guarantee/. Therefore, we only require that the runtime pinning of `protobuf` is of the same major version and an equal-or-greater minor version. That allows us to relax this pinning.

Follow-up to #12864, see that PR for more context.

Authors:
  - Bradley Dice (https://github.com/bdice)
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Ray Douglass (https://github.com/raydouglass)

URL: #13770
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@galipremsagar galipremsagar galipremsagar approved these changes

@jjacobelli jjacobelli jjacobelli approved these changes

Assignees

@bdice bdice

Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bdice @beckernick @jjacobelli @galipremsagar