Skip to content

fix(train): CpuInfo compared a machine-wide core count against a process-restricted one - #2445

Closed
noahgift wants to merge 1 commit into
mainfrom
fix/cpu-info-cores-threads-cgroup
Closed

fix(train): CpuInfo compared a machine-wide core count against a process-restricted one#2445
noahgift wants to merge 1 commit into
mainfrom
fix/cpu-info-cores-threads-cgroup

Conversation

@noahgift

Copy link
Copy Markdown
Contributor

Coverage Nightly is red on main 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

threads comes from available_parallelism() — cgroup- and affinity-aware, so it reports what this process may use. cores came 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:

$ 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. 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_cores now clamps the machine-wide physical count to the parallelism actually available, so cores means "physical cores this process can use" — which is what consumers need (estimated_memory_bandwidth_gbps sizes from it).

Mutation check — old expression restored, new tests kept:

detected_physical.unwrap_or_else(|| threads.max(1))  // MUTATION

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_cpu_info_detect stayed 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

…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

Copy link
Copy Markdown
Contributor Author

Superseded by #2449 — this branch is merged verbatim into that batch.

The binding constraint was one ~50-minute workspace-test per PR on one shared box; nine concurrent PRs starved each other into 75-minute step timeouts (every blocked PR this morning classified as CONTENTION, zero merges in 4.5 hours). #2449 lands 24 branches in a single CI run.

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.

@noahgift noahgift closed this Aug 11, 2026
auto-merge was automatically disabled August 11, 2026 13:39

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant