feat(partition): 📈 report the launcher's placement, and delete the pinning knob by diagonal-hamiltonian · Pull Request #269 · Algorithmiq/monoprop

diagonal-hamiltonian · 2026-08-21T10:10:38Z

🤖 AI text below 🤖

Summary

Adds a report-only diagnostic for the CPU placement the launcher actually gave each rank, and
deletes the knob that made pinning optional. feat/commplace-placement-report 7e376be, 3 commits on
cb9a033, 7 files +218/−51.

monoprop_COMMPLACE reports the placement the launcher actually gave each rank. One
COMMPLACE line per rank on stderr at propagator construction, naming the CPUs the launcher gave
that rank and whether co-located ranks got disjoint masks (masks=private vs masks=shared);
default off. The flag is a noise gate, not a cost gate, and it is worth being exact
about that: the MPI_Allgather of the masks runs unconditionally — it is how NodeMask is
picked, and it predates this PR — so what the flag avoids is one hwloc bitmap pass over the
gathered rows plus a std::format, once per propagator construction. Measured volume is one
line per rank per process (2 lines for a 2-rank bench), and PartitionGroup is built only
under n_partitions > 1, so serial runs are silent either way — the whole Python suite emits
nothing with the knob on. It earns its place by keeping a library from writing to stderr
unbidden, not by saving work. The diagnostic earns a knob
because --cpu-bind=none and --cpu-bind=cores give the same thread count and the same partition
count, differing only in the mask the kernel enforces — a difference that has already cost a 1.45x
tax nobody could see. A rank holding 16 of a host's 128 CPUs is equally "Slurm gave me my own 16"
and "eight of us share these 16", and sampling /proc cannot separate them: it describes the
process you are in, never a peer rank on the same host. PartitionGroup already exchanges the
masks to pick a placement, so this reports what that exchange saw — no new collective. The flag
gates printing only: the exchange is deliberately not gated on it, since the environment is
per-rank, so a predicate over it is not rank-uniform, and one rank skipping a collective its peers
entered is a hang, not a missing diagnostic. format_place_line returns the line rather than
writing it, which is what keeps the formatting reachable in a test binary launched without the
knob set.

Known gap, deliberately not closed here. A flag cannot help someone who does not know to
set it, and that is the person who loses the 1.45x. The case worth catching unprompted is
node_size > 1 with masks=shared; partition_cpusets three functions away already warns
unconditionally via std::call_once when pinning is impossible, so both the precedent and the
pattern exist. A follow-up should warn there. This PR ships the recording half only.

Consequence of adding a boolean knob: off now means off. The shared parse_flag matched only
the first character against {0,f,F,n,N}, so monoprop_COMMPLACE=off would have switched the
diagnostic on. It now compares whole words, case-insensitively, against 0|false|no|off,
keeping its (value, default_value) signature so a default-ON flag stays expressible. Deliberate
consequence: bare f and n were falsey and are now truthy; a test pins that, and is
mutation-verified — restoring first-character parsing fails it.
monoprop_PARTITION_PINNING is deleted; pinning is now unconditional. Not because its parser
was wrong, but because the experiment that would have used it is refuted: see below. No campaign
ran on the wrong arm and none could have — no harness script ever set the variable, verified across
the whole harness repo, so nothing needs re-auditing.

Not shipped: the placement deletion this was salvaged from. Its pre-registered falsifier was
abandon if >5% cost anywhere, and propagate[hubbard] at layout A/N=1 came in 2.90x slower, 10
of 10, with 11 of 12 layout-A tests slower. Refuted by its own criterion — which is why the knob
can be deleted rather than fixed. (Correcting an earlier draft of that work: main places at
both layouts, 128 at A and 16 at B.)

Two corrections to an earlier revision of this PR:

It listed disabled among the values the old parser read as ON and the new one fixes. disabled
parses as ON under both rules, so it is not evidence of anything. Only off/OFF/Off change
behaviour. With that removed, the parser change stops being "we found and fixed a bug" — the knob
whose parser had the bug is deleted by this same PR, and nothing ever set it — and becomes what it
is: a footgun removed from the knob being added. It is presented that way above.
It claimed zero overlapping files with the open PRs. That holds against perf(evolution): ⚡ halve the follower-marking epoch stamp #259 and perf(evolution): ⚡ carry cross-rank queries as a width-adaptive record #263 only.
test(cpp): ✅ migrate unit tests to Catch2 #264 rewrites both test files this PR touches (cpu_topology_tests.cpp,
env_config_tests.cpp) to Catch2, and feat(partition): 🧬 let derived propagators build correctly-typed partition children #265 touches PartitionGroup.h. No semantic
interaction in either case, but both are textual conflicts in one order or the other.

Changes

monoprop_COMMPLACE: one COMMPLACE line per rank on stderr at propagator construction,
reporting the launcher's actual CPU mask and masks=private/shared/alone/unknown. Off by
default; nothing is computed when off. alone is not private — nothing to be disjoint from is
not evidence a multi-rank launcher did the right thing — and unknown is what a mask too wide for
the exchanged window reports, rather than a plausible zero.
summarize_masks is the one exported helper behind the line: hwloc_bitmap_weight/_or/
_list_snprintf do the popcount, union and range formatting, in a TU that already includes
<hwloc.h>. Truncation is stated as a trailing ,+ and cuts back to the last whole range, so the
marker never follows a half-written CPU id.
monoprop_PARTITION_PINNING deleted; pinning is unconditional, and parse_flag matches whole
words instead of first characters.
docs/features/parallelism.mdx swaps the row, states the boolean vocabulary once for the whole
table instead of per row, and states that pinning is not configurable, with the reason.

Measurement

Neither piece can move a number: the diagnostic is report-only and off by default, and the deleted
knob was never set by any harness script. No campaign ran, and none is presented here.

Gates, all on arm md5 76eacbae: ctest -L unit 228/228, -L serial 227/227, -L mpi 1/1, and
592 Python tests passed at each of four MPI layouts (2x1, 2x16, 4x8, 16x16 — worlds of 2, 32, 32 and
256). Counts are not comparable with the 222/221 an earlier revision quoted, nor with 225/224 from
before this branch merged main again: #259 added three cases.

Every new case asserts something, checked rather than assumed (--report_level=detailed):
summarize_masks 11, place_line_reports_every_field 1, place_line_unknown_is_not_a_verdict 1,
parse_flag_falsey_words_in_any_case 10, parse_flag_truthy_words 7,
parse_flag_compares_whole_words 9.

The COMMPLACE line is checked against ground truth rather than against a previous build, because
ground truth is the stronger check: under srun -n2 --cpu-bind=cores the two co-located ranks must
report masks=private with node_cpus = 2x cpus, and under --cpu-bind=none masks=shared with
node_cpus == cpus. Both arms give the same thread count and the same partition count, which is
the whole reason the line exists.

Measured, same binary as the gates:

--cpu-bind=cores  COMMPLACE rank=0 node_rank=0 node_size=2 masks=private cpus=64  node_cpus=128 cpu_list=0-127
                  COMMPLACE rank=1 node_rank=1 node_size=2 masks=private cpus=64  node_cpus=128 cpu_list=0-127
--cpu-bind=none   COMMPLACE rank=0 node_rank=0 node_size=2 masks=shared  cpus=128 node_cpus=128 cpu_list=0-127
                  COMMPLACE rank=1 node_rank=1 node_size=2 masks=shared  cpus=128 node_cpus=128 cpu_list=0-127

Note that cpu_list is identical in both arms — the job owns the whole node either way. masks and
cpus are what separate them, which is the case for reporting the peers' masks rather than sampling
/proc.

The parser change is likewise verified exhaustively rather than argued: of 18 representative values,
7 change behaviour — off/OFF/Off ON→off (the point), f/n/0abc/nope off→ON (bare
initials lost, prefix accidents tightened). disabled and offbeat are ON under both rules.

Relationship to the open PRs

No semantic interaction with any open PR. Textual overlap: #264 rewrites both test files here to
Catch2, and #265 touches PartitionGroup.h; zero overlapping files with #259 or #263. The refuted
placement deletion this is salvaged from is unrelated to the placement fix that merged as #249.

Checklist

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
I used the following tool to help write this PR description: Claude Code
I used the following tool to generate or modify code: Claude Code

Important

By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.

Warning

If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.

…nning knob --cpu-bind=none and --cpu-bind=cores give the same thread count and the same partition count, and differ only in the mask the kernel enforces -- a difference that has already cost this project a 1.45x tax it could not see. A rank holding 16 of a host's 128 CPUs is equally "Slurm gave me my own 16" and "eight of us share these 16", and sampling /proc cannot tell them apart: it describes the process it runs in, never a peer rank on the same host. PartitionGroup already exchanges the affinity masks to choose a placement, so monoprop_COMMPLACE reports what that exchange saw -- one line per rank on stderr, masks=private vs masks=shared being the distinction the whole thing exists for. Default off, and nothing below the flag is computed when it is off. The flag gates PRINTING only. The mask exchange is deliberately not gated on it: the environment is per-rank, so a predicate over it is not rank-uniform, and one rank skipping a collective its peers entered is a hang, not a missing diagnostic. emit_place_line takes the flag rather than reading it, so a test binary launched without it set can still reach the emitting path. monoprop_PARTITION_PINNING is deleted and pinning is unconditional. Its parser matched only the first CHARACTER against {0,f,F,n,N}; `o` is not in that set, so `off`, `OFF` and `disabled` all parsed as ON. No campaign ran on the wrong arm -- no harness script ever set it. parse_env_flag compares whole words, case-insensitively, against 0|false|no|off, which is the one thing the bug was about; bare `f` and `n` change from falsey to truthy as a consequence, and a test pins that. The knob can be deleted rather than fixed because letting the launcher own placement was refuted by its own falsifier: propagate[hubbard] at layout A/N=1 measured 2.90x slower, 10 of 10. Mutation-verified: restoring first-character parsing fails the whole-word case.

github-actions · 2026-08-21T10:16:26Z

Docs preview: https://pr-269.monoprop-docs.pages.dev

codecov · 2026-08-24T12:22:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.70%. Comparing base (5262335) to head (928d31c).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #269   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files          14       14           
  Lines         742      742           
  Branches       98       98           
=======================================
  Hits          725      725           
  Misses         12       12           
  Partials        5        5

Flag	Coverage Δ
cpp	`97.70% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Same COMMPLACE output, same four masks= verdicts, in 7 files +218/-51 instead of 9 files +464/-59, with comments back at house density. - summarize_masks replaces cpu_mask_popcount, cpu_mask_union and cpu_mask_ranges. hwloc_bitmap_weight / _or / _list_snprintf already do this in a TU that includes <hwloc.h>, so the hand-rolled run-length encoder, the public kMaxCpuRanges and the ",+N" run counter go. The truncation marker now fires on a 512-char line rather than a 32-run one, which prints MORE of a sparse mask, and cuts back to the last whole range so it never follows a half-written CPU id. - PlacementReport.h is deleted, with its struct and its PartitionGroup member. They existed only to carry state between two adjacent calls: the ctor emitted immediately after discover_node_peers_() filled the report, so report_placement_ now formats and writes where it classifies. That also removes the copy-ctor question of whether a clone re-emits -- it is reached only from the primary ctor. format_place_line lives beside summarize_masks in CpuTopology, so the PR adds no file and no CMakeLists entry. - One report_placement_ replaces fill_node_report_ and fill_own_report_; the `mine` argument was redundant, since row node_rank_ of the gathered array IS our own mask. - format_place_line returns the line instead of writing it to a FILE*, which is what makes the emitting path reachable in a binary launched without the knob set. The tests' tmpfile CaptureFile, count() and field() helpers go with it, and the checks are whole-line rather than seven field extractions. - parse_flag keeps its (value, default_value) signature; only the comparison changes to whole words. Narrowing it to parse_env_flag(value) rewrote four tests and cannot express a default-ON flag, which a sibling branch already has. - Corrected: `disabled` parses as ON under BOTH the old and the new rule, so it is not evidence of the fix. Only `off`/`OFF`/`Off` are, which is why the parser change is demoted to a consequence of adding a knob rather than presented as the repair of a bug that bit anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Co-authored-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com> Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>

robertodr

this looks good to me, it's just probing the hwloc masks to get a human-readable report for thread placement.

Changes:

The report is printed to stderr so there's no need to hide them behind a new env-var. The user can decide how to redirect the stream.
clang-tidy and SonarQube had some lints on CpuTopology.cpp. Some predate this PR, but we should fix them.

…CpuTopology lints Review asked for both. The COMMPLACE line goes to stderr, so the reader can redirect it: the `monoprop_COMMPLACE` knob bought nothing and is gone, and with it the last boolean env var, so `parse_flag`/`iequals` and their tests go too. CpuTopology.cpp lints, clang-tidy and SonarQube, pre-existing ones included: braced return, three `auto *const topo`, two misplaced-const hwloc handles, two more on the summarize_masks bitmaps, designated PhysicalCore init, a signed comparison, and cpp:S886 -- the row loop's stop condition read `ok`, which its own body writes; it now tests the counter and breaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

robertodr · 2026-08-25T14:09:14Z

I'll be very glad when we figure out how to document how to do this from outside, instead of programmatically (which we seem to patch up for every new machine that pops up) 😓

sonarqubecloud · 2026-08-25T14:16:44Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

🤖 _AI text below_ 🤖 ## Summary Follow-up to #269, which made the `COMMPLACE` line unconditional at review request. That change is correct and I am not walking it back — but making it unconditional exposed its volume, and the volume is a defect: **One serial 585-test Python run emits 437 `COMMPLACE` lines, and all 437 are the same string.** A process has one placement. Reporting it 437 times is noise, not data, and it is on `main` now. `fix/commplace-collapse-repeats` `f87b662`, 2 commits on `34db77d`, 4 files +42/−8. ### Why #269 did not catch this Because it measured with `pytest` capturing. #269's body states that "the whole Python suite emits nothing" — that reading came from a run without `-s`, and pytest's fd capture swallows C++ `stderr`. Same arm, same command, `-s` the only difference: | | `COMMPLACE` lines | | --- | --- | | `pytest … -q` | 0 | | `pytest … -q -s` | 1 | The zero was capture, not silence. Any future measurement of this line needs `-s`, and the probe script now says so in a comment rather than leaving the next person to rediscover it. ### The fix `place_line_is_new(line)` returns true the first time a line is seen and whenever it differs from the previous call; `PartitionGroup` writes only when it does. Three choices in it worth stating, because the obvious implementations are each wrong in a way: - **It compares the whole line, not a once-flag.** A `std::call_once` would report the *first* placement only. A process that builds an `alone` propagator before an MPI one has two placements and both are worth saying; a changed line still gets through. - **It is a non-template free function in `CpuTopology.cpp`, not a static local in `PartitionGroup<NumModes>`.** A static inside the template gets one guard per `NumModes` instantiation, so the collapse would leak one line per distinct mode count. - **It returns a bool rather than writing.** The write stays at the call site, and the collapse is testable without capturing `stderr` — which is exactly the instrument that hid the problem. The second commit takes the argument by `std::string_view` (SonarQube `cpp:S6009` on the first): the function only compares and assigns, so it never needed to bind a `std::string`. No knob, no behaviour change to what a *distinct* placement reports, and nothing branches on it. ## Measurement Arm md5 `f34e996d`. The counts are the point of the PR, so they are the measurement: | | before (`34db77d`) | after (`d7ff600`) | | --- | --- | --- | | Python suite, 585 tests, `-s` | 437 lines, 1 distinct | **1** | | C++ unit binary | 19 lines, 1 distinct | **1** | Gates on `f34e996d`: `ctest -L unit` 239/239, `-L serial` 238/238, job `COMPLETED 0:0`. The unit count is one above `main`'s 238 because of the new case. Python suite 585 passed, 8 deselected. The `string_view` commit is a signature change only; its case still asserts 6 of 6 and the counts above re-measured identically. CI: all 16 checks green, clang-tidy 0 unsuppressed warnings, SonarQube 0 open issues. The new case asserts 6 of 6 (`--report_level=detailed`) and covers the two failure modes separately: a repeat is suppressed, and a *changed* line is not — the second is what a `call_once` implementation would fail. Ground-truth multi-rank behaviour is unchanged and re-verified with no env var set; the collapse is per process, so co-located ranks still each report: ``` --cpu-bind=cores COMMPLACE rank=0 node_rank=0 node_size=2 masks=private cpus=64 node_cpus=128 cpu_list=0-127 COMMPLACE rank=1 node_rank=1 node_size=2 masks=private cpus=64 node_cpus=128 cpu_list=0-127 --cpu-bind=none COMMPLACE rank=0 node_rank=0 node_size=2 masks=shared cpus=128 node_cpus=128 cpu_list=0-127 COMMPLACE rank=1 node_rank=1 node_size=2 masks=shared cpus=128 node_cpus=128 cpu_list=0-127 ``` These runs are not reproducible from the diff: they need the Deucalion harness, which is not in this repo. ## Checklist - [x] Tests added or updated to cover the changes - [ ] Documentation updated (docstrings, `docs/`, `CONTRIBUTING.md`) if needed - [ ] `CHANGELOG` / release notes updated if applicable Docs unchanged on purpose: `parallelism.mdx` already says the line is written per rank at propagator construction with no knob, and that stays true. ## 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 > [!IMPORTANT] > By opening this PR I confirm that I have read [CONTRIBUTING.md](https://github.com/Algorithmiq/monoprop/blob/fa820c3e5a90f773417c367fedbf2bd50683b496/CONTRIBUTING.md) and I agree to the terms of the [Contributor License Agreement](https://github.com/Algorithmiq/monoprop/blob/fa820c3e5a90f773417c367fedbf2bd50683b496/CLA.md). > [!WARNING] > If you're contributing on behalf of your employer, contact [cla@algorithmiq.fi](mailto:cla@algorithmiq.fi) to arrange a Corporate CLA. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Ramón L. Panadés-Barrueta <rpana92@gmail.com>

github-actions Bot added documentation Improvements or additions to documentation cpp labels Aug 21, 2026

Merge branch 'main' into feat/commplace-placement-report

d601f37

diagonal-hamiltonian marked this pull request as ready for review August 24, 2026 12:14

diagonal-hamiltonian requested review from fpietra, ludmilaasb and robertodr as code owners August 24, 2026 12:14

diagonal-hamiltonian and others added 3 commits August 24, 2026 15:04

Merge branch 'main' into feat/commplace-placement-report

7e376be

Merge branch 'main' into feat/commplace-placement-report

ce279dd

robertodr reviewed Aug 25, 2026

View reviewed changes

Comment thread docs/content/docs/features/parallelism.mdx Outdated

Apply suggestions from code review

82c7496

Co-authored-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com> Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>

robertodr reviewed Aug 25, 2026

View reviewed changes

Comment thread docs/content/docs/features/parallelism.mdx Outdated

robertodr requested changes Aug 25, 2026

View reviewed changes

robertodr and others added 2 commits August 25, 2026 15:34

Merge branch 'main' into feat/commplace-placement-report

b6b1a7c

robertodr approved these changes Aug 25, 2026

View reviewed changes

robertodr enabled auto-merge (squash) August 25, 2026 14:08

robertodr merged commit 34db77d into main Aug 25, 2026
20 checks passed

robertodr deleted the feat/commplace-placement-report branch August 25, 2026 14:10

diagonal-hamiltonian mentioned this pull request Aug 25, 2026

fix(partition): 🔇 collapse identical COMMPLACE repeats #286

Merged

6 tasks

Navigation Menu

Uh oh!

feat(partition): 📈 report the launcher's placement, and delete the pinning knob - #269

feat(partition): 📈 report the launcher's placement, and delete the pinning knob#269
robertodr merged 8 commits into
mainfrom
feat/commplace-placement-report

diagonal-hamiltonian commented Aug 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 21, 2026

Uh oh!

codecov Bot commented Aug 24, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

robertodr left a comment

Uh oh!

robertodr commented Aug 25, 2026

Uh oh!

Uh oh!

sonarqubecloud Bot commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

diagonal-hamiltonian commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Measurement

Relationship to the open PRs

Checklist

AI/LLM disclosure

Uh oh!

github-actions Bot commented Aug 21, 2026

Uh oh!

codecov Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

robertodr left a comment

Choose a reason for hiding this comment

Uh oh!

robertodr commented Aug 25, 2026

Uh oh!

Uh oh!

sonarqubecloud Bot commented Aug 25, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

diagonal-hamiltonian commented Aug 21, 2026 •

edited

Loading

codecov Bot commented Aug 24, 2026 •

edited

Loading