fix(ci): pin xdist auto to the cgroup allocation, not the host's 128 cores#15
Merged
Merged
Conversation
…8 cores `-n auto` was starting ~128 workers into a 48-CPU Spartan lease: 2.7x continuous oversubscription on every CI run in the ecosystem. Measured on spartan-bm155 inside a live 48-CPU lease cgroup (job_27144058/step_extern): os.sched_getaffinity(0) -> 48 (the actual allocation) os.cpu_count() -> 128 psutil.cpu_count(logical=False) -> 128 <- what `auto` resolves to xdist's pytest_xdist_auto_num_workers consults psutil FIRST and returns immediately; only if psutil is ABSENT does it fall back to sched_getaffinity, which would have been correct. So `auto` is right without psutil and wrong with it. Corroborated independently in scitex-hub CI, whose worker ids reached gw121. PYTEST_XDIST_AUTO_NUM_WORKERS is consulted AHEAD of psutil (verified in pytest-xdist v3.8.0 source: the env var is read at the top of the hook and returns before psutil is imported), so exporting it corrects `auto` without hardcoding a worker count. `-n auto` is kept. Not keyed on $SLURM_CPUS_PER_TASK / $SLURM_JOB_CPUS_PER_NODE: both are EMPTY on these runners. The runner is ssh-adopted into the lease cgroup, so it inherits the cpuset but NOT the step environment. sched_getaffinity reads the cpuset directly and is the only thing that works. ADR-0004 P1(b) prescribed the SLURM-var form and was wrong; already corrected in scitex-dev. sched_getaffinity is Linux-only, which is safe here: runs-on is the fixed label [self-hosted, Linux, X64, spartan-cpu] and the matrix varies only python-version, so no non-Linux leg exists. Uses .venv/bin/python explicitly, matching the surrounding steps.
This file contains hidden or 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
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.
The defect
pytest -n autoin the org-sharedpytest-matrix.ymlstarts ~128 workers into a 48-CPU Spartan lease — 2.7x continuous oversubscription on every CI run across the ~74 repos that call this workflow.Measured directly on
spartan-bm155, inside a live 48-CPU lease cgroup (job_27144058/step_extern):os.sched_getaffinity(0)os.cpu_count()psutil.cpu_count(logical=False)-n autoresolves toSLURM_CPUS_PER_TASKSLURM_JOB_CPUS_PER_NODEIndependently corroborated: scitex-hub's CI log shows worker ids reaching
gw121.Mechanism
xdist's
pytest_xdist_auto_num_workersconsults psutil first and returns immediately. Only if psutil is ABSENT does it fall back tosched_getaffinity, which would have been correct. Soautois right without psutil and wrong with it.PYTEST_XDIST_AUTO_NUM_WORKERSis read at the top of that hook and returns before psutil is imported — verified against pytest-xdist v3.8.0 source:So exporting it corrects
autorather than replacing it.-n autois kept; no worker count is hardcoded.Why not the SLURM vars
Both are EMPTY on these runners. The runner is ssh-adopted into the lease cgroup, so it inherits the cpuset but not the step environment. Any fix keyed on
$SLURM_CPUS_PER_TASKsilently produces nothing. ADR-0004 P1(b) prescribed exactly that and was wrong; already corrected in scitex-dev.The change
One line in
.github/workflows/pytest-matrix.yml, immediately before the pytest invocation (verified at line 89 onmain), plus explanatory comments:Uses
.venv/bin/pythonexplicitly, matching the surrounding steps — the venv is created byuv venv --seedearlier in the same step, so the interpreter provably exists.Platform safety
sched_getaffinityis Linux-only. Checked rather than assumed:runs-onis the fixed label[self-hosted, Linux, X64, spartan-cpu]and the matrix varies onlypython-version, so no non-Linux leg exists. No guard needed.Scope
git grepacross all tracked files onmainfor-n auto/xdist/numprocesses/PYTEST_XDIST:pytest-matrix.ymlis the only file that matches. No other workflow shares the pattern; nothing else to fix.Verification
yaml.safe_load), job/matrix/steps structure intact.run:block passesbash -n.sched_getaffinity == cpu_count == 16, so the 48-vs-128 distinction is invisible here. Not claimed as verified.Pending confirmation: after merge, the proof is a CI run whose worker ids top out near the allocation (~gw47) instead of
gw121.