fix(partition): 🐛 place partitions inside the cgroup the batch system carved - #249
Merged
Conversation
diagonal-hamiltonian
requested review from
fpietra,
ludmilaasb and
robertodr
as code owners
August 19, 2026 20:17
github-actions
Bot
added
documentation
|
Docs preview: https://pr-249.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
diagonal-hamiltonian
force-pushed
the
pr/partition-cgroup-placement-v2
branch
from
August 19, 2026 21:16
2812589 to
153be12
Compare
… carved Placement must not divide an already-divided machine. `enumerate_physical_cores` reports only the cores inside the calling thread's affinity mask. When a launcher has already given each co-located rank its own disjoint slice -- `srun --cpu-bind=cores`, or any cgroup-confined launch -- that slice IS the rank's share. Passing the node-wide ranks-per-node through as `group_count` then asks for group_count x n cores out of a list that only ever held n, `placement_order` correctly refuses, and every rank silently runs UNPLACED. The two-level barrier loses its domains at the same time, because `cpuset_domains` derives them from the placement. Measured on 8 ranks x 16 partitions: 437 us/sync unplaced against 15.5 us/sync placed. `PartitionGroup` therefore allgathers the masks over its node-local communicator and `classify_node_mask` MEASURES disjointness; a `NodeMask::PerRank` result collapses `group_count` to 1. Mask width cannot substitute for that measurement: "8 ranks holding 16 cores each" and "8 ranks sharing one 16-core mask" both leave a rank seeing 16 of 128, and they need opposite placement. Collapsing in the wrong direction pins every co-located rank to the SAME cores, so `Shared` is the default and the safe error. This regressed once already, when topology discovery was rewritten onto hwloc, because the guard lives in the placement policy rather than in discovery -- any rework of that layer has to re-check it. `cpu_topology_policy_per_rank_slice_starves_without_collapse` pins the mechanism without needing live hardware, which is what makes it a regression test rather than a machine-specific one. Assisted-by: ClaudeCode:claude-opus-5
…ve hardware The fix as committed added no hardware-free test case. It extends one live-topology case, which early-returns when fewer than two physical cores are visible or pinning is off -- so on a constrained runner the whole mechanism can go unexercised while the suite reports green. That matters more than usual here: this regressed once already, when topology discovery moved onto hwloc, because the guard lives in the placement policy rather than in discovery. So the rule becomes a free function. masks_are_pairwise_disjoint() is pure bit arithmetic over the array MPI_Allgather already leaves behind -- no communicator, no hwloc, no live hardware -- and classify_node_masks_ keeps only the exchange and the all_ok reduction. Six cases are ported from the competing implementation on perf/multinode-comm-scaling, which spelled the same mechanism as a NodeMask enum over a CpuMask POD; dropping that implementation in favour of this one would otherwise have dropped its tests with it. The empty-mask clause is folded into the predicate rather than left implicit. An all-zero mask is trivially disjoint from everything, so a bare disjointness test answers "private" for a rank that can see no CPU at all -- collapsing group_count in exactly the case where the caller knows least. Not private is the safe error. kMaskWords moves out of PartitionGroup and beside affinity_mask_words as kAffinityMaskWords: the two must agree on the width and a constant living next to only one of them can drift silently. process_is_cpu_confined() and its helper are deleted -- 40 lines of public-header API with no caller, no test, and a Doxygen block warning against its own use. Corrects the AGENTS.md paragraph, which described the OTHER branch's design: classify_node_mask, NodeMask::PerRank and Shared appear nowhere in this tree, and neither did the test it cited. Replaces the 437 -> 15.5 us/sync figure, which is a pair of range midpoints measured on the pre-hwloc tree with a different branch as the arm, with the placement counts the /proc probe takes on both arms (0 vs 16 threads pinned per rank), and records what the record actually says: at 99.4M terms this does not make the workload faster, and pinning did not reduce variance either. Assisted-by: ClaudeCode:claude-opus-5
The rewrite in b3d788d left "every rank silently runs unpinned -- which also costs every rank silently runs unpinned": half of the deleted clause and its own replacement, side by side. The clause being deleted was the fix commit's "the two-level barrier loses its domains at the same time, because `cpuset_domains` derives them from the placement". `cpuset_domains` is the OTHER branch's vocabulary -- `git grep` finds it nowhere in this tree -- so the clause goes rather than being repaired, and the consequence stated is the one this branch can actually show: every rank runs unpinned. Assisted-by: ClaudeCode:claude-opus-5
…ment count kAffinityMaskWords and affinity_mask_words() "must agree", but they cannot be tied by a static_assert: the function takes the width as a runtime `nwords` argument, as does masks_are_pairwise_disjoint()'s `words`, so there is no shape to assert against. Hoisting the constant out of PartitionGroup (b3d788d) is the whole fix -- there is now one definition, and the drift it could have had was a second one. What is assertable is the constraint the exchange imposes on the value: PartitionGroup passes the width to MPI_Allgather as an element count, and MPI counts are `int`. State it where the constant is declared, so a future widening past INT_MAX fails to compile rather than passing a truncated count. No behaviour change: a static_assert emits no code, and the objects prove it. Recompiled with the job-1828023 build's own flags, CpuTopology.cpp.o, Evolution.cpp.o (which reaches this header through MonomialPropagator.h -> PartitionGroup.h) and partition_equivalence_tests.cpp.o are byte-identical with and without this hunk. Assisted-by: ClaudeCode:claude-opus-5
…t cases A case that early-returns on a constrained runner reports green while testing nothing, which is the exact defect the ported cases exist to fix -- and three of them had it. cpu_topology_shared_mask_keeps_co_located_ranks_disjoint required FOUR physical cores. A standard GitHub runner has two, so on CI it returned at the first line, every time. Two cores are enough: one per rank still asserts that two co-located ranks do not share one. cpu_topology_per_rank_mask_still_places required two. One is enough -- the request is group_count x n out of a list holding n at any n -- so the window shrinks from "fewer than two cores" to "hwloc loaded no topology at all", which is the condition under which nothing in this file means anything. cpu_topology_affinity_mask_covers_enumerated_cores returned silently when affinity_mask_words() said no. But enumerate_physical_cores() has just succeeded at that point, so hwloc demonstrably works and the only licensed refusal is the truncation guard. It now asserts that some enumerated PU really does sit past kAffinityMaskWords * 64; a mask function that had simply stopped working used to read as "nothing to test". Two premises are checked rather than assumed. confine_to_first() re-enumerates and requires the narrowing to have reached hwloc -- if sched_setaffinity ever stopped moving what effective_allowed_cpuset() sees, both live cases would go on asserting against the unconfined machine and go on passing, which is an early return that does not look like one. And an empty placement is now licensed only by monoprop_PARTITION_PINNING being off; "placed nothing" is the bug this branch fixes and must never pass as a configuration. What remains vacuous, and cannot be fixed here: hwloc loading no topology, and the kernel refusing sched_setaffinity. Both are named at the return. The mechanism itself is covered hardware-free by cpu_topology_policy_per_rank_slice_starves_without_collapse and cpu_topology_masks_*, which have no early return at all. Assisted-by: ClaudeCode:claude-opus-5
…nt retry The MPI_Allreduce over the per-rank affinity_mask_words result cannot change a verdict: refusal leaves the mask zeroed, and masks_are_pairwise_disjoint already rejects an all-zero row, so every peer computes "not private" from the allgathered buffer alone. The private-mask collapse becomes unconditional rather than a retry after the normal split has failed, which removes the second call to placement_order. A rank whose mask is private owns all of it, so it now takes the head of its own interleaved order instead of a group_index-offset slice of it even when the split would have succeeded.
… is impossible group_count == 0 never advanced the interleave arm's stride, so `mine` grew without bound. It joins the existing oversubscription guard. An empty placement is the failure this branch exists to fix and it was returned silently. One warning per process now names the core count and the request it could not meet.
…nal collapse
The truncation-guard assertion compared kAffinityMaskWords*64 against the
core representatives, which are the LOWEST allowed sibling of each core,
while affinity_mask_words refuses on the HIGHEST allowed PU. A core holding
PUs {100, 4196} refuses with a representative of 100. It now re-reads the
mask through a wider window and asserts against its own highest bit.
empty_placement_is_licensed() moves out of the __linux__ block so the two
live smoke cases can use it; both hard-failed under
monoprop_PARTITION_PINNING=0.
The new policy case covers a private mask whose split WOULD have fit, which
the fallback form never reached.
…t say The AGENTS.md bullets restated the header and the PR body; both are dropped. What survives elsewhere is the empty-mask rule, what the synthetic core literals stand for physically, and why two cores rather than four are enough on a CI runner.
The private-mask arm needs n cores, the shared arm group_count x n; the clause named only the former. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
robertodr
force-pushed
the
pr/partition-cgroup-placement-v2
branch
from
August 20, 2026 07:54
fb2453a to
655ea3c
Compare
robertodr
approved these changes
Aug 20, 2026
|
This was referenced Aug 20, 2026
robertodr
pushed a commit
that referenced
this pull request
Aug 20, 2026
…bool (#252) 🤖 _AI text below_ 🤖 ## Summary Follow-up to #249, which shipped the cgroup placement fix with a `bool mask_is_private` parameter. This renames that parameter to `enum class NodeMask { Shared, PerRank }`. No behaviour change. `partition_cpusets(n, gi, gc, true)` does not say at the call site which regime `true` means, and the safe default is spelled `false`. The enum names both, and a third launcher regime would be a new enumerator rather than a second bool. ## Changes - `enum class NodeMask { Shared, PerRank }` in `CpuTopology.h`. - `partition_cpusets`'s fourth parameter becomes `NodeMask mask = NodeMask::Shared`. - `PartitionGroup::node_mask_private_` becomes `NodeMask node_mask_`; `classify_node_masks_` converts `masks_are_pairwise_disjoint`'s `bool` at the call site. The predicate keeps its `bool` return -- it answers a question about masks, not about policy. - Test call sites lose their `/*mask_is_private=*/` argument comments, which the enum value replaces. Every assertion and every test-case name is unchanged, and `grep -rn mask_is_private cpp/` is empty. ## Checklist - [x] Tests added or updated to cover the changes - [ ] Documentation updated (docstrings, `docs/`, `CONTRIBUTING.md`) if needed - [ ] `CHANGELOG` / release notes updated if applicable ## AI/LLM disclosure - [ ] I did not use LLM tooling, or used it only privately for ideation - [x] I used the following tool to help write this PR description: Claude Code - [x] I used the following tool to generate or modify code: Claude Code Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
6 tasks
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
2 participants
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.
🤖 AI text below 🤖
Summary
placement_orderrefuses whenevergroup_count * n > cores.size(). Under a per-rank cgroup that isthe ordinary case, not an edge case:
cores.size()is 16 (the cgroup Slurm carved) whilegroup_count * nis8 * 16 = 128(the whole node). It returns empty,cpusets_stays empty,master_loop_skipspin_this_thread, and every rank runs unpinned — with no diagnostic, becausepin_this_threadignores bind errors by design.A rank cannot settle this from its own mask: seeing 16 of 128 CPUs is equally "Slurm gave me my own
16" and "eight of us share these 16", and those want opposite placements. Only the peers' masks
separate them, so
PartitionGroupnow measures it instead of guessing.Changes
classify_node_masks_allgathers each co-located rank's affinity mask over the node communicatorit already opens (512 B per rank, once per group construction), and
masks_are_pairwise_disjointdecides. A mask too wide to represent is exchanged as zeros, and an all-zero row is never private:
"shared" is the safe error.
partition_cpusetsgainsmask_is_private. Under it the group split is skipped outright — thelauncher already separated the ranks, so subdividing our own cgroup again can only shrink the
placement or empty it.
every thread unpinned. That silence is what this PR is about.
placement_orderrefusesgroup_count == 0, which would otherwise loop without advancing.Gap worth stating: the rule is unit-tested as a free function over the gathered array, but the
exchange in
classify_node_masks_has no MPI test — only the end-to-end runs below cover it.Measurement
Two prebuilt arms, one allocation per cell holding both, order flipped per
(rep, cell), 10 reps,median of paired per-rep ratios, two-sided sign test, Holm across the 24-test family.
main
216d7e2f/ port6ad4ffec. Term counts identical across arms in all 12 cells.Placement, from
/proc/self/task/*/status, reduced over ranks:8 of 24 tests clear Holm,
P(X ≥ 8)= 1.5e-16:grid-pauli-Bgrid-pauli-Bgrid-hubbard-Bgrid-hubbard-graph-Bgrid-pauli-Bgrid-pauli-Bgrid-pauli-Bgrid-pauli-AAll twelve layout-B tests point the port's way; seven of them clear Holm. The largest single
effect is not in that table:
propagate[hubbard]at B/N=2 goes 16834.2 → 15083.6 ms (1.12xfaster), which at 9/10 does not survive Holm at
m = 24.The last row is a 1% regression at a layout where the fix cannot act, and is reported as measured.
At layout A the process mask is unconfined,
mask_is_privateis always false, and at N=1 there is noMPI at all — none of the new code runs. The other eleven layout-A tests scatter 0.98x–1.01x with no
direction. Read it as this grid's known sensitivity to binary layout, not as a mechanism.
Peak RSS is flat (one cell 0.98x, the rest 1.00x). Gates:
ctest -L unit216/216,-L serial215/215, MPI suite 592 passed at every layout.
Not reproducible from this diff. Deucalion, 2× EPYC 7742 / 128 cores / SMT off / NPS4; a private
harness driving two prebuilt venvs with the hashes above, which is not proposed for this repo.
Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableAI/LLM disclosure