Repository navigation
Migrate from the deprecated rocm-smi to amd-smi - #1924
Draft
marcusr-amd wants to merge 7 commits into
Draft
marcusr-amd wants to merge 7 commits into
marcusr-amd wants to merge 7 commits into
Conversation
…mi scrapers
rocm-smi is deprecated from ROCm 7.0 and is no longer built as part of the
ROCm 10.1 standard build, so a stack Hyperloom supports can ship amd-smi and
nothing else -- the ROCm 10 pip/TheRock wheels already do. There
detect_gpu_count() returned 0, because `except FileNotFoundError: return 0`
swallowed the absence; Ray then derived 0 GPUs and a run hung on an actor that
could never be scheduled.
Adds hyperloom/common/amd_smi.py, modelled on the _amd_smi_json helper
gpu_partition.py already carried, and points the three byte-identical `GPU[`
scrapers and the VRAM reader at it. There is deliberately no rocm-smi
fallback: every ROCm in the support matrix ships amd-smi, so a second parser
would serve no supported configuration.
The real compatibility burden is that 7.2.x and 10.0 are supported
simultaneously, i.e. amd-smi 26.2.2 and 27.1.0 at once. A field may be a bare
number on one and {value, unit} on the other, and an unsupported reading is
the string "N/A" on 27.1.0 where 26.5.0 omits the key entirely. scalar() and
lookup() exist for exactly that, and are covered against real captures from
both majors rather than hand-written payloads.
Memory is now reported in MiB rather than the bytes rocm-smi printed, which
changes the unit of GpuInfo.memory_used/memory_total. Nothing in-tree reads
those fields numerically.
Verified on a ROCm 10.2 nightly, where rocm-smi is absent: gpu_count() and
detect_gpu_count() both return 1 where both returned 0. Verified again on
ROCm 7.2.3 (amd-smi 26.2.2), the oldest supported stack, where power and
utilization read "N/A" and arrive as None rather than crashing a float().
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Board identity, MEC firmware, per-PID VRAM, partition mode and the free-VRAM fraction. _autodetect_gpu_type moves to `amd-smi static --asic`, which returns the marketing name and the gfx target in one call. rocm-smi resolved neither on gfx1151 -- "Card Series: N/A" on ROCm 7.2.3, the generic "AMD Radeon Graphics" on 7.15 -- so the board fell through to unknown_gpu, which is the bug the board-identity change set out to fix. It resolves to radeon8060s on 7.15 and 10.2. Note the marketing name is not always resolved either: amd-smi 26.2.2 reports it as the bare device id "0x1586" for this part, which is why the target_graphics_version tier exists rather than being treated as redundant, and why the torch gcnArchName probe is kept as a last resort. The MEC probe prefers CP_MEC1 by name instead of taking whichever MEC queue amd-smi lists first. A part reporting CP_MEC2 as well would otherwise let list order decide HSA_NO_SCRATCH_RECLAIM -- the old rocm-smi line scrape took the first matching line for every part. The substring pass is kept as a fallback so a part spelling the id differently still probes rather than silently skipping the workaround. Which queue the workaround truly depends on is unverified: no dual-MEC hardware was available. Partition reads delegate to gpu_partition.py rather than growing a second parser; its _amd_smi_json is now a thin adapter over the shared reader that still raises PartitionError, so its callers are unchanged. kill_multinode.py stays stdlib-only and vendors its readers: it is cloudpickled onto every pod via NodeAffinitySchedulingStrategy, so a module-level hyperloom import would require the package on pods carrying only the serving image. A parity test asserts the vendored VRAM reader and the shared one agree on identical input, so the copy cannot drift silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sampler writes a CSV on a pod; the orchestrator reads it back by matching header names. Changing the tool changes every header, and the consumer degraded quietly rather than failing -- the highest-ranked risk in this migration. Replayed against a real amd-smi capture, the pre-change consumer resolved no usable sample at all. Predicates now match by exact column name, backed by a round-trip test that feeds the producer's own header to the consumer, so a future rename fails a test instead of silently emptying the data. Three real --csv captures are pinned, spanning every supported tool major. They are not interchangeable: 26.2.2 (ROCm 7.2.3) emits 20 columns with `usage` as a single column, 26.5.0 emits 45 and expands `usage` into gfx_activity while dropping `hotspot` entirely, and 27.1.0 emits 55 and brings `hotspot` back. The progression is not monotonic, which is why these are captures and not hand-written headers. There is no rocm-smi header path. The sampler is read from this checkout and shipped to the pod at launch, so producer and consumer are always the same build, and a pre-existing CSV whose header disagrees is now rotated out of the consumer's glob before any new row is written. Two defects found on hardware: amd-smi --csv ends every invocation with a blank line, which the sampler was timestamping into a one-cell row; and the sampler appended to a pre-existing CSV regardless of header, which misaligns every subsequent row across a tool upgrade. Also fixes a pre-existing mis-read the port exposed: temperature was coming from apu_temperature_gfx rather than edge, and the "junction" predicate matched nothing on any amd-smi version. A header that resolves no field at all is now logged at error level rather than dropping metrics in silence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five shell sites: two "is ROCm present" gates, the wedged-driver probe, the human GPU label and the max-VRAM read. The VRAM%-scraping site computes a ratio from used_vram/total_vram, since amd-smi has no VRAM% field at all. It reads those two keys by exact name: GTT is ~24x VRAM on an APU, so a loose "*vram*" match would report a wildly inflated number rather than fail. The sbatch reader also had to decouple the amd-smi call from the python parse, because pipefail propagates an earlier stage's non-zero exit through the pipeline even when the parse succeeds. The GPU label keeps amd-smi's market_name verbatim instead of re-applying the old MI###-only regex, which structurally could never match a non-Instinct part. A name amd-smi could not resolve -- "N/A", or a bare 0x device id -- is discarded so the gfx-derived label is used instead. Note the torch-compatibility gate has been silently skipping on ROCm 10 stacks, where no rocm-smi exists for `command -v` to find, so the check that stops pip pulling the NVIDIA CUDA wheel onto a ROCm pod never ran there. It now runs, which means an install on such a node can newly refuse where it previously proceeded. Neither tool sets a non-zero exit code when there is no driver -- amd-smi logs "Unable to detect any GPU devices" to stderr and still exits 0 -- so no gate here relies on exit status alone. That condition was simulated with a shim rather than reproduced on hardware. shellcheck was not available on the host these were written on, so none of the three scripts is lint-checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The probe is a shell string executed over a Ray job or SSH on a pod, not locally. `amd-smi static --asic --json` returns the marketing name and the gfx target together, so the torch gcnArchName fallback no longer fills a gap in the data and is dropped rather than keeping a remote probe dependent on torch being importable in whatever shell Ray or SSH happens to land in. Tested through a real shell against amd-smi 26.2.2, 26.5.0 and 27.1.0. On 26.2.2 the marketing name is unresolved for this part, so the probe prints "0x1586" and "gfx1151" and the consumer falls through to the gfx tier -- which is why the probe prints both lines rather than just the name. Remote execution over Ray/SSH is unverified: this was exercised on a single-node host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mostly prose, but three were executable instructions that fail outright on a ROCm 10 stack, where no rocm-smi binary exists: two specialist-prompt strings handed to an agent to run, and a pasteable VRAM recipe in SKILL.md that agents are told to run on a docker host. Every command introduced here was run against amd-smi 26.5.0 and 27.1.0 before being written down, including the SKILL.md snippet, which also degrades to "gpu_vram=unreadable" rather than raising when amd-smi is absent. Explanatory references to rocm-smi are deliberately kept where they explain why the code now reads what it reads -- that memory is MiB rather than the bytes rocm-smi printed, or that amd-smi has no VRAM% field. Removing those would delete the reasoning, not the staleness. One docstring claimed physical cards are "numbered as rocm-smi numbers them". Whether amd-smi enumerates in the same order is untested on multi-GPU, so it now says "by their absolute device index" rather than asserting the new tool's ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nothing test_preflight_gpu_visibility.py patched preflight.subprocess.run, a path the code no longer takes once _check_gpu_visibility moved onto the shared reader. Two of its three tests were failing outright and the third passed without exercising the function it named, so the GPU-visibility preflight had no working coverage. Rewritten against the real call path, plus four tests for branches that were never reached. detect_gpu_count() had no direct test despite seven production call sites. Adds coverage for visible-device mask precedence and for the empty-vs-unset distinction, which are not the same thing: an empty mask means "pinned to zero" and must not fall through to the device probe. A bash stub in test_setup_selected_python.py named a tool the installer no longer calls, so it could never fire and no assertion inspected the label it was meant to pin -- on a GPU host the test would have silently resolved the real board. Replaced with a stub matching the installer's actual contract. Tests that encode assumptions about hardware unavailable here say so in their docstrings rather than implying validation: CPX partition enumeration, where amd-smi list reports one row per partition, and which MEC queue governs HSA_NO_SCRATCH_RECLAIM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code metrics gate: FAILEDJudged: the 82 files this change touches. Files it does not touch never fail it; anything found there is listed under Outside this change for information.
Worse than the baseline: 5
Baseline out of date (run
|
| Baseline entry | Dimension | Baseline | Now |
|---|---|---|---|
src/hyperloom/inference_optimizer/tests/test_common_utils.py |
Duplicated lines per file | 958 | 948 |
src/hyperloom/inference_optimizer/tests/test_coverage_margin3_unit.py |
Duplicated lines per file | 191 | 181 |
src/hyperloom/inference_optimizer/tests/test_workload_envs.py |
Duplicated lines per file | 402 | 400 |
src/hyperloom/inference_optimizer/tests/test_workload_envs_branches_unit.py |
Duplicated lines per file | 403 | 401 |
Comments: 1
| Where | Problem |
|---|---|
src/hyperloom/inference_optimizer/tests/test_visible_devices.py:142 |
comment points at a PR or issue (PR #1827) |
Repeated literals: 2
| Where | Problem |
|---|---|
src/hyperloom/common/amd_smi.py:42 |
1024 is written 3 times (was 0); name it as a module-level constant |
src/hyperloom/common/amd_smi.py:46 |
1024.0 is written 4 times (was 0); name it as a module-level constant |
How to fix
- New or worse: split the unit (extract a function, a class, a module) until it is back under the limit, or no worse than its recorded value. The baseline cannot absorb a new violation: CI refuses any baseline entry added or raised relative to the base branch.
- Baseline out of date: something improved. Run
python scripts/code_metrics.py --update-baselineand commitscripts/code_metrics_baseline.txt; the command only lowers or removes entries. - Comments: a comment block of at most 8 lines that says what the code does and why; the history (PR, issue, incident) goes in the commit message and the PR description.
- Repeated literals: name the value once as a module-level constant and use the name.
- Override: the
baseline-raisePR label waives new, worse and baseline-growth findings and the failing design checks (they stay listed here). Use it only with the reason in the PR description; it waives nothing else. - Tools for a local run:
pip install ruff==0.16.2 complexipy==8.0.1 vulture==2.16andnpm install -g jscpd@5.4.1(or pointCODE_METRICS_JSCPDat another jscpd command, such asnpx --yes jscpd@<version>).
Module length warning: over 800 lines, within the 1200-line failure limit (not failing): 4
| Module | Lines | Limit |
|---|---|---|
src/hyperloom/inference_optimizer/cli/parser.py |
1185 | warns > 800 |
src/hyperloom/inference_optimizer/multi_node/scripts/launch_multinode.py |
1127 | warns > 800 |
src/hyperloom/orchestrator/actions/executors/benchmark_result.py |
1017 | warns > 800 |
src/kernelforge/gemm_tune/cli.py |
936 | warns > 800 |
Already over at the base (backlog, not this change's): 17
98 findings in files this change does not touch
Outside this change (informational): 98
| Unit | Dimension | Baseline | Now |
|---|---|---|---|
src/hyperloom/common/process_owner.py supervise |
Cognitive complexity | absent | 38 |
src/hyperloom/inference_optimizer/reference_script.py render_reference_script |
Cognitive complexity | absent | 31 |
src/hyperloom/orchestrator/enablement/recipe/projections.py project_launch_evidence |
Cognitive complexity | absent | 36 |
src/hyperloom/orchestrator/phases/machine_state.py _kernel_exit |
Cognitive complexity | absent | 33 |
src/hyperloom/orchestrator/state/shared_state.py resolve_graded_comparison |
Cognitive complexity | absent | 34 |
src/hyperloom_kb/collect/mapping.py _experience |
Cognitive complexity | absent | 31 |
src/hyperloom_kb/schema.py FieldDeclaration.__post_init__ |
Cognitive complexity | absent | 32 |
src/kernelforge/cli/forge_loop_command.py _prepare_measurement_driver |
Cognitive complexity | absent | 36 |
src/kernelforge/cli/forge_loop_command.py _read_kb_warm_start |
Cognitive complexity | absent | 40 |
src/kernelforge/loop/runner.py IterationLoop._collect_session_candidate |
Cognitive complexity | absent | 44 |
src/kernelforge/loop/runner.py IterationLoop._rule_on_attempt |
Cognitive complexity | absent | 38 |
src/hyperloom/orchestrator/loop/dispatcher.py DispatcherCollaborator.run_task_registered |
Cyclomatic complexity | absent | 25 |
src/hyperloom_kb/tests/test_cli.py |
Duplicated lines per file | absent | 18 |
src/hyperloom_kb/tests/test_global_identity_and_pull.py |
Duplicated lines per file | absent | 13 |
src/hyperloom_kb/tests/test_local_state.py |
Duplicated lines per file | absent | 18 |
src/hyperloom/inference_optimizer/multi_node/cli.py _restart_multinode |
Function length (lines) | absent | 107 |
src/hyperloom_kb/http_service.py RequestHandler._dispatch |
Function length (lines) | absent | 91 |
src/hyperloom_kb/sync.py GlobalSync.pull |
Function length (lines) | absent | 86 |
src/kernelforge/cli/forge_loop_command.py _RunReporter.build_result |
Function length (lines) | absent | 91 |
src/kernelforge/cli/forge_loop_command.py _prepare_measurement_driver |
Function length (lines) | absent | 140 |
src/kernelforge/cli/forge_loop_command.py _read_kb_warm_start |
Function length (lines) | absent | 154 |
src/kernelforge/loop/knowledge_integration.py _measure_warmstart_field |
Function length (lines) | absent | 83 |
src/kernelforge/loop/runner.py IterationLoop._close_iteration |
Function length (lines) | absent | 135 |
src/kernelforge/loop/runner.py IterationLoop._collect_session_candidate |
Function length (lines) | absent | 187 |
src/kernelforge/loop/runner.py IterationLoop._implementer_history |
Function length (lines) | absent | 112 |
...and 73 more; the job log lists every row.
Verdict computed by the gate scripts in
/home/runner/work/_temp/code-metrics-gate, not by this tree's copy.
Thresholds and sources: [tool.hyperloom.code_metrics] in pyproject.toml. Tools: complexipy 8.0.1, jscpd 5.4.1, ruff 0.16.2, vulture 2.16.
| for name in names: | ||
| if name not in mapping: | ||
| continue | ||
| value = mapping[name] |
This branch has not been deployed
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
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.
Description: what and why
ROCm 7.0 deprecated
rocm-smiin favour ofamd-smi, and per the ROCm 10.1 release notesrocm-smi-libis no longer built or installed as part of the standard ROCm/TheRock build. Hyperloom supports 10.0 today, so this gates moving the support matrix past it.Two live bugs, both measured. The ROCm 10 wheels ship no
rocm-smi, wheredetect_gpu_count()swallowed the absence viaexcept FileNotFoundError: return 0— Ray then derived 0 GPUs and a run hung on an actor that could never be scheduled (0 -> 1on a 10.2 nightly). Androcm-smi --showproductnameresolves no board on gfx1151 (Card Series: N/Aon 7.2.3, genericAMD Radeon Graphicson 7.15), so identity fell through tounknown_gpu, whereamd-smi static --asicreturns the marketing name and gfx target together (None -> radeon8060s).19 invocation sites plus a CSV-schema consumer; six centralise into a new
hyperloom/common/amd_smi.py, three of them byte-identicalGPU[scrapers. All--json, matched by exact column name. Norocm-smifallback — every supported ROCm shipsamd-smi, so a second parser would serve no supported configuration. Since 7.2.x and 10.0 are supported at once the reader tolerates amd-smi 26.2.2 and 27.1.0 together: a field may be a bare number or{value, unit}, an unsupported reading is"N/A"on one major and an absent key on the other, and--csvwidth runs 20 / 45 / 55 columns non-monotonically.Linked issue(s): none exist — JIRA has nothing for
rocm-smi, and no Hyperloom issue or PR covers the migration. Overlaps Give the Radeon 8060S a board identity instead of unknown_gpu #1483 (already merged here) and Track experimental Radeon 8065S (Halo/gfx1151) custom-workload bring-up #1467 in the product-name path; sincerocm-smi --showproductnameresolves nothing on gfx1151, this is arguably a better route to Give the Radeon 8060S a board identity instead of unknown_gpu #1483's goal than extendingrocm-smiuse there.Tests: added/updated, all passing locally.
test_amd_smi.pyandtest_gpu_sampler_round_trip.py(new). The round-trip test feeds the producer's own header to the consumer, so a column rename fails a test instead of silently emptying the data, and asserts the negative — that no predicate reaches a column it must not, such as a GTT capacity standing in for a VRAM percentage. Three real--csvcaptures are pinned as fixtures rather than hand-written headers.test_preflight_gpu_visibility.pyrewritten: it patched a call path the code no longer takes, so two of three tests were failing and the third passed without exercising its target — the GPU-visibility preflight had no working coverage.test_visible_devices.py:detect_gpu_count()had no direct test despite seven call sites. Adds mask precedence and the empty-vs-unset distinction, since an empty mask means "pinned to zero" and must not fall through.Exercised on three ROCm stacks including 7.2.3 / amd-smi 26.2.2, the oldest supported, where power and utilization read
"N/A"and arrive asNonerather than crashing afloat(). MI300/MI355 is entirely unverified — all hardware testing ran on an APU outside the support matrix, and those assumptions are marked in the test docstrings. Most notablyamd-smi listenumerates one row per partition under CPX, so the GPU-count sites may see 8x the card count; that is the likeliest follow-up. Remote Ray/SSH probe execution, the missing-driver path andshellcheckon the shell scripts are also unverified.Size/complexity triggers crossed: yes — 82 files, ~2.2k insertions, as seven commits one per area (
common:/inference_optimizer:/orchestrator:/assets:/multi_node:/docs:/tests:). The first is the smallest and most load-bearing.If this simplifies or refactors: yes — three byte-identical scrapers and an inlined duplicate VRAM reader collapse into one module, and
gpu_partition.py's_amd_smi_jsonbecomes a thin adapter over it that still raisesPartitionError, leaving its callers unchanged. Nothing is removed butrocm-smisupport itself.Observable effect: GPU detection works on a ROCm 10 stack instead of hanging a run on an unschedulable actor; the board resolves to a real identity instead of
unknown_gpu; pod GPU metrics keep arriving across the tool change instead of silently emptying.Four defects surfaced, three pre-dating this work: temperature came from
apu_temperature_gfxrather thanedgeand thejunctionpredicate matched no column on any amd-smi version;amd-smi --csvends each invocation with a blank line the sampler was timestamping into a junk row; the sampler appended to a pre-existing CSV regardless of header, misaligning every row across a tool upgrade; and the MEC probe let list order decideHSA_NO_SCRATCH_RECLAIMon a part reporting both queues.Breaking changes: the torch-compatibility gate has been silently skipping on ROCm 10 stacks, where no
rocm-smiexisted forcommand -vto find, so the check that stopspippulling the NVIDIA CUDA wheel onto a ROCm pod never ran there. It now runs, and an install on such a node can newly refuse where it previously proceeded. Separately, memory is reported in MiB rather than bytes, changing the unit ofGpuInfo.memory_used/memory_total; nothing in-tree reads those numerically.PR addresses single concern: yes — one migration, split by area for review and bisect.
Root cause is upstream, ticket filed: no — the deprecation is upstream ROCm, but every change here is in Hyperloom. Note this fixes only the multi-node half of GPU telemetry: single-node metrics come from Magpie's
GPUMonitor, whose vendor probe still gates onrocm-smiand so returnsunknownon a ROCm 10 stack, skipping collection silently. That and GEAK's ownrocm-smicall sites are being ported separately; TraceLens has none.