feat(partition): 🔊 make a refused partition placement queryable and audible - #294
feat(partition): 🔊 make a refused partition placement queryable and audible#294diagonal-hamiltonian wants to merge 2 commits into
Conversation
…udible partition_cpusets() returns an empty placement order when the partitions requested exceed the visible physical cores, and every partition thread then runs unpinned -- 16.4x on propagate at 192 partitions over 128 cores, 24.6x at 256. The only sign was a std::print to C++ stderr, which pytest's file-descriptor capture swallows without -s, and nothing in-process could be asked whether pinning had happened, so a harness could not fail closed on it. Record the outcome rather than only printing it. PlacementReport -- pinned, cores visible, groups, partitions, and a monotonic decision counter -- is written by every partition_cpusets() call and read back through _core.placement_report(), re-exported as monoprop.placement_report(). The message text moves into format_unpinned_line() so the stderr line and the new RuntimeWarning cannot drift apart; the stderr line itself stays, being the only channel that works with no Python in the process. The warning is raised from _init_simulator, not from the engine: PyErr_WarnEx needs the GIL, which the partition masters never hold, and raising it from the binding's __init__ would strand a constructed PartitionGroup with live threads when a caller's filter turns the warning into an error. The decision counter is what keeps a build that placed nothing from re-announcing an earlier refusal. Pinning behaviour is unchanged: nothing refuses, clamps, or falls back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Docs preview: https://pr-294.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
=======================================
Coverage 97.70% 97.71%
=======================================
Files 14 14
Lines 742 744 +2
Branches 98 98
=======================================
+ Hits 725 727 +2
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Decision on the This repo sets
The alternative considered was exempting it from the error filter so it warns without raising. Rejected because a suite running Happy to flip it if you'd rather — it is a one-line filter entry, no code change. |
|
@robertodr is this a good idea? I want users to feel pain when they use threading incorrectly. |
Removed commented sections for clarity and conciseness. Signed-off-by: Aaron Miller <61472721+diagonal-hamiltonian@users.noreply.github.com>
|
1 participant
Summary of the slop below, basically ive been having some issues with the threading etc and oversubscription of the partitions. This attacks this. Maybe we should have some sort of logging for the partiontions and their placement to avoid this if possible?
AI text below
When
monoprop_PARTITIONSexceeds the visible physical cores,placement_order()returns empty,partition_cpusets()refuses, and every thread runs unpinned. Measured on Deucalion x86 (128 physical cores), against an in-run pinned control on the identical workload:Both collapsing cells have an exactly integer partitions-per-thread ratio, so this is not about divisibility — the rule is
partitions > visible cores, full stop. Cost grows with the oversubscription factor rather than being a fixed penalty, and the unpinned cells are also unstable across reps (2.46× spread, against 1.01× when pinned).The problem is not that it degrades. It is that it degrades silently.
partition_cpusets()already prints a warning, but it goes to C++ stderr, which pytest's file-descriptor capture swallows without-s— which is how the benchmarks and most harnesses run. And there was no way to ask, from Python, whether pinning actually happened, so nothing could assert on it.This PR adds channels. It does not change pinning behaviour — no refusal, no clamping, no fallback placement. That is a separate decision.
What it adds
The engine records rather than only printing.
PlacementReport { pinned, cores_visible, groups, partitions, decisions }, written by everypartition_cpusets()call into a mutex-guarded process-wide record and read back viaplacement_report().decisionsis a monotonic counter, which is what lets a caller distinguish a fresh verdict from a stale one. The existing stderr write stays — it is the only channel that works with no Python in the process — and both channels now render one string viaformat_unpinned_line()so they cannot drift.groupsrecords the count the placement used — 1 once aNodeMask::PerRankmask collapses it, not the launcher'snode_size— since that is what explains the outcome. A test pins both directions.The binding exposes it.
_core.placement_report()returns a dict;monoprop.placement_reportis the public re-export so a harness has a surface to fail closed on._core.warn_if_unpinned(stack_level=…)raises theRuntimeWarning.The warning is raised from Python, in
MonomialPropagator._init_simulatorright after the dispatch call — not from the engine, not from the binding's__init__.PyErr_WarnExneeds the GIL and the partition masters never hold it, but the constructing thread does, so that alone would have allowed__init__. The blocker is this repo's ownfilterwarnings = ["error"]: under it the warning is an exception, and nanobind does not mark an instance ready when__init__throws, so the destructor never runs — stranding a fully-constructedPartitionGroupwith S live master threads. Raising after the object is bound to a Python name makes the error path plain refcounting.The cost of that choice: a construction that goes straight through
_corebypasses the warning. The report still records it.Because of
filterwarnings = ["error"], in any environment where placement legitimately fails, every propagator construction under pytest now raises rather than warns. That is fail-closed and arguably the point — a silent 25× is worse — but it is a real behaviour change for anyone running the suite on an oddly-confined box, and it is worth an explicit yes or no rather than arriving as a side effect. All four measured layouts stay pinned, so nothing in the current harness trips it. Say the word and I will downgrade it to a plain warning with the filter exempted.Verification
Built and run on
dev-x86compute nodes:ctest -L unitctest -L serialctest -L mpipytest testsThe layout sweep was run specifically because of the
filterwarningsinteraction — it is the check that the new warning does not fire spuriously under realsrun --cpu-bind=coresmasks and turn the suite red. It doesn't, andtest_oversubscription_warns_and_reports_unpinnedpassed on all 24 rank-runs.The unpinned branch is confirmed reached, not assumed. A probe printed the record at four states: unset → confined to 1 CPU with
partitions=2(warning raised,pinned=False,cores_visible=1,decisions=1) →partitions=1, no placement decided (decisionsunchanged, no re-announcement) → unconfined (pinned=True,cores_visible=128,decisions=2). The C++ stderr line appeared alongside, so both channels fire on the same event.No test skips. The affinity helper falls back to
cpu_count() + 1partitions wheresched_setaffinityis absent or refused, and both arms assert identically. The C++ case asserts the returned set is empty before reading the record, so a refusal that silently placed fails.cpu_topology_placement_report_records_a_placementassertspinned == !cores.empty()— a host with no hwloc topology is a verdict the record must state, not a reason to return early.🤖 Generated with Claude Code