ARROW-10792: [Rust] [CI] Modularize builds for faster build and smaller caches #8821
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8821 +/- ##
==========================================
- Coverage 84.48% 77.39% -7.09%
==========================================
Files 190 170 -20
Lines 46854 39528 -7326
==========================================
- Hits 39584 30593 -8991
- Misses 7270 8935 +1665
Continue to review full report at Codecov.
|
| restore-keys: ${{ runner.os }}-${{ matrix.arch }}-target-cache-${{ matrix.rust }}- | ||
| - name: Run tests | ||
| run: | | ||
| cd rust/arrow |
There was a problem hiding this comment.
@GregBowyer we'd be able to add parquet to stable tests here, so we can leave out adding it to CI in your PR
| container: | ||
| image: ${{ matrix.arch }}/rust | ||
| env: | ||
| ARROW_TEST_DATA: /__w/arrow/arrow/testing/data |
There was a problem hiding this comment.
It is the absolute path to where the container action places the checkout code on. I tried using relative paths but I was unable to make it work.
| @@ -903,35 +902,6 @@ services: | |||
| /bin/bash -c " | |||
| /arrow/ci/scripts/r_sanitize.sh /arrow" | |||
|
|
|||
| ################################ Rust ####################################### | |||
|
|
|||
| debian-rust: | |||
There was a problem hiding this comment.
Even if we plan to use plain GHA jobs without docker-compose it'd be nice to keep it around to let contributors execute rust builds without installing the toolchain locally.
There was a problem hiding this comment.
The arguments I used to remove it:
- this compose and associated dockerfile are no longer tested under the CI, and their primary purpose was to be used in the CI
- the dockerfile is not needed, as the default rust image is sufficient
- the compose contains non-trivial aspects, such as:
- volume mounting with
delegate - using the no-longer needed
rust_test.sh - non-default
CARGO_*env variables
- volume mounting with
docker-compose run debian-rustis IMO less transparent to a user about what is happening, and the person needs to go to a +1k LOC file to understand.
In opposition, this whole project can now be built using
docker run --rm -v $(pwd)/rust:/rust -it rust /bin/bash -c "cd /rust && cargo build"which IMO is really simple to understand and reproduce. Wouldn't this command on the README be sufficient for contributors?
In the end, I evaluated that having these around would be more confusing than just having the user to follow standard rust practices of building a rust project (including using the official docker image), and therefore there was no need for the maintenance burden of having these around.
There was a problem hiding this comment.
Okay, sounds good to me - lucky us that we have cargo around.
Please document how to reproduce the builds locally with docker.
There was a problem hiding this comment.
I have added a section on the README on how to compile, which includes both pure cargo and docker variants.
|
Thanks Jorge for working on this! In general it looks good to me, but this PR makes harder to reproduce the exact same environment locally. Considering that cargo makes the building process straightforward and if it indeed speeds up the build execution by a factor of 2 I can accept that tradeoff. |
github-actions
bot
added
Component: Rust
needs-rebase
github-actions
bot
removed
the
needs-rebase
|
I rebased it against latest master and placed the c interface as part of the tests, instead of the cronjob, which will allow us to verify that part on every pr, as it should be. |
|
cc @alamb and @andygrove , as this will have a (hopefully positive) impact on all our builds. |
|
I will try and read this more carefully later this evening -- it sounds pretty awesome |
There was a problem hiding this comment.
This looks like a really nice improvement @jorgecarleitao
I checked that the commands in ci/scripts made it into the rust.yaml and I poked around in this PR's output -- things look good to me. 👍
I don't fully follow the concern about the lack of a way to build inside of docker-compose but I have never used that feature and if I wanted to build arrow in docker I would indeed use the image that @jorgecarleitao has used
The only failure on the PR at this time is some C++ / windows job which looked unrelated to this change: https://github.com/apache/arrow/pull/8821/checks?check_run_id=1504026175
unity_1_c.c
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\stdio.h(378,9): error C2220: the following warning is treated as an error [D:\a\arrow\arrow\build\cpp\src\arrow\arrow_shared.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\stdio.h(378,9): warning C5105: macro expansion producing 'defined' has undefined behavior [D:\a\arrow\arrow\build\cpp\src\arrow\arrow_shared.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.17763.0\ucrt\stdio.h(378,9): message : to simplify migration, consider the temporary use of /Wv:18 flag with the version of the compiler with which you used to build without warnings [D:\a\arrow\arrow\build\cpp\src\arrow\arrow_shared.vcxproj]
|
I retriggered the C++ CI tests on github in hopes it passes on a subsequent run. @jorgecarleitao I propose we merge this PR in and then send a note to the dev@arrow mailing list with a heads up ("major refactor of rust CI configuration, please let us know if you see issues") -- I am happy to do so if you would prefer |
|
Let's do that. I can write the note, as I can also describe the changes in more depth, as IMO they are relevant to other parts of the project also. |
|
@jorgecarleitao -- sounds good (with you writing the note) The windows C++ test is still failing in a strange way -- I can't seem to find
https://github.com/apache/arrow/pull/8831/files#r537152984 |
|
@alamb , these are unrelated and I (unfortunately) see them often whenever I need to change dockerfiles and other CI files. I have been ignoring them, as I expect / hope that people from the respective implementations to be handling them already. I have been also trying to reduce the number of builds that we trigger when we change rust-specific code, e.g. #8824 |
|
Ok, 🚀 it is then! |
This is a requirement fielded to merge #7767, and so I though about adding it to our CI. I am not sure whether this is exactly what we need to test #7767, and so I would like to request help from @paddyhoran and @rj-atw here. NOTE: this is built on top of #8821 . Only the last commit is relevant. Basically, this runs ``` rustup add target wasm32-unknown-unknown cd rust/arrow cargo build --target wasm32-unknown-unknown ``` Note that this does not actually run the tests yet. I think that that is #7767 's goal. Closes #8864 from jorgecarleitao/wasm32 Authored-by: Jorge C. Leitao <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
…er caches This PR refactors the CI with the purpose of making it faster, easier to extend, and more robust. The main consequences of this PR are: 1. build time of non-integration jobs reduced by half (10m vs 20m) 2. coverage is now part of all builds 3. integration with the c data interface is now part of all builds 4. significantly reduced the risk of cache misses, and the size of the caches 5. reduced complexity by removing the need to build a docker image 6. macos now have caching in place, like other builds 7. significantly easier to extend to other architectures, os, etc. The overall design of this PR is that most steps of the CI process should be independent of all others as much as possible, so that * they can run in parallel * it is clearer where the failure is * it is easier to extend the build to other variations (arch, os, flags, etc.) Intrinsically, the CI pipeline is just a DAG where certain jobs benefit from shared compute and thus depend on common nodes, while others run independently. This PR re-writes most of our CI as simple github jobs that share caches whenever makes sense, thereby describing the aforementioned DAG. Consequently, many of the sequential steps are now executed in parallel, causing the items describe above. There are two tasks depending on rust that are not in the `rust.yaml`: * lint, that happens via archery * integration tests, that happens via archery The bottleneck of a green pipeline are the integration tests, that currently re-compile a significant number of components (as I already flagged on the mailing list), but at least we get faster failures for compilation (2.5m) and clippy (4.5m). Closes apache#8821 from jorgecarleitao/build_tests Authored-by: Jorge C. Leitao <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
This is a requirement fielded to merge apache#7767, and so I though about adding it to our CI. I am not sure whether this is exactly what we need to test apache#7767, and so I would like to request help from @paddyhoran and @rj-atw here. NOTE: this is built on top of apache#8821 . Only the last commit is relevant. Basically, this runs ``` rustup add target wasm32-unknown-unknown cd rust/arrow cargo build --target wasm32-unknown-unknown ``` Note that this does not actually run the tests yet. I think that that is apache#7767 's goal. Closes apache#8864 from jorgecarleitao/wasm32 Authored-by: Jorge C. Leitao <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
This PR refactors the CI with the purpose of making it faster, easier to extend, and more robust. The main consequences of this PR are:
The overall design of this PR is that most steps of the CI process should be independent of all others as much as possible, so that
Intrinsically, the CI pipeline is just a DAG where certain jobs benefit from shared compute and thus depend on common nodes, while others run independently.
This PR re-writes most of our CI as simple github jobs that share caches whenever makes sense, thereby describing the aforementioned DAG. Consequently, many of the sequential steps are now executed in parallel, causing the items describe above.
There are two tasks depending on rust that are not in the
rust.yaml:The bottleneck of a green pipeline are the integration tests, that currently re-compile a significant number of components (as I already flagged on the mailing list), but at least we get faster failures for compilation (2.5m) and clippy (4.5m).