update to protbuf-28.2, absl-20240722, grpc-1.67 #58661
Conversation
Signed-off-by: Matti Picus <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
…fixed Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: Matti Picus <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
…h (from review) Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
This reverts commit 8501079. Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: Matti Picus <[email protected]>
Signed-off-by: Matti Picus <[email protected]>
Signed-off-by: Matti Picus <[email protected]>
Signed-off-by: Matti Picus <[email protected]>
Signed-off-by: mattip <[email protected]>
Signed-off-by: mattip <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates several core dependencies: protobuf to 28.2, absl to 20240722, and gRPC to 1.67.1. The changes include updating dependency definitions in Bazel files, adapting C++ code to new Protobuf APIs, and adjusting patches for the new dependency versions. The CI configuration is also updated to use larger instances, likely to handle increased resource usage. Overall, the changes look correct and necessary for the dependency upgrades. I have one suggestion regarding a now-empty patch file that is still being applied.
| patches = [ | ||
| "@io_ray//thirdparty/patches:grpc-cython-copts.patch", | ||
| "@io_ray//thirdparty/patches:grpc-avoid-goaway-messages.patch", | ||
| "@io_ray//thirdparty/patches:grpc-zlib-fdopen.patch", |
There was a problem hiding this comment.
The patch file grpc-zlib-fdopen.patch has been emptied in this pull request, but it's still listed here to be applied to com_github_grpc_grpc. While applying an empty patch might be a no-op, it's confusing and can hide intent. If this patch is no longer necessary with the updated gRPC version, it would be clearer to remove this line entirely.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Stale Patch Reference Blocks Builds
The patch file grpc-zlib-fdopen.patch has been completely emptied but is still referenced in bazel/ray_deps_setup.bzl at line 282. Applying an empty patch file will cause Bazel build failures when processing the grpc dependency.
thirdparty/patches/grpc-zlib-fdopen.patch#L1-L1
| build:linux --cxxopt="-std=c++17" | ||
| build:macos --cxxopt="-std=c++17" | ||
| build:clang-cl --cxxopt="-std=c++17" | ||
| build:clang-cl --cxxopt="-std:c++17" |
There was a problem hiding this comment.
Bug: Clang-cl Standard Flag Syntax Mix-up
The C++ standard flag for clang-cl was changed from -std=c++17 to -std:c++17, mixing GCC-style prefix (-) with MSVC-style separator (:). The correct syntax is either -std=c++17 (GCC-style with equals) or /std:c++17 (MSVC-style with slash and colon). This invalid flag syntax will cause compilation failures when using the clang-cl configuration.
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
github-actions
bot
added
the
stale
aslonnie
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
github-actions
bot
added
the
stale
aslonnie
removed
the
stale
|
cc @edoakes and @dayshah about conda-forge/ray-packages-feedstock#253 |
|
cc @andrew-anyscale too |
Signed-off-by: Lonnie Liu <[email protected]>
Signed-off-by: Lonnie Liu <[email protected]>
|
upgrading the libraries also upgrades |
Signed-off-by: Lonnie Liu <[email protected]>
|
I need to remove the pygithub package (or the test db altogether) so that the python rules thing can be upgraded without breaking macos and windows test jobs.. or maybe via some other means, like further upgrade or patch python rules and its related dependencies... ugh.. bazel... |
carrying on @mattip 's work