feat(platform): record, check and publish the compute-partition shape - #1288
Conversation
CI E2E report — ❌ Timeout
|
|
The measurement work is real and the docstrings carry hard-won operational The repo has a policy for this class of knob, and it is the oppositeCompute partitioning is a host-global, privileged, throughput-affecting knob.
That is why Suggestion
Why this is better and not merely smaller:
Note also that Please split
|
ff451d8 to
0e70ae9
Compare
|
Thanks for this review — it was detailed and nearly every finding held up when I On the All of it is gone: What I kept. The validation, which is where the value actually was. A The rest of the findings. The Two findings dissolved rather than got fixed: Your Two bugs I found while rewriting, worth flagging since neither was in the review: I've force-pushed the rewrite onto this branch rather than opening a new PR, so |
bd80f04 to
647cfc8
Compare
|
/retest |
|
The read-only direction is right, and the objections from the first round are Must fix1. A serving session is refused on an already-partitioned card, with no flags passed.
The 2. On a fresh launch the report says the CU count was "derived from the board table" when it was probed from the device.
Report output, same session, two paths:
Since Should fix3. 4. Validation runs before quantization and before GPU-type resolution. (For the record: 5. "Let the benchmark entrypoint set it" is not achievable. Both Minor6. Multi-node silently records the controller's shape. The 7. 8. Dead API. Not blockingResume can cross shapes when the operator passes a mode that differs from the |
647cfc8 to
ff6ff66
Compare
|
Both must-fix items were real and I reproduced both before touching anything. Must fix1. A serving session refused on an already-partitioned card. Reproduced My first fix was to run the check at one stream instead of two, and that is It stays a refusal where the premise holds — a scriptable framework, or an The gate lives in the CLI rather than in 2. False "derived from the board table" on a fresh launch. Fixed as you I also fixed the rendering, because the seed is not the only reader of the lossy There were no tests on Should fix3. 4. Validation ran before quantization and GPU-type resolution. Fixed by This also removed a second bug: the call was unconditional at the top of One consequence to flag: the refusal now lands after 5. "Let the benchmark entrypoint set it" is not achievable. Agreed and Minor6. Multi-node recorded the controller's shape. Took the stronger of your two 7. 8. Dead API. On The report says plainly which case it is, and no longer prints a Not blockingAgreed on resume crossing shapes, and agreed it is a follow-up. The variant you Tests
|
|
/retest |
|
All eight findings from the last round are verified fixed, including the two I Worth fixing1.
but 2. Cleanup3. 4. 5. The unknown-capacity branch in 6. |
|
All six are fixed. One of them turned up a seventh while I was in the file, noted Worth fixing1. Two details worth stating. The refusal happens before the card is read, because 2. It still returns 0 rather than raising, since a malformed environment variable Cleanup3. 4. I kept
5. The unknown-capacity branch. The two guards now share one predicate, I did keep the guard inside 6. One more, same familyChasing finding 6 I found the claim you retired last round still alive in two Tests and docs
Docs followed the two behaviour changes rather than only the code: the This is |
An MI300-series card can be split into independent partitions (SPX, DPX, QPX, CPX), and splitting one trades per-request latency for aggregate throughput. Nothing recorded which shape a number came from, so the same configuration measured on the same card in SPX and in CPX was indistinguishable in the history: two different experiments filed under one name. The observed mode now joins NPS in the platform fingerprint, the session report names it on partitioned runs, and the shape is published for the benchmark entrypoint that places work across partitions. The optimizer does not change the mode. Setting it is privileged, evicts every process holding a context on the card, and renumbers its devices -- not something an optimization loop should do between benchmark rounds, and not something to hand agent-authored code. The card must be in its mode before optimize starts: the shape is checked and recorded at launch, so a mode applied later is too late to be either. Every probe added here is an unprivileged read, and a host without amd-smi behaves exactly as before. That leaves two things worth doing at the boundary, both at launch. --compute-partition-mode asserts the mode the card is already in and refuses the session when it is in another, or when the card cannot be read at all: the flag exists to catch an external set that did not take, so an unverifiable assertion is not a satisfied one. And the per-stream footprint is checked against one partition's memory, sized from the checkpoint's weight bytes -- a lower bound, since each stream holds its own copy of the weights, which is why a "does not fit" verdict from it is a proof and a "fits" verdict is no evidence. The arithmetic costs milliseconds and replaces an out-of-memory crash three hours in. When the checkpoint cannot be sized the session runs and says so. The footprint refusal applies only where streams will actually share a partition. Without a fan-out nothing places a second stream, and nothing pins the benchmark to a partition at all -- whole cards enumerate before partitions, so on a node with one card of eight split, device 0 is a whole card. Refusing a serving session that merely started on a card someone else left split would be arithmetic about a shape it was never going to run in, so the mode is recorded there and nothing is refused. For the same reason the published env is split by reader: mode, count and CU describe the card and are always published, since the platform fingerprint reads them back on the crash path, while streams and total streams are directions to a benchmark that fans out and are published only when one will. CU per partition is read from the device rather than divided out of a board table, because partition devices are selected by matching that count exactly: an index list computed at launch would be wrong in the one case that matters and wrong invisibly. The table remains a fallback and the recorded shape says which of the two it came from, so a derived count is never presented as a measurement -- and an unknown provenance is reported as unknown rather than as the table. Multi-node sessions record no shape. The card this process can read is not the card the benchmark runs on, and a shape recorded from the wrong node is the mislabelling this exists to prevent. Co-authored-by: Cursor <cursoragent@cursor.com>
…ion GPU id Second review round on the compute-partition shape check. Still read-only, still no mutation anywhere. Two changed behaviours. validate_session_shape used `streams or DEFAULT`, so 0 silently became 2 and reported success for the same value the CLI exits 2 on; it now refuses, and refuses before the card is read, since a bad request needs no probe to judge. partition_gpu_id swallowed an unparseable HYPERLOOM_PARTITION_GPU and filed card 0's topology as the session's in silence; it now warns and names the consequence. The unknown-capacity guards in validate_session_shape and fits_in_partition asked one question with two different tests, so a zero capacity skipped the arithmetic *and* skipped the warning that explains why. Both now share PartitionLayout.capacity_known. Cleanup in the same pass: drops the dead UNPARTITIONED_MODE, takes the four probe helpers behind observe_partition out of __all__ so it describes the interface rather than the call graph, requires a layout in session_shape_summary instead of answering None with a second schema whose absent provenance key read as a positive claim, and retires the last two "measured peak, preferred when present" claims for a field nothing in this repository writes. Co-authored-by: Cursor <cursoragent@cursor.com>
0986b14 to
3044f81
Compare
This PR has been rewritten since its first review, so the diff no longer resembles what was reviewed. Same hardware and the same measured wins, but the mode is now a fixed session property established outside the optimizer rather than a lever searched inside it. No
sudo, noamd-smi set, no mutation of any kind in the optimizer. The previous revision is preserved atff451d840if it is useful for comparison.It has since been updated for the second review round; the finding-by-finding resolutions are in the comments below, and the description that follows reflects the current behaviour.
--max-latency-msis no longer part of this PR. It was orthogonal — the constraint applies to any throughput-for-latency trade, not just partitioning — so it moved to #1297 to be reviewed on its own.Why the design changed
The first revision put
amd-smi set compute-partitioninside the optimization loop. Three objections from review, all fair:preflight.pyreads NPS and cpufreq and warns; it does not change them.What survives is the part that was actually load-bearing: validation. A launch-time refusal saves a three-hour session, and the
fits_in_partition/read_hbm_gibarithmetic that produces it costs milliseconds.What this PR does
Three things, all read-only:
Records the shape. The observed mode joins NPS in the platform fingerprint. Without it, the same configuration measured on the same card in SPX and in CPX is indistinguishable in the history — two different experiments filed under one name. The session report names it on partitioned runs, stating that the numbers are not comparable with a whole-card run. Where the framework's benchmark cannot place work per partition, the report says so outright: the figure is one device's, and which device is not knowable from here. Where it can, the report says the aggregate depends on a fan-out this process cannot verify.
Checks it at launch. Fail-closed, on the things it can be wrong about:
--compute-partition-mode SPX|DPX|QPX|CPXasserts the mode the card is already in. If the card is in another mode, the session is refused. If the card cannot be read at all, that is also a refusal — the flag exists precisely to catch an external set that did not take, so an unverifiable assertion is not a satisfied one. The help text says "assertion, not request" in as many words.--streams-per-partition(default 2) multiplied in. The footprint is the checkpoint's weight bytes, read byte-exact from the safetensors index. That is a lower bound and is used as one: each stream holds its own copy of the weights, so a "does not fit" verdict from it is a proof, while a "fits" verdict is no evidence — exactly the asymmetry a refusal needs, since it only ever acts on the former. When the checkpoint cannot be sized, the session runs and says so.There is a second source in the code, a measured
peak_gib_per_stream, and it would be the tighter one — but nothing in this repository writes it, so every refusal today is made on the weights bound. The reader is kept so a harness that starts reporting it is honoured without a change here, and it is labelled as having no in-tree producer rather than presented as a fallback that gets exercised.--nodes >= 2session records no shape. The card this process can read is not the card the benchmark runs on, so a declared mode there cannot be checked and is a usage error rather than a silently unchecked assertion.Publishes it. The env block is split by reader. Mode, partition count and CU per partition describe the card and are published for any single-node session on a readable one, because
platform_fingerprint()reads them back from there — it runs on the crash path, where spawningamd-smiis not acceptable. Streams per partition and total streams are instructions to a benchmark that places work on each partition, so they are published only when one will. The entrypoint gets the shape, not a device list, on purpose: HIP enumerates whole cards before partitions, so an index list computed at launch would be wrong in the one case that matters and wrong invisibly. The process holding the GPU context is the one positioned to check a device's CU count and refuse what does not match.The consumer is out of tree
Worth stating outright, since the diff does not show it: nothing in this repository reads the variables this PR publishes. The program that places work on each partition is the benchmark entrypoint, and it lives outside the repo —
assets/benchmark_scripts/is not in the tree, as you noted. The env block is a contract offered to a consumer that lives elsewhere.partition_device_predicate()is in the same position: defined, documented and tested, but within this PR called only by its own test. It is the reference implementation of the rule the external script is most likely to get wrong — select partition devices by matching CU count, never by index, because HIP enumerates whole cards first, so under DPX on one card of eightdevice 0is a full 256-CU GPU while the partitions are devices 7 and 8. Keeping it here puts that rule next to the documentation describing it instead of leaving it to be re-derived downstream. Its first real caller now exists: the sweep driver in #1299 uses it to select partition devices, so the rule and its consumer land together across the two PRs.Two limits that follow, both worth being explicit about:
optimizesession on a split card measures one partition, not the aggregate. The recorded shape is still correct and still worth having — it is what stops a CPX number being filed as though it were SPX — but the throughput is not yet the aggregate figure the mode is chosen for. The report now states which of the two cases applies rather than leaving it to be inferred.scripts/operator tool for the same reason the mutation left this PR: it sets the mode, and that belongs outside the optimizer.Review findings from the first round
set_partition_mode,partitioned(), the drain/retry loop, the restore path, and the sudo env are all deleted. The card must be in its mode beforeoptimizestarts: the shape is checked and recorded at launch, so a mode applied later — by the benchmark entrypoint, for instance — is too late to be either.--max-latency-mshonoured only by ExploreExecutor while the help text claimed otherwise_lift_to_current_bestso it covers every promotion path.--frameworkshort-circuit (if framework and not ...silently passes when None)_partition_fanout_supported(), which returns three explicit cases: supported, known-unsupported, and not-yet-resolved. All three are surfaced to the operator, and both call sites now pass a resolved framework.PartitionLayout.probedrecords which source was used, so a derived count is never presented as a measurement — and is warned about, since partition devices are selected by matching that count exactly.read_hbm_gibfailurepeak_gib_per_streamdropped after the first KEEPRUNTIME_MODE_ENV/PARTITION_MODE_ENVPARTITION_*constants, now incommon/gpu_partition.pywhere the fingerprint can reach them without importing an executor.observe_partition()call per launch.partition_*variants appear unprompted. Left the prompt untouched deliberately — platform topology (NPS included) is a recording concern in this repo and reaches no agent prompt.docs/reference/environment-variables.mdgains a "Compute partitioning (AMD)" section covering both the operator inputs and the published runtime hand-off the entrypoint reads;CHANGELOG.mdgains an entry under [Unreleased]. The_SUDOvariable it asked about no longer exists.Bugs found while writing the tests
_export_partition_shapenever passed the model path to the footprint resolver, so the headline feasibility check could only ever warn — the refusal was unreachable. Now wired on both the fresh-launch and resume paths, with a test that asserts the resolver actually receives it.--streams-per-partition 0was read as "not passed" (0 or DEFAULTisDEFAULT) and silently became 2, leaving the>= 1guard unreachable for the value most likely to be a mistake. Same trap in the resume restore path. Both now test againstNone._run_optimizeand again in the resume branch, so a resume validated twice — the first time against an unresolved model, where it could only warn. There is now exactly one call per path, placed after the framework, GPU type and post-quantization model path are all resolved.Resume
The shape is part of the measurement contract, so it restores on the same path as the other operator-supplied values, and is then re-checked against the live card rather than trusted. A card can be repartitioned while a session is stopped; resuming into a different topology would compare candidates measured under one shape against a baseline from another.
One case is a recording rather than a refusal: an archive with no recorded mode — the card was unreadable at the first launch — has no assertion to re-check, so a resume onto a partitioned card observes the new shape and files it rather than stopping. The fingerprint then differs between the two halves of the session, which is the honest outcome, but nothing refuses.
Tests
140 new tests in two new files (
test_gpu_partition.py, 50;test_partition_shape.py, 90), plus two intest_cli_bootstrap.py. They cover theamd-smipayload shapes actually in the wild, the MI355X case that motivated this (20.7 GiB × 2 streams does not fit a 36 GiB CPX partition), the refusal semantics, the fan-out gate, the multi-node behaviour, resume restore, the CLI exit codes, and the report section's provenance line — which had no coverage at all before, and is where the second round's false "derived from the board table" bug was hiding.Nothing here needs privilege, and a host without
amd-smibehaves exactly as before: unreadable card plus no declaration is the ordinary case and is not an error.