Conversation
5b23648 to
22ac878
Compare
|
OK MacOS CI build is fixed. The requirements.txt still had the old version of protobuf specified, which overrode the new version. The PR is ready to be merged. |
rules_rust 0.56.0 with s390x support we managed to upstream.
Some caveats apply:
- bazel related toolchain support on ppc64le is even worse
than that on s390x so this PR will not work on ppc64le.
As a result, we will have to retire our ppc64le Jenkins CI
after this PR (we can resurrect it once the bazel toolchain
on ppc64le catches up). ppc64le Jenkins CI is not required
for merging
- onnx cmake build file needs to be patched to add some missing
dependencies on abseil, which should eventually be upstreamed
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
- subdivide jobs so mlir cache can be posted early Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
Signed-off-by: Gong Su <[email protected]>
onnx doesn't get installed then reinstalled Signed-off-by: Gong Su <[email protected]>
22ac878 to
046183c
Compare
|
Do we need to update the document(such as BuildOnLinuxOSX.md) accordingly? |
There is really nothing changed in terms of building onnx-mlir itself. Most of the changes are for building protobuf with C/C++ backend on s390x. We can certainly describe what's need to be done in BuildOnLinuxOSX.md but unless you are a developer you probably don't care about that. This is also the reason we typically recommend people to use the prebuilt docker image instead of trying to do all these dirty hack themselves. |
|
@gongsu832 I am running some tests based off this pr especially on the abseil dependency part. I will update the results here with my experiment. Sorry for the delay caught up with a few urgent issues. Will try to post the results and findings by next week. |
Sunny-Anand
left a comment
There was a problem hiding this comment.
I am not very well acquainted with Bazel and Protobuf. It would be wonderful if we could store the dependency matrix and also the patch for each broken dependency.
Otherwise looks good to me. Approving as based on my newer tests based off this pr, the builds are still running into broken dependencies when trying with bazel 7.2 and using bzlmod and rust.
| rm -rf ${ABSL_DIR} ${HOME}/.cache | ||
|
|
||
| # Install protobuf | ||
| PROTOBUF_VERSION=6.31.1 |
There was a problem hiding this comment.
Do we want to keep the same version as 6.32 or do we want to update requirements.txt with 6.31.1
| cd python | ||
| python3 setup.py install --cpp_implementation | ||
| # Install absl for new version of protobuf | ||
| ABSL_VERSION=20240722.1 |
There was a problem hiding this comment.
Same as above for ABSEIL version.
| cd ${PROTOBUF_DIR}/build | ||
| # Must specify -Dprotobuf_BUILD_TESTS=OFF otherwise find_package(absl) | ||
| # in onnx will fail due to missing protobuf::gmock target | ||
| cmake -DCMAKE_INSTALL_LIBDIR=lib \ |
There was a problem hiding this comment.
Same as above for using cmake and bazel together.
| @@ -0,0 +1,9 @@ | |||
| git fetch --prune --unshallow --tags | |||
There was a problem hiding this comment.
Same comment as above. It seems this patching is needed for all CI platforms, is this correct? Then should we get someone from onnx take a look at this?
| RUN ONNX_ROOT=${WORK_DIR}/onnx-mlir/third_party/onnx \ | ||
| && cd ${ONNX_ROOT} \ | ||
| && CMAKE_ARGS="-DCMAKE_INSTALL_LIBDIR=lib" python3 -m pip install . | ||
| # Require patching until upstreamed |
There was a problem hiding this comment.
Why is this patching needed for onnx here for abseil? Doesn't onnx as third-party install its own protoc using cmake and abseil? Is this something we need to report to onnx as a breakage?
| && cargo install cargo-bazel --version ${CARGO_BAZEL_VERSION} | ||
|
|
||
| # Install absl for new version of protobuf | ||
| ARG ABSL_VERSION=20240722.1 |
There was a problem hiding this comment.
Should this be updated to the latest version of ABSEIL for protobuf 32.0 https://registry.bazel.build/modules/protobuf/32.0
| RUN git clone -b v${PROTOBUF_VERSION} --recursive https://github.com/protocolbuffers/protobuf.git \ | ||
| && cd protobuf && ./autogen.sh \ | ||
| && ./configure --enable-static=no \ | ||
| ARG PROTOBUF_VERSION=6.31.1 |
There was a problem hiding this comment.
Do we want to keep the same version as 6.32 or do we want to update requirements.txt with 6.31.1
| # Must specify -Dprotobuf_BUILD_TESTS=OFF otherwise find_package(absl) | ||
| # in onnx will fail due to missing protobuf::gmock target | ||
| && CC=clang CXX=clang++ \ | ||
| cmake -DCMAKE_INSTALL_LIBDIR=lib \ |
There was a problem hiding this comment.
I did try to change the build from cmake to bazel for building and installing protobuf and building the protoc; however as usual, it runs into issues with missing upstreams for rust_rules for s390x. The only comment I have here is, if we had found a way to use Bazel for this step, it would avoid needing us to build abseil as it is a dependency of protobuf and if bazel is used for building protobuf then it would also be able to fetch and build abseil own its own. https://registry.bazel.build/modules/protobuf/32.0
|
@AlexandreEichenberger @gongsu832 if you guys are ok with merging this pr, I can merge this now. Also do we want to update https://github.com/onnx/onnx-mlir/blob/main/README.md to drop ppc64le-Linux and also remove/stop the CI from running? |
|
Jenkins Linux amd64 Build #17470 [push] Upgrade to protobuf 6.31... started at 18:33 |
|
Jenkins Linux s390x Build #17472 [push] Upgrade to protobuf 6.31... started at 19:33 |
|
Jenkins Linux s390x Build #17472 [push] Upgrade to protobuf 6.31... passed after 2 hr 31 min |
|
Jenkins Linux amd64 Build #17470 [push] Upgrade to protobuf 6.31... passed after 3 hr 13 min |
|
FYI, on my Mac I had to install the package below brew install abseil |
Upgrade to protobuf 6.31.1, which is the 1st version that uses rules_rust 0.56.0 with s390x support we managed to upstream. Some caveats apply:
bazel related toolchain support on ppc64le is even worse than that on s390x so this PR will not work on ppc64le. As a result, we will have to retire our ppc64le Jenkins CI after this PR (we can resurrect it once the bazel toolchain on ppc64le catches up). ppc64le Jenkins CI is not required for merging
onnx cmake build file needs to be patched to add some missing dependencies on abseil, which should eventually be upstreamed