fix(train): CpuInfo compared a machine-wide core count against a process-restricted one - #2445
Closed
noahgift wants to merge 1 commit into
Closed
fix(train): CpuInfo compared a machine-wide core count against a process-restricted one#2445noahgift wants to merge 1 commit into
noahgift wants to merge 1 commit into
Conversation
…ess-restricted one
Coverage Nightly went red on main this morning after 7451 passing tests:
thread 'efficiency::device::tests::test_cpu_info_detect' panicked at
crates/aprender-train/src/efficiency/device/tests.rs:50:5:
assertion failed: cpu.threads >= cpu.cores
test result: FAILED. 7451 passed; 1 failed
make: *** [Makefile:339: coverage] Error 1
The two fields are measured against different denominators. `threads` comes from
std::thread::available_parallelism(), which is cgroup- and affinity-aware and
reports what THIS process may run on. `cores` came straight from /proc/cpuinfo,
which describes the whole machine and honours no restriction at all. Whenever the
process is CPU-restricted the second number is the larger one and the invariant
inverts.
Measured on the runner that failed, a 24-physical-core host:
$ awk -F: '/^physical id/{p=$2} /^core id/{print p"-"$2}' /proc/cpuinfo | sort -u | wc -l
24
$ nproc # unrestricted
48
$ taskset -c 0,1 nproc # what a restricted process sees
2
so the assertion became `2 >= 24`. That is not a flake: with 16 concurrent CI jobs
on one box it is the expected reading, and it aborts the run with the remaining
tests unexecuted, so a real regression behind it would be invisible.
cpu.rs now reconciles the two in `usable_cores`, clamping the machine-wide physical
count to the parallelism actually available. `cores` consequently means "physical
cores this process can use", which is what its consumers need --
estimated_memory_bandwidth_gbps sizes from it, and sizing from cores the scheduler
will never grant is wrong independently of the test.
Mutation check. Restoring the old expression, keeping the new tests:
detected_physical.unwrap_or_else(|| threads.max(1)) // MUTATION: pre-fix behaviour
test usable_cores_never_exceeds_available_parallelism ... FAILED
test usable_cores_upholds_the_cores_le_threads_invariant ... FAILED
assertion `left == right` failed: a process allowed 2 CPUs must not report
24 usable physical cores
test result: FAILED. 47 passed; 2 failed
Note that test_cpu_info_detect stayed GREEN under that mutation, because this host
is unrestricted. That is exactly why the defect appeared only on a loaded CI runner
and never locally, and why the new tests drive the reconciliation over its input
space instead of over whatever the current machine happens to be.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
noahgift
enabled auto-merge
August 11, 2026 07:23
Contributor
Author
|
Superseded by #2449 — this branch is merged verbatim into that batch. The binding constraint was one ~50-minute Closing rather than leaving open so this PR cannot move #2449's base and force it to re-run. The branch is untouched and this is reopenable if the batch does not land. |
auto-merge was automatically disabled
August 11, 2026 13:39
Pull request was closed
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.
Coverage Nightly is red on
mainafter 7451 passing tests:threadscomes fromavailable_parallelism()— cgroup- and affinity-aware, so it reports what this process may use.corescame from/proc/cpuinfo, which describes the whole machine and honours no restriction. Different denominators; under CPU restriction the machine-wide number is the larger one and the invariant inverts.Measured on the 24-physical-core runner that failed:
so the assertion became
2 >= 24. Not a flake — with 16 concurrent CI jobs on one box that is the expected reading, and it aborts the run with the rest of the suite unexecuted, so a real regression behind it would be invisible.usable_coresnow clamps the machine-wide physical count to the parallelism actually available, socoresmeans "physical cores this process can use" — which is what consumers need (estimated_memory_bandwidth_gbpssizes from it).Mutation check — old expression restored, new tests kept:
test_cpu_info_detectstayed green under that mutation, because this host is unrestricted — which is exactly why the defect only ever appeared on a loaded CI runner. The new tests drive the reconciliation over its input space instead of over whatever the current machine happens to be.🤖 Generated with Claude Code