-
Notifications
You must be signed in to change notification settings - Fork 367
protobuf upgrade #3255
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
base: main
Are you sure you want to change the base?
protobuf upgrade #3255
Conversation
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
&& make -j${NPROC} install && ldconfig \ | ||
&& cd python && python3 setup.py install --cpp_implementation \ | ||
&& cd ../.. && rm -rf protobuf | ||
ARG PROTOBUF_VERSION=25.1 |
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.
The version is 4.25.1 in other places. What's the reason?
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.
There is no difference between the 2 as you can see the commit is the same. The different naming style is due to the fact that 3.
or 4.
or 5.
is a different release style adopted by protobuf for python verions.
So all the places where there is a python pip reference the 4.25.1
version is used while in onnx-mlir we rely on the c++ protobuf so the verion is 25.1
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
@chentong319 @gongsu832, this pr is ready for review. |
You removed the part that installs the protobuf python binding with cpp_implementation, which typically should fail the build. But the build didn't fail. That's because onnx apparently is installing its own version of protobuf anyway, which you can see from the build log:
The problem with this version of protobuf is that its internal implementation is pure python rather than C/C++ so its performance is going to be a lot worse.
For a C/C++ implementation, you should see |
I did try with the initial approach of keeping the c++ implementation with setup.py but it either never built or completed the tests. I will add the change again and see if the failure happens again with setup.py, one thing to remember is there is std-c++=14 code level with the setup.py with
|
It will fail. |
…as it fails to build on s390x Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Signed-off-by: Sunny Anand <[email protected]>
Here is a complete breakdown of all the experiments I have run trying to find a solution for protobuf install such that the new protobuf upb c implementation can be used for the python bindings. Before moving to the bazel approach, below is the approach I took to see if using the cmake approach I can get the python bindings to use the new c backend. This approach however proves that pip always now defaults to python implmenetation of the api and the only time pip install worked with upb installation was when a new protobuf was installed apart from the one installed by cmake.
Dockerfile1
https://github.com/protocolbuffers/protobuf/blob/main/src/README.md After exhausting this approach I turned to building protobuf with bazel. I tried with bazel 6.5.0 and protobuf v25.1 and it ran into the below errors for both rhel and ubuntu on s390x where the s390x arch is not recognized. Error from UBI image
Error from Ubuntu image:
Now changing the Dockerfiles to point to the latest Bazel 8.3.0 and Protobuf 6.32.0 I run into different issue.
I have opened an issue for the above at protobuf: protocolbuffers/protobuf#23241 Dockerfiles used for above experiments Dockerfile-step-by-step-2
Dockerfile-step-by-step-1
@gongsu832 @chentong319 let me know if you guys have any suggestions I can try here. |
When
the protobuf gets built at
|
Protobuf upgrade from 4.21.12 to 4.25.1 which is the new minimum required for onnx 1.18.0. With protobuf 4.22.0 autotools support is dropped, so we need to build and install protobuf from the source in our docker with the abseil lib without the python setup.py