Treat GPU activity as optional and hand the worktree back clean

[zhiding512]

Addresses items 1-3 of @zufayu's review at head 706eafafc, plus one related portability gap found while reproducing item 1. Based directly on 706eafafc, so it applies to the current PR head.

What changed

1. amdsmi_get_gpu_activity is no longer a hard dependency (review item 1)

A new read_activity() helper returns None for an unavailable metric instead of raising, and is shared by the picker and both inline probes in validate_pr.sh (record_gpu_activity_after, and the gpu_claim metadata probe). gpu_claim.idleness_basis now reports the evidence the claim rests on, in the shape you already use for arch_coverage_basis:

• activity+vram — busy percentages were measured
• vram-only — the activity API was unavailable; only resident VRAM separated the devices

Unknown stays distinct from zero. The previous gfx = gfx if isinstance(gfx, int) else 0 already silently reported an N/A metric 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_claim skip 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 own PATCH_APPLIED flag, reverted in cleanup(). BASE_ACTIVE keeps 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_ACTIVE stays 0. cleanup() called restore_head, which re-applies the patch, so even a run that reached the baseline path exited with the worktree patched. Every path leaked, and the NO_GPU branch was just the easiest one to hit.

3. pick-idle-gpu.py has 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_cli is the CLI package and exposes no amdsmi module. The fallback probed only the latter, so on those containers the picker exited 2 with No module named 'amdsmi' before reaching any activity query — the same NO_GPU end 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/rocm and /opt/rocm-* under share/amd_smi, newest first.

SKILL.md documents the optional metric, idleness_basis, the split skip semantics, and the worktree revert. report_schema.json declares idleness_basis rather 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_activity works here and returns gfx_activity: 0 on all 8 GPUs, and a wider probe (gpu_metrics_info, power_info, board_info, process_list) found no failure either. Error 43 comes from native libamd_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 real amdsmi that forces amdsmi_get_gpu_activity to raise AmdSmiLibraryException(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:

Call site	Before
pick-idle-gpu.py:67	exit 2, AMD SMI probe failed: 43 | AMDSMI_STATUS_UNEXPECTED_DATA
validate_pr.sh:462 metadata probe	GPU_INFO_RC=1, empty output, gpu_claim: skip
validate_pr.sh:314 post-run	tolerated, but prints a full traceback
picker, no fault, container as-is	exit 2, No module named 'amdsmi'
worktree after one NO_GPU run	M utils.py + ?? test_flydsl_utils.py; second run: 8 stage result was missing notes + not isolated-clean

I also confirmed empirically that PICKER cannot route around item 1: a working VRAM-only picker returned exit 0 and a valid HIP index, and gpu_claim still skipped, because the probe at :437 loads $SCRIPT_DIR/pick-idle-gpu.py rather 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_DIR reference.

After the change:

Scenario	Result
Fault injected, all three call sites	picker exit 0 vram-only; probe RC=0 with gfx_activity_before_pct: null; post-run records a note, no traceback
Container as-is, no fault	exit 0, activity+vram, all 8 GPUs idle
HIP 0-3 reporting 90% busy	correctly excluded, picks HIP 4, basis activity+vram
Partial failure: 0-1 fail, 2 busy, rest idle	picks HIP 3 — measured-idle preferred over unknown, busy still excluded
All 8 reporting 90% busy	exit 1, not a vram-only fallback
All 8 failing	exit 0, basis honestly vram-only
NO_GPU path, two consecutive runs	worktree clean both times, 0 missing-stage notes
Real-GPU path, two consecutive runs	worktree clean both times
SIGTERM / SIGINT while patch applied	worktree clean (unpatched: residue)
SIGTERM in the base-state window	worktree clean (unpatched: residue)

A full real-GPU run reaches the baseline block, so the BASE_ACTIVE path is genuinely exercised rather than skipped:

gpu_claim: pass, arch gfx942, idleness_basis "activity+vram",
           gfx_activity_before_pct 0, gfx_activity_after_pct 0
baseline_control: pass, repo_tests.state "target-not-present"
correctness_repo_tests: pass, tests:1 executed:1
arch_coverage: {"gfx942": "runtime"}

The interrupt and residue tests were checked against unmodified 706eafafc first 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 regression
• ruff check pick-idle-gpu.py clean; bash -n validate_pr.sh clean; report_schema.json parses
• Patch applies cleanly to 706eafafc and the applied files are byte-identical to the tested ones
• Native confirmation on a ROCm 7.0 host that error 43 is gone — I could not reproduce it natively and would appreciate @zufayu re-running on the MI308X box that surfaced it

Not 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 GPU activity optional while ensuring validation restores the supplied worktree state after every run.

New Features:

• Treat GPU activity metrics as optional and record whether GPU idleness was established from activity plus VRAM or VRAM alone.
• Support AMD SMI Python bindings across additional ROCm installation layouts.

Bug Fixes:

• Prevent validation runs from leaving the candidate patch applied, including on degraded paths and interrupts.
• Avoid misreporting unavailable GPU activity as measured idle and distinguish unavailable AMD SMI from an environment with no idle GPUs.

Enhancements:

• Add executable invocation support for the idle-GPU picker and clarify validator behavior for optional activity metrics and worktree restoration.

Documentation:

• Document optional GPU activity, idleness evidence, skip semantics, and worktree restoration behavior.

Tests:

• Verify GPU selection and validator behavior across activity failures, busy GPUs, VRAM-only fallback, repeated runs, and interrupts.

Chores:

• Declare the new idleness evidence field in the validation report schema.

[sourcery-ai]

Reviewer's Guide

The validator now tolerates unavailable AMD SMI activity metrics and ROCm binding layout differences, records whether GPU idleness was established by activity plus VRAM or VRAM alone, and reliably returns the worktree to its input state on all exit paths.

Sequence diagram for optional GPU activity validation

sequenceDiagram
    participant Validator
    participant Picker as pick-idle-gpu.py
    participant AMD_SMI as AMD SMI
    participant Worktree

    Validator->>Picker: main()
    Picker->>AMD_SMI: amdsmi_get_gpu_activity(handle)
    alt Activity available
        AMD_SMI-->>Picker: gfx_activity
        Picker-->>Validator: selected GPU, idleness-basis: activity+vram
    else Activity unavailable
        AMD_SMI-->>Picker: AmdSmiException or N/A
        Picker-->>Validator: selected GPU, idleness-basis: vram-only
    end
    Validator->>AMD_SMI: read_activity(handle)
    AMD_SMI-->>Validator: measured percentage or null
    Validator->>Validator: Record gpu_claim.idleness_basis

Loading

Flow diagram for worktree restoration

flowchart TD
    A[Validator starts with supplied worktree] --> B[Apply candidate patch]
    B --> C[PATCH_APPLIED = 1]
    C --> D{Validation path or interrupt}
    D --> E[cleanup]
    E --> F{BASE_ACTIVE?}
    F -->|yes| G[Mark patch as already reversed]
    F -->|no| H[git apply -R PATCHF]
    G --> I[Worktree matches supplied state]
    H --> I

Loading

File-Level Changes

Change	Details	Files
Make GPU activity optional while preserving honest idleness claims and adding ROCm-version portability.	Centralize activity reads so unavailable or non-numeric metrics remain unknown instead of becoming zero. Allow VRAM-only selection when activity is unavailable, while preferring measured-idle GPUs and excluding measured-busy GPUs. Emit and schema-declare idleness_basis, distinguish portability failures from no-idle-GPU skips, and tolerate unavailable post-run probes. Expand AMD SMI binding discovery for ROCm >= 7.1 and add an executable shebang.	.claude/skills/validate-kernel-pr/pick-idle-gpu.py .claude/skills/validate-kernel-pr/validate_pr.sh .claude/skills/validate-kernel-pr/report_schema.json .claude/skills/validate-kernel-pr/SKILL.md
Ensure validation restores the supplied worktree state across normal, degraded, and interrupted execution paths.	Track whether this process applied the candidate patch independently from whether the baseline state is active. Revert an applied patch during cleanup, avoid reapplying it after baseline validation, and handle signal-triggered exits.	.claude/skills/validate-kernel-pr/validate_pr.sh .claude/skills/validate-kernel-pr/SKILL.md
Document the revised validator contract and report semantics for reviewers and downstream consumers.	Describe optional activity evidence, null-versus-zero semantics, split skip meanings, and clean-worktree guarantees. Declare idleness_basis in the report schema.	.claude/skills/validate-kernel-pr/SKILL.md .claude/skills/validate-kernel-pr/report_schema.json

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zhiding512]

Superseded by #2, which carries the identical commit (fb95189) from a branch in ROCm/aiter (zhiding512/validate-kernel-pr-amdsmi-fix) instead of a personal fork. Same 4 files, +146/-36, same base. Closing this one to avoid a duplicate review.