Conversation
|
another benefit here is just seeing the impact on overall runtime of individual tests under miri |
|
I've pushed another change that runs all crates in a single invocation so nextest can better parallelize between the crates, it seems like just |
|
Locally this PR takes the runtime to around ~5 minutes for all but the following tests (obviously also because its a much more powerful machine). The 3 slowest tests are:
I think the first can be specialized into less cases with a edit: FWIW - this change as is and end-to-end also saves around 30 minutes. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @AdamGS -- my only concern is the use of the github action
I also reviewed the timings and this PR does help shave about 30 minutes off the run:
This PR (1h 57m 27s): https://github.com/apache/arrow-rs/actions/runs/23751988211/job/69196496005?pr=9629
Main (2h 29m 46s): https://github.com/apache/arrow-rs/actions/runs/23801294865/job/69362162088
| cargo miri test -p arrow-ord | ||
| cargo miri test -p arrow-array | ||
| cargo miri test -p arrow-arith No newline at end of file | ||
| cargo miri nextest run \ |
There was a problem hiding this comment.
There was a problem hiding this comment.
nextest is probably the most important non built in tool working in rust for my workflow
| rustup toolchain install nightly --component miri | ||
| rustup override set nightly | ||
| cargo miri setup | ||
| - name: Set up nextest |
There was a problem hiding this comment.
This action scares me security wise (as it runs some bash script)
https://github.com/apache/arrow-rs/actions/runs/23751988211/job/69196496005?pr=9629
I think it would be better (if not ideal) to install nextest explicitly (cargo install cargo-nextest --version 0.9.132 --locked) rather than use this pre-built binary
There was a problem hiding this comment.
yeah absolutely, I'll change it.
How would you feel ignoring those long tests and minimizing the first one? I think it might shave the runtime here significantly
There was a problem hiding this comment.
How would you feel ignoring those long tests and minimizing the first one? I think it might shave the runtime here significantly
Let's do it
maybe we can make a separate PR to do so (so we can review the changes in isolation)
There was a problem hiding this comment.
SGTM, 3b003ec just installs nextest using cargo install with a locked version, once this merges I'll open up a follow up.
There was a problem hiding this comment.
It seems like this adds 3 minutes to the job
https://github.com/apache/arrow-rs/actions/runs/23844918251/job/69509710473?pr=9629
However, we are ahead of where main is
| cargo miri test -p arrow-ord | ||
| cargo miri test -p arrow-array | ||
| cargo miri test -p arrow-arith No newline at end of file | ||
| cargo miri nextest run \ |
| rustup toolchain install nightly --component miri | ||
| rustup override set nightly | ||
| cargo miri setup | ||
| - name: Set up nextest |
There was a problem hiding this comment.
It seems like this adds 3 minutes to the job
https://github.com/apache/arrow-rs/actions/runs/23844918251/job/69509710473?pr=9629
However, we are ahead of where main is
2 participants
Which issue does this PR close?
Rationale for this change
Miri in CI is VERY slow (around 2.5 hours), but the github runners actually have 4 vCPUs and some memory, so using nextest can give us some speedup.
What changes are included in this PR?
Install nextest in CI and then use it to run Miri
Are these changes tested?
tested the script locally
Are there any user-facing changes?
No