Start using xdist in pytest opts #1074
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus,
kurtmckee,
m1yag1 and
MaxTueckeGlobus
as code owners
kurtmckee
approved these changes
Jan 13, 2025
|
I got coverage reporting failure because I didn't account for the parallel coverage settings. I'm going to get that sorted out and then this should be nice and clean. EDIT: I'm actually a bit confused right now. Local coverage is busted for me but we clearly have the options set for it. |
|
I have to convert this to a draft for the moment (should convert back later today). I've run into some issues with local coverage and need to test more to proceed. Notably, I'm also rebasing this onto #1070 to pick up the improved config there. I've had to customize our |
We had dependency data for tests in a `test` extra, which is not intended to be part of our public package interface. Move it to a dependency group and remove the `test` extra. Update usage of `.[test]` in the `Makefile`, which appears to be highly outdated but which could be in use for some developers. The `test` group had an entry for `setuptools` which appears to be unnecessary, so it is removed here. The dependency update script (`make update-dependencies`) gets a small update to find where `mypy` is currently pinned.
Of necessity, this change also updates the min bound for `requests` (for simplicity, to the latest version). Moving these data into a dependency group revealed that our bound for requests was not effective because it was in conflict with our version of `responses`. With `deps` installed before the package and test dependencies, we were actually not using the old version of `requests` which was declared. This change fixes the issue for the most part. It's still notable that package install and dependency group installation are separate steps in tox and can conflict.
It does not appear to be necessary to pass `GLOBUS_SDK_PATH` to a subprocess, which won't work under `tox_uv` anyway (because the virutalenv won't have `pip`). Most likely, this method of passing is something which didn't work in tox v3 and which v4 fixed. (It seems unlikely that we wouldn't have tried this config.) In any case, the new simpler config works fine.
- `testenv:clean` was still using `setup.py clean`! Remove. - Make them all inherit from a common `coverage` env. - Use the `coverage` dependency group to share with `test` deps - Declare `depends` correctly (fixes `tox p` usage!)
We now use Trusted Publishers config, so this is no longer needed.
This imitates our prior art in globus-sdk to move `allowlist_externals = rm` usage into a plugin which can take care of this for us. It also sets up `toxfile.py` for us in case we want to put any other customizations in there.
We have `pytest-xdist` already installed, but we aren't using it. Set `-n auto --maxprocesses 8` in our config, so that it will be activated. The max prevents `-n auto` from starting large numbers of workers (e.g., 20) and losing any speed benefit to overheads. Using `-n auto` rather than (e.g., `-n 4`) ensures that in CI and other variable environments, we get the right number of workers. One test with a randomized parameter needed a tweak, as it made an xdist parameter consistency check fail.
Primarily, add a new plugin configuration option which customizes the test environment with a sitecustomize.py to launch coverage automatically. Additionally, customize `set_env` via this invocation path. In aggregate, this is a tox-plugin version of the type of behavior used by `pytest-cov`. Because the `coverage.process_startup()` call adds some overhead, highly parallel runs of the tests could start to fail under pytest-timeout. `tox p` parallelization on top of `pytest-xdist` parallelization became too aggressive, resulting in slow starts when timing was bad. Scaling maxprocesses back down to 4 (no appreciable difference in speed between 4 and 8) results in fast, successful runs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have
pytest-xdistalready installed, but we aren't using it.Set
-n auto --maxprocesses 8in our config, so that it will be activated. The max prevents-n autofrom starting large numbers of workers (e.g., 20) and losing any speed benefit to overheads. Using-n autorather than, for example,-n 4ensures that in CI and other variable environments, we get the right number of workers.One test with a randomized parameter needed a tweak, as it made an xdist parameter consistency check fail.
After CI runs for this setting, I'll be able to share overall numbers from that context.
In my local testing, this was a 4x speed boost.