search

rust-ndarray / ndarray

ndarray: an N-dimensional array with array views, multidimensional slicing, and efficient operations
https://docs.rs/ndarray/
Apache License 2.0
3.43k stars 295 forks source link

Further ci updates - numeric tests, and run all tests on PRs #1369

Closed bluss closed 4 months ago

bluss commented 4 months ago

Somehow, numeric test take 30m on powerpc, which is too slow (much slower than anything else.) Now use s390x instead for big endian coverage, and release mode, which seems to use 15 minutes.

All the regular tests move back to PR checks to give contributors feedback on their PRs. It is more productive if contributors get access to the Rust 1.51 buildbot results immediately themselves.

adamreichold commented 4 months ago

I think running them in release mode is a good call, especially with the CI caching the builds. But I also remember that this became significantly worse when I switched the big endian build from MIPS to PowerPC. I wonder if there is another big endian tier 2 target we could use which has faster emulation? s390x-unknown-linux-gnu because it is CISC and hence faster to emulate?

bluss commented 4 months ago

That sounds good - s390x- because even release mode powerpc is not so fast, also considering just not testing big endian anymore.

I'm still tinkering with the CI.

My current thought is that all the regular tests need to move back to PR checks, because it just encourages maintainer busywork if we have to be in the loop to give contributors feedback on their PRs. More productive if contributors get access to the Rust 1.51 buildbot results immediately themselves (because it's a tricky one, yeah it's too old, and nobody runs that on their machine.)

adamreichold commented 4 months ago

My current thought is that all the regular tests need to move back to PR checks, because it just encourages maintainer busywork if we have to be in the loop to give contributors feedback on their PRs. More productive if contributors get access to the Rust 1.51 buildbot results immediately themselves (because it's a tricky one, yeah it's too old, and nobody runs that on their machine.)

Agreed, the tests should be run on all PR, at least part of the matrix and your reasoning for 1.51 makes sense to me. I think the cross tests could still be limited to the merge queue. (I would also argue for keeping the merge queue as it makes the state of the main branch more reliable.)

That sounds good - s390x- because release mode is not so fast, also considering just not testing big endian anymore.

I would prefer keeping it and trying to make it faster for a bit more. Too much unsafe in the code base to pass any available testing. (I would even argue adding a cargo-careful run to the merge queue since we probably cannot use Miri with the FFI interactions.)

bluss commented 4 months ago

By default the merge queue runs many PRs concurrently, but for now I set it to sequential. Thanks for the help with this, now it's at least slightly modernized.