Treat GPU activity as optional and hand the worktree back clean - #2
Closed
zhiding512 wants to merge 2 commits into
Closed
Conversation
amdsmi_get_gpu_activity was a hard dependency at three call sites, so a host where that single query fails degrades the whole run to NO_GPU even with idle GPUs available. @zufayu hit this on MI308X / ROCm 7.0, where the call returns AMDSMI_STATUS_UNEXPECTED_DATA (43) while enumeration, BDF, ASIC and VRAM queries all work. PICKER could not route around it because the executor re-queries the API inline rather than going through the picker's selection. Activity is now optional. A new read_activity() helper is shared by the picker and both inline probes, and gpu_claim.idleness_basis names the evidence the claim rests on: activity+vram, or vram-only when only resident VRAM separated the devices. Unknown stays distinct from zero -- the previous `gfx if isinstance(gfx, int) else 0` substitution reported an unavailable metric as a measured idle GPU. The gpu_claim skip also stops conflating "GPUs present but none idle", an environment fact, with "AMD SMI is unqueryable", a portability gap in the validator. The import fallback did not probe $ROCM_PATH/share/amd_smi, where the bindings ship on ROCm >= 7.1, so on those containers the picker exited 2 before reaching any activity query -- the same NO_GPU symptom from an unrelated cause. Separately, merge_sim applied the patch but cleanup was guarded by BASE_ACTIVE, which is set only inside the baseline path. A run that skipped correctness exited with the patch applied, and when the baseline path did run, cleanup's restore_head re-applied it, so every path left the caller's worktree patched and the next run reported not isolated-clean against the caller. The application now carries its own flag and is reverted on exit, including on interrupt. Reported by @zufayu on #4870.
Reviewer's GuideThe PR makes AMD GPU activity best-effort with honest VRAM-only reporting and selection behavior, expands AMD SMI import portability, and ensures validation always returns the worktree to its supplied state; documentation, schema, and fault/interrupt regression coverage are updated accordingly. Sequence diagram for optional GPU activity selectionsequenceDiagram
participant Picker as pick-idle-gpu.py
participant AMD_SMI as AMD_SMI
participant GPU as GPU
Picker->>AMD_SMI: amdsmi_get_processor_handles()
loop sampling window
Picker->>AMD_SMI: read_activity(amdsmi, handle)
AMD_SMI-->>Picker: GFX/UMC percentages or unavailable
Picker->>AMD_SMI: amdsmi_get_gpu_vram_usage(handle)
AMD_SMI-->>Picker: resident VRAM
end
alt activity measured
Picker->>Picker: Select measured-idle GPU
Picker-->>Picker: idleness-basis: activity+vram
else activity unavailable
Picker->>Picker: Select VRAM-eligible GPU
Picker-->>Picker: idleness-basis: vram-only
else known-busy GPU
Picker-->>GPU: Exclude GPU
end
Flow diagram for validator worktree restorationflowchart TD
Start([Validator starts]) --> Apply{Patch applies?}
Apply -->|No| Exit([Exit with supplied worktree])
Apply -->|Yes| Applied[Set PATCH_APPLIED=1]
Applied --> Run[Run validation and optional baseline]
Run --> Interrupt{Normal exit or interrupt}
Interrupt -->|Either| Cleanup["cleanup()"]
Cleanup --> Baseline{BASE_ACTIVE=1?}
Baseline -->|Yes| ClearBase[Clear PATCH_APPLIED; remain in supplied state]
Baseline -->|No| Revert[git apply -R patch]
ClearBase --> Clean([Return worktree clean])
Revert --> Clean
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
4 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
A target driven by a `__main__` guard could reach neither correctness_s1_grid nor
execution_receipt, so INCONCLUSIVE was the hard ceiling for every aiter
`op_tests/*.py` -- the repository's standard test shape. Neither skip described a
property of the target:
* the route profiler is sys.setprofile plus a receipt write, and pytest was
only where the hook happened to be installed;
* the grid was injected solely through an environment variable, while these
targets take shapes on the command line. That is a limit of the injector.
Reported as skips, both read as honest abstention while actually hiding a
capability gap -- the failure mode this skill exists to prevent.
The probe is now installed for script targets by run_script_with_probe.py, which
executes the file under runpy with run_name="__main__". It calls the probe's own
hooks rather than re-implementing the profiling, so `producer` stays truthful and
the tested PR still cannot forge a receipt. The probe module is generated whenever
a route is named, not only on the pytest branch; without that the runner had
nothing to import.
--shape-arg names the target's own CLI flag for the grid. The flag is named by the
caller rather than guessed, because a wrong guess appends argv the target silently
ignores. The hook is held to the same standard of proof as the env-var channel: the
flag must appear in an add_argument call, and the existing invalid-grid probe still
has to make the target fail before the stage is credited.
A receipt is now validated whenever a route was named, including when no grid was
configured or the grid channel could not be established. Two branches used to
abandon the receipt along with the grid, discarding evidence already collected. With
no grid it asserts route execution and nothing about shapes, which is all it is then
entitled to claim.
report_schema.json relaxes the PASS constraint `test_selection.runner: const pytest`
to `enum [pytest, script]`, since a script target can now supply both missing
stages. shape_arg is declared but deliberately not required, so reports from earlier
validators stay valid.
Verified on #4538 (gfx950, MI355 OAM), base 78b9440/891788643:
* script target with --shape-arg -s: PASS 9/9, exit 0, review-pr consumable,
receipt naming _auto_variant over the four injected production shapes
* unaccepted --shape-arg flag: correctness_s1_grid skip, receipt still pass
* seeded defect (fused -inf fill dropping [e, seq_len_kv)): BLOCK, exit 1, with
correctness_s1_grid failing independently of the PR's own suite
* tests/test_validator.py 24 passed; PASS/INCONCLUSIVE/BLOCK reports and one
pre-shape_arg report all validate against the schema
zhiding512
force-pushed
the
zhiding512/validate-kernel-pr-amdsmi-fix
branch
from
August 31, 2026 08:36
995af8b to
6de48d6
Compare
Author
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
1 participant
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.
Addresses items 1-3 of @zufayu's review at head
706eafafc, plus one related portability gap found while reproducing item 1. Based directly on706eafafc, so it applies to the current PR head.What changed
1.
amdsmi_get_gpu_activityis no longer a hard dependency (review item 1)A new
read_activity()helper returnsNonefor an unavailable metric instead of raising, and is shared by the picker and both inline probes invalidate_pr.sh(record_gpu_activity_after, and thegpu_claimmetadata probe).gpu_claim.idleness_basisnow reports the evidence the claim rests on, in the shape you already use forarch_coverage_basis:activity+vram— busy percentages were measuredvram-only— the activity API was unavailable; only resident VRAM separated the devicesUnknown stays distinct from zero. The previous
gfx = gfx if isinstance(gfx, int) else 0already silently reported anN/Ametric as a measured-idle GPU, which is the overclaim-by-omission this PR argues against, so that substitution is gone too. Selection still prefers GPUs whose idleness was actually measured, and a GPU with a known-busy reading is still excluded.The
gpu_claimskip no longer conflates two different facts: "GPUs present but none idle" is an environment fact, while "AMD SMI is unqueryable" is a portability gap in the validator and says nothing about the GPUs.2. The worktree is handed back in the state it was supplied (review item 2)
merge_sim's patch application now carries its ownPATCH_APPLIEDflag, reverted incleanup().BASE_ACTIVEkeeps its original meaning of "currently in base state".One correction to the review's diagnosis: the residue was not limited to the paths where
BASE_ACTIVEstays0.cleanup()calledrestore_head, which re-applies the patch, so even a run that reached the baseline path exited with the worktree patched. Every path leaked, and theNO_GPUbranch was just the easiest one to hit.3.
pick-idle-gpu.pyhas a shebang (review item 3)4. The amdsmi import fallback did not cover ROCm >= 7.1 (not in the review)
On ROCm 7.2 the Python bindings ship at
$ROCM_PATH/share/amd_smi;/opt/rocm/libexec/amdsmi_cliis the CLI package and exposes noamdsmimodule. The fallback probed only the latter, so on those containers the picker exited 2 withNo module named 'amdsmi'before reaching any activity query — the sameNO_GPUend state as item 1, from an unrelated cause. Item 1's fix does not help here, since this fails at import. The search list now also probes$ROCM_PATH,/opt/rocmand/opt/rocm-*undershare/amd_smi, newest first.SKILL.mddocuments the optional metric,idleness_basis, the split skip semantics, and the worktree revert.report_schema.jsondeclaresidleness_basisrather than letting it appear undeclared.Verification
Host: MI308X (gfx942) x8, ROCm 7.2.0, amd-smi 26.2.1, torch 2.9.1, amdgpu 6.16.13.
Caveat on the evidence, stated up front: error 43 does not reproduce natively on this host.
amdsmi_get_gpu_activityworks here and returnsgfx_activity: 0on all 8 GPUs, and a wider probe (gpu_metrics_info,power_info,board_info,process_list) found no failure either. Error 43 comes from nativelibamd_smi.so, so it is amd-smi/ROCm-version dependent, not a gfx942 hardware trait — the driver is shared between host and container here, and only the userspace library version differs. Item 1 was therefore verified by fault injection: a shim wrapping the realamdsmithat forcesamdsmi_get_gpu_activityto raiseAmdSmiLibraryException(43). Items 2-4 are verified natively. I have no direct observation of the ROCm 7.0 native failure and am not claiming one.Reproduced first, against unmodified
706eafafc:pick-idle-gpu.py:67AMD SMI probe failed: 43 | AMDSMI_STATUS_UNEXPECTED_DATAvalidate_pr.sh:462metadata probeGPU_INFO_RC=1, empty output,gpu_claim: skipvalidate_pr.sh:314post-runNo module named 'amdsmi'NO_GPUrunM utils.py+?? test_flydsl_utils.py; second run: 8stage result was missingnotes +not isolated-cleanI also confirmed empirically that
PICKERcannot route around item 1: a working VRAM-only picker returned exit 0 and a valid HIP index, andgpu_claimstill skipped, because the probe at:437loads$SCRIPT_DIR/pick-idle-gpu.pyrather than$PICKER. After this change the inline calls tolerate the failure, so there is nothing left to route around and I did not alter that$SCRIPT_DIRreference.After the change:
vram-only; probeRC=0withgfx_activity_before_pct: null; post-run records a note, no tracebackactivity+vram, all 8 GPUs idleactivity+vramvram-onlyfallbackvram-onlyNO_GPUpath, two consecutive runsA full real-GPU run reaches the baseline block, so the
BASE_ACTIVEpath is genuinely exercised rather than skipped:The interrupt and residue tests were checked against unmodified
706eafafcfirst to confirm they discriminate, so "clean" is a real result rather than a test that cannot fail.Test plan
tests/test_validator.py: 24 passed — same before and after, no regressionruff check pick-idle-gpu.pyclean;bash -n validate_pr.shclean;report_schema.jsonparses706eafafcand the applied files are byte-identical to the tested onesNot included
Review item 4 — making the validation report expected rather than required for FlyDSL kernel PRs in
review-pr— is a judgement about your own skill's mandate rather than a defect, so I left that wording to you.Made with Cursor
Summary by Sourcery
Make kernel validation portable across AMD SMI environments, preserve worktree cleanliness, and extend evidence collection for script and shape-driven targets.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Chores:
The head branch lives in
ROCm/aiteraszhiding512/validate-kernel-pr-amdsmi-fixrather than in a personal fork, so you may not have write access to push to it. If you want changes made to this branch, say so and I will push them.