Feat/rpoornac/partition mode sweep - #1299
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
CI E2E report — ✅ Succeeded
|
|
/retest |
647cfc8 to
ff6ff66
Compare
5371e66 to
4041b23
Compare
0986b14 to
3044f81
Compare
Choosing a partition mode was left to judgement. The optimizer treats the mode as a fixed session property and asserts it, which is right -- a card-wide privileged mutation does not belong in a loop that also runs agent-authored code -- but it leaves the question of which shape a workload actually wants unanswered, and answering it by hand means four sessions and a spreadsheet. This is the other half of that boundary. It sets each mode on one card in turn, runs the same benchmark on every partition the mode creates, sums the throughput, and restores the card's entry mode on the way out -- after a failure or a Ctrl-C included. The optimizer still never sets a mode; the privileged amd-smi set lives here and nowhere else, in a script an operator ran on purpose, between benchmarks rather than inside one. The fan-out is the substance rather than a detail. A benchmark that loads one partition and ignores the other seven measures an eighth of the card, so a sweep without it would report CPX as catastrophically slow while looking like a partitioning comparison -- the misleading number that kept this out of the previous branch. Every partition is launched before any is waited on, and a mode is reported only when all of its partitions came back: six of eight summed understates by a quarter while still looking like a result. Validated on an 8-card MI355X node with a synthetic workload whose throughput scales with CU, where a correct fan-out is visible as a constant aggregate across modes -- SPX 1x2560, DPX 2x1280, QPX 4x640 -- and a broken one would show DPX at half. Partitions are selected by matching CU count within the swept card's PCI bus, never by device index, and the reason is now measured rather than argued: on that node with card 0 in CPX, amd-smi orders by PCI address and calls the partitions devices 0-7, while HSA/HIP enumerates whole cards first and calls them 7-14. Under DPX the partitions are HIP devices 7 and 8, exactly as partition_device_predicate's docstring predicted, which also gives that helper its first caller. The bus half of the match matters separately: SPX's "partition" is a whole card, so CU alone would collect all eight of them. Reuses the launch-time feasibility check to skip modes whose partitions provably cannot hold the configured streams, showing the arithmetic instead of running into an out-of-memory failure -- on MI355X that correctly refuses CPX for a 20.7 GiB stream at two per partition, since 36 GiB does not hold 41.4. Three traps found on the hardware and encoded in the tests: rocminfo puts BDFID before Compute Unit in each agent block, so a line-at-a-time parser attributes every agent the previous one's CU count and silently turns the first partition into a whole card; amd-smi reports an idle card's process_list as the string "No running processes detected" rather than an empty list, so a naive length check finds a process on every free node and refuses to start; and an inherited HIP_VISIBLE_DEVICES alongside the ROCR mask applies as a second mask indexing into the first, quietly redirecting every partition's work onto one device. 88 tests, none needing a GPU. Co-authored-by: Cursor <cursoragent@cursor.com>
CodeQL flagged the NaN guard in the sweep's local float coercion (alert 2628, "comparison of identical values"): out == out is the classic idiom but it reads as a tautology, and the scanner is right that math.isnan states the intent. The better fix is to delete the helper rather than repair it. hyperloom.common.coerce.to_float already is this function -- rejecting bool, rejecting non-finite, stripping strings, falling back to a default -- and its docstring calls itself the single home for the idiom, with zero first-party imports precisely so anything may depend on it. The local copy was a reimplementation of a documented primitive that happened to also carry a lint finding. Behaviour is unchanged except that a numeric string with surrounding whitespace now parses, which the tests already asserted should work. Co-authored-by: Cursor <cursoragent@cursor.com>
4041b23 to
56d0b43
Compare
|
Rebased onto current #1288 was squash-merged, so the commit this branch was stacked on 88 sweep tests pass against the merged tree, The two earlier CodeQL notes on |
|
Re-reviewed the current head (
Would like these addressed before merge. |
Three fail-open paths around the one destructive operation in this script,
all found in review.
The busy check that stands in front of amd-smi set read any payload it did
not recognise as an idle node. rows = payload if isinstance(payload, list)
else [] answered {} for a wrapped or renamed process listing, and per-row
key mismatches were skipped, so the sweep proceeded to evict whatever was
running. Measured against the old parser: a dict-wrapped payload gave {},
and a process entry without the process_info wrapper gave zero live
processes on a card that had one. Both now raise SweepError, which is exit
2 before anything is set. The same strictness catches the inverse, which
was also live: a process_list arriving as the bare idle string was iterated
character by character and counted as 29 processes, refusing the sweep on a
free node -- the exact failure the sentinel constant exists to prevent,
reintroduced one level up.
The check was also node-wide while the set only ever touches --gpu. Any
busy card on a shared node blocked sweeping an idle target, and the only
escape, --allow-busy, gave up the protection on the target card as well.
It is now scoped to the swept card, and a target absent from the listing is
a refusal rather than an assumed-idle card. --allow-busy and --dry-run skip
the check entirely, since neither has a set for it to guard -- which also
makes the documented "--dry-run touches nothing" true when the node is busy.
An exception that was not a SweepError or a PartitionError escaped the
per-mode loop, past the finally, and out of main(): the card was restored,
but the table and sweep_summary.json never ran, the modes already measured
were lost, and a restore that failed on the way out exited 1 instead of the
documented 3. Verified with a KeyError from set_mode. Unexpected exceptions
now stop the sweep by breaking rather than propagating, so the restore and
the report both still happen, and they exit 4 -- a new code, because
reporting a crashed sweep as 0 because some mode measured would be a lie,
and 1 already means "ran, measured nothing". The restore is a helper that
raises nothing, attempts the set even when the read-back that would have
proved it necessary is what broke, and 3 outranks 4: a card left in the
wrong shape mislabels whatever runs on the node next.
37 tests added, 125 total, including the first that drive main() end to end
against a fake node -- the control flow above cannot be reached any other
way. 29 of the new tests fail against the previous revision.
Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Base this PR on
feat/rpoornac/compute-partition-lever(#1288), notmain. Itimports
hyperloom.common.gpu_partition, which #1288 adds. Reviewing it againstmainwill show that module as missing.What this is
#1288 made the partition mode a fixed session property: the optimizer reads it,
records it, refuses a session that cannot work in that shape, and never sets it.
That is the right split, but it leaves the obvious question unanswered — which
shape does this workload want? — and answering it by hand is four sessions and a
spreadsheet.
This is the other half of that boundary. It sets each mode on one card in turn,
runs the same benchmark on every partition the mode creates, sums the throughput,
prints the comparison, and restores the card's entry mode on the way out. The
privileged
amd-smi setlives here and nowhere else: between benchmarks, in ascript an operator ran deliberately, rather than inside a loop that also runs
agent-authored code.
optimizestill never changes a mode.The fan-out is the substance, not a detail
A benchmark that loads one partition and ignores the other seven measures an
eighth of the card. A sweep built that way would report
CPXas catastrophicallyslow while looking exactly like a partitioning comparison — which is the
misleading number #1288 promised not to produce, and the reason it shipped
without a sweep. So every partition is launched before any is waited on, and a
mode is reported only when all of its partitions came back. Six of eight summed
understates by a quarter while still looking like a result, so that is
"unmeasured", not "slow".
The table above is the validation. The synthetic workload's throughput scales
with CU count, so a correct fan-out shows a constant aggregate across modes —
SPX 1×2560, DPX 2×1280, QPX 4×640 — while a broken one would show DPX at half
and QPX at a quarter. Run on an 8-card MI355X node. The latency column moving
1000 → 2000 → 4000 ms is the cost being paid for it.
The enumeration rule, now measured rather than argued
#1288 justified publishing the shape instead of a device list by asserting that
HIP enumerates whole cards before partitions. That is now verified on hardware,
and the two tools genuinely disagree. With card 0 of 8 in
CPX:amd-smi(orders by PCI address)rocminfo)So a device list computed with one tool and handed to the other is wrong, and
wrong invisibly — the benchmark runs to completion, on the wrong silicon. Under
DPX the partitions are HIP devices 7 and 8, exactly as
partition_device_predicate's docstring predicted.That also settles the loose end #1288 disclosed:
partition_device_predicatehadno caller in the tree. It has one now, and this script is the fan-out consumer
the published env contract was written for. It sets the same
HYPERLOOM_PARTITION_*variables a session sets, so an entrypoint writtenagainst that contract behaves identically under either.
Selection matches CU count within the swept card's PCI bus. Both halves
matter: the CU match distinguishes a partition from a whole card, and the bus
match is what stops
SPX— whose "partition" is a whole card — from collectingall eight cards on the node.
Refusals instead of failures
The launch-time feasibility check from #1288 is reused per mode, so a mode whose
partitions provably cannot hold the configured streams is skipped with the
arithmetic shown rather than run into an out-of-memory failure. On MI355X that
correctly refuses
CPXfor a 20.7 GiB stream at two per partition: 36 GiB doesnot hold 41.4.
The card is restored on every exit path, including a failure mid-sweep and a
Ctrl-C, and a failure to restore is its own exit code (
3) rather than a line inthe log — a card left in a shape nobody asked for silently mislabels whatever
runs next. The sweep also refuses to start when processes hold GPU contexts,
since repartitioning evicts them;
--allow-busyis there for when they areyours.
Three traps found on the hardware
All three were real failures during bring-up, and each is now a named test.
rocminfoputsBDFIDbeforeCompute Unitin each agent block, so aline-at-a-time parser that emits on
BDFIDattributes every agent theprevious one's CU count. It turned the first CPX partition into a 256-CU card
and would have pointed the benchmark at whole silicon while labelling it CPX.
amd-smireports an idle card'sprocess_listas the string"No running processes detected", not an empty list, so a length check findsone process on every idle card and the sweep refuses to start on a free node.
HIP_VISIBLE_DEVICESalongside the ROCR mask applies as asecond mask indexing into the first, quietly redirecting every partition's
work onto one device — every process succeeds and the mode reads as uniformly
slow. Both it and
CUDA_VISIBLE_DEVICESare removed rather than set.Scope and shape
scripts/partition_mode_sweep.py, plusscripts/tests/— 88 tests, noneneeding a GPU. Follows the
platform_audit.pyconventions: argparse,Usage::docstring,
raise SystemExit(main()), tests loaded viaimportlib.profile query) is recovered from
ff451d840, the revision of feat(platform): record, check and publish the compute-partition shape #1288 that had itbefore the mutation surface was removed from the optimizer. Its principle
survives intact: a set is not a set until it reads back.
amd-smi setreports success for a staged change and exits zero on some permission
failures, so trusting the exit code means benchmarking the old topology under
the new mode's name.
--dry-runprints the plan and touches nothing.--benchmark-config(the in-tree Magpie invocation) or--benchmark-commandwith{device}/{output_dir}placeholders, substitutedper already-split token so no shell is involved and a path with a space stays
one argument.
What it does not do
It chooses a mode; it does not tune one. The intended sequence is: sweep to pick
the shape, then run
optimize --compute-partition-mode <winner>so the sessionrefuses to start if the card is not actually in it. The report says so in as many
words, because a throughput table is easy to mistake for a tuning result.