8000 CI: improve test suite runtime via pytest parallelism and disabling mypy in most jobs by rgommers · Pull Request #24291 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

CI: improve test suite runtime via pytest parallelism and disabling mypy in most jobs #24291

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

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

rgommers
Copy link
Member
@rgommers rgommers commented Jul 30, 2023

Should address some of the issues identified in gh-24280.

Regarding the Mypy tests: these tests are super slow, and they're effectively always passing in CI. Running them on all "full" test suite runs is too expensive, so enable them on a single job instead. Note that SciPy has an XSLOW mark, NumPy does not. So use an env var for now. Typical guidelines are that fast tests should be >0.5 sec, slow only in the 0.5-5 sec range, and anything above that should be avoided. The boundaries are fuzzy, but Mypy takes 1-2 minutes to run, so needs custom opt-in. Cc @BvB93 for visibility.

Also adds a spin mypy developer command to make it easier to run only static typing tests.

@BvB93
Copy link
Member

Considering we have a dedicated static typing label, would it be an idea to automatically enable to env var if it's set? Situations wherein the label is present but one is not interested in running the mypy are, realistically speaking, non existent.

@rgommers
Copy link
Member Author

Considering we have a dedicated static typing label, would it be an idea to automatically enable to env var if it's set?

That seems like a good idea to me. In case that is hard to do on, e.g., Azure (since I don't think we have any label-based logic there) we can perhaps also recognize [run mypy] in the commit message.

@rgommers
Copy link
Member Author

The Intel SPR failures seem unrelated:

AssertionError: FPU precision mode changed from 0x77f to 0x37f during the test

The macOS arm64 wheel build jobs looks much better:

image

So this almost halved the runtime compared to before (see here).

The Run cibuildwheel step duration decreased by more than 2x, and average CPU utilization increased from ~1 to ~2.5 (out of 4 cores) during that whole phase.

@rgommers
Copy link
Member Author

And same for the Linux aarch64 jobs, runtime about halved, from 19 min to 9 min 30s on average:

image

@BvB93
Copy link
Member
BvB93 commented Jul 30, 2023

...Azure (since I don't think we have any label-based logic there) we can perhaps also recognize [run mypy] in the commit message.

Hm, a shame that it cannot be done automatically for Azure, but so be it; directly using the commit message is a decent plan B.

Just brain storming here at this point, but how feasible would it be to automatically run it if a change is detected in one of the .pyi files?

@rgommers
Copy link
Member Author

Just brain storming here at this point, but how feasible would it be to automatically run it if a change is detected in one of the .pyi files?

On GitHub Actions one can provide a list of paths to trigger on if those are touched in the PR diff. It'd be a pretty long list, but it's doable. I think on Azure it's again more tricky.

Let me ask a question in return though: what kind of coverage actually makes sense for these tests? I imagine that we'd want macOS/Linux/Windows and also all covered Python versions (currently 3.9-3.12). Does it have to be a dense matrix though across all CI providers? It's hard to see why that would matter. So maybe what would be easier is to ensure we cover that set of OSes and Python versions on GitHub Actions?

Or maybe we can use pyright instead? It's several times faster typically.

@BvB93
Copy link
Member
BvB93 commented Jul 30, 2023

Let me ask a question in return though: what kind of coverage actually makes sense for these tests?

You make a good point here: all that's really important is to check the basic mac/windows/unix + Python version combinations. But anything else? Tests with specific compiler options, 32 bit windows, Etc.? They're all, at minimum, fairly irrelevant here for the purpose of typing related tests and disabling them in full on those platforms is perfectly fine IMO.

As for switching to Pyright? This will be a non-trivial amount of work as our test suite is currently heavily specialised for mypy. There is #21505 on the wish list that should make adapting the test suit for different type checkers a bit easier, but again: that's just on the wish list for now.

@rgommers
Copy link
Member Author

You make a good point here: all that's really important is to check the basic mac/windows/unix + Python version combinations

Thanks for confirming. I'm considering putting that in a separate GHA yaml file - then it's also triggerable by hand via "workflow dispatch" for any maintainer, and we can keep all the paths to .pyi files in a single list. And in addition we have single mypy run in regular CI for PRs that don't touch .pyi files. Running mypy takes about as much time as a full build, so it's n 10000 ot like the new/extra jobs would add a ton of overhead.

@BvB93
Copy link
Member
BvB93 commented Jul 30, 2023

GH also supports wild cars containing file paths for its trigger list, so it's probably easier to just use **.pyi over explicitly enumerating them.

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-including-paths

@rgommers rgommers marked this pull request as draft August 3, 2023 09:43
@rgommers
Copy link
Member Author
rgommers commented Aug 3, 2023

I'm marking this as draft until the new Mypy-specific CI jobs have been added. I am backporting the changes here to the 1.26.x branch, because there we don't need those new jobs, the only purpose of that branch is build system changes and a new release, so cutting the wheel build time by ~2x is very useful there.

[skip azp] [skip circle] [skip travis]
…is set

These tests are super slow, and they're effectively always passing in CI.
Running them on all "full" test suite runs is too expensive. Note that
SciPy has an XSLOW mark, NumPy does not. So use an env var for now.

[skip circle] [skip travis] [skip azp]
@rgommers rgommers force-pushed the improve-ci-runtime branch from bd1bda6 to 3411697 Compare August 15, 2023 10:55
@rgommers rgommers marked this pull request as ready for review August 15, 2023 12:30
@rgommers rgommers added this to the 2.0.0 release milestone Aug 15, 2023
[skip cirrus] [skip travis] [skip circle] [skip azp]
@rgommers rgommers force-pushed the improve-ci-runtime branch from 3411697 to 7de0dbb Compare August 15, 2023 12:36
@rgommers
Copy link
Member Author

This should be good to go now:

  • I added a spin mypy command for convenience, and
  • added 3 dedicated jobs (runtime is only ~15 minutes combined, so should be okay) to run the numpy/typing tests on Linux, macOS and Windows with 3 different Python versions.
  • Those jobs run by default, and only for PRs that touch some clearly unrelated folders like docs/ the runs will be skipped.

python -c "import sys; import numpy; sys.exit(not numpy.test(label='full'))"
# Run full tests with -n=auto. This makes pytest-xdist distribute tests across
# the available N CPU cores: 2 by default for Linux instances and 4 for macOS arm64
python -c "import sys; import numpy; sys.exit(not numpy.test(label='full', extra_argv=['-n=auto']))"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure it is worthwhile distributing tests when there are only 2 cores? Does the total time go down?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's faster. typically ~50% or so

@mattip
Copy link
Member
mattip commented Aug 15, 2023

This comment mentions triggering on the "Static Typing" label and on a [run mypy] commit message. I don't see that in the mypy.yaml, is that follow-on work or is it too hard to do?

@rgommers
Copy link
Member Author

This comment mentions triggering on the "Static Typing" label and on a [run mypy] commit message. I don't see that in the mypy.yaml, is that follow-on work or is it too hard to do?

It's run by default already, so that's not needed. We shouldn't turn it off completely for regular Python/C work in the package, because that can break static typing

@mattip mattip merged commit a2ac8fd into numpy:main Aug 15, 2023
@mattip
Copy link
Member
mattip commented Aug 15, 2023

Thanks @rgommers

@rgommers rgommers deleted the improve-ci-runtime branch August 15, 2023 14:57
charris pushed a commit to charris/numpy that referenced this pull request Nov 5, 2023
This is a backport of numpy#24493 and numpy#24291. The purpose of
this is to ease future backports that expect these files.

- CI: move some jobs in `build_test.yml` to Meson

- CI: split `build_test.yml` into three GHA jobs files

  Also documents better what is being run. See numpygh-24410 for the
  overall restructuring plan for GitHub Actions CI.

- CI: merge `linux_meson.yml` into `linux_blas.yml`

- TST: disable mypy tests in test suite unless an environment variable is set

  These tests are super slow, and they're effectively always passing in CI.
  Running them on all "full" test suite runs is too expensive. Note that
  SciPy has an XSLOW mark, NumPy does not. So use an env var for now.

- CI: add new GHA CI jobs to run MyPy across OS/Python flavors
charris pushed a commit to charris/numpy that referenced this pull request Nov 11, 2023
This is a backport of numpy#24493 and numpy#24291. The purpose of
this is to ease future backports that expect these files.

- CI: move some jobs in `build_test.yml` to Meson

- CI: split `build_test.yml` into three GHA jobs files

  Also documents better what is being run. See numpygh-24410 for the
  overall restructuring plan for GitHub Actions CI.

- CI: merge `linux_meson.yml` into `linux_blas.yml`

- TST: disable mypy tests in test suite unless an environment variable is set

  These tests are super slow, and they're effectively always passing in CI.
  Running them on all "full" test suite runs is too expensive. Note that
  SciPy has an XSLOW mark, NumPy does not. So use an env var for now.

- CI: add new GHA CI jobs to run MyPy across OS/Python flavors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0