chore(tooling,ci): throttle py3 xdist workers locally, use all cores in CI#2120
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2120 +/- ##
================================================
Coverage 86.07% 86.07%
================================================
Files 599 599
Lines 39472 39472
Branches 3780 3780
================================================
Hits 33977 33977
Misses 4862 4862
Partials 633 633
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
How does limiting the number of workers avoid a deadlock? |
|
I'm thinking we maybe combine this with #1393? It should be possible with
And come up with a reasonable number of cores. |
Deadlock was a poor choice of wording! What I mean is: Running Updated the description! :) |
9dde841 to
2b4c639
Compare
I took a look #1393 and commented there: #1393 (comment) According to this, I've just updated this PR to:
This is a great idea! This would def help future-proof our setup for CI runner changes. I'd prefer to defer this to a follow-up issue though (tbh, I need to move on in terms of priorities and patience), but happy if someone else picks it up! I'd just add for the follow-up issue, that this calculation probably won't be set in stone, as new optimizations will impact the memory footprint of generating tests. For now, this PR means:
|
spencer-tb
left a comment
There was a problem hiding this comment.
LGTM for now! I noticed tests_pytest_py3 has --dist=loadgroup but py3 doesn't
Replace `-n auto` with `-n {env:PYTEST_XDIST_AUTO_NUM_WORKERS:6}` in
py3 and tests_pytest_py3 tox envs. Defaults to 6 workers locally to
avoid deadlocks; CI sets the env var to `auto` for full core usage.
Removes the now-redundant `--maxprocesses` flag.
Without `psutil` pytest-xdist reports the number of logical cores.
39a6ae8 to
8cc9f9a
Compare
|
Rebased on forks/amsterdam to get the changes from #2116.
Thanks for flagging! It's the other way round ( We'll likely revisit this flag very soon, as we'll likely somehow also enable |
|
Ah, #1393 is already exactly this issue. I hadn't understood that from the description! I added your comment to #1393's description to make it clearer. |
fselmo
left a comment
There was a problem hiding this comment.
It seems like this helps those of us who run these locally without an additional -n flag. I think this opens up some flexibility which is nice. Approving based on the other support here for this feature 👍🏼
🗒️ Description
This PR changes the xdist behavior of the
py3andtests_pytest_py3tox environments such that:-n auto) instead of being capped via--maxprocesses.PYTEST_XDIST_AUTO_NUM_WORKERS. This lower default intends to avoid xdist utilizing all cores, which leaves the local workstation unresponsive until the end of the pytest run.It additionally adds
psutilto thedevdependency group in order to accurately detect the number of physical cores, cf https://pytest-xdist.readthedocs.io/en/stable/distribution.html#running-tests-across-multiple-cpusResults
py3 runs:
Approach
Replace
-n auto(and-n auto --maxprocesses N) with:Tox resolves
{env:VAR:default}at config-parse time from the host shell. CI workflows set the env var toauto; locally it falls back to6.-n 6PYTEST_XDIST_AUTO_NUM_WORKERS=3-n 3PYTEST_XDIST_AUTO_NUM_WORKERS=auto-n autoWhy not
passenv+PYTEST_XDIST_AUTO_NUM_WORKERSdirectly?xdist reads
PYTEST_XDIST_AUTO_NUM_WORKERSfrom the child process environment to resolve-n autoto a worker count and requires an integer. Passingautothroughpassenvwould cause xdist to crash onint("auto"). By resolving the env var in tox's own substitution layer, the child process only ever sees-n <int>or-n auto(with the env var absent), so xdist never encounters a non-integer value.🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.Cute Animal Picture
🦖