Skip to content

ci: gate on KFD IOCTL version in driver/GPU sanity check - #7360

Merged
lumachad merged 4 commits into
mainfrom
users/lumachad/kfd-version-check
Sep 1, 2026
Merged

lumachad merged 4 commits into
mainfrom
users/lumachad/kfd-version-check

Conversation

@lumachad

@lumachad lumachad commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Extends build_tools/print_driver_gpu_info.py to query the KFD IOCTL
version from /dev/kfd and log it alongside the existing amd-smi and
rocminfo output. The sanity job now exits non-zero when the version falls
below the minimum rocdbgapi requires (>= 1.13), when /dev/kfd is
missing, or when the ioctl call fails. A version above the tested ceiling
(>= 2.0) produces a warning in the log but does not block testing, since
it may still work fine in practice.

The test_sanity_check job in test_artifacts.yml is gated directly
on fromJSON(sanity_component).test_runner, removing the separate
run_gpu_sanity_check output variable that was introduced in an earlier
iteration of this PR.

Motivation

CI runs on runners with old KFD drivers (e.g. version 1.6) were producing
a cryptic rocdbgapi failure:

rocm-dbgapi: warning: AMD GPU driver's version 1.6 not supported (version must be >= 1.13 and < 2.0)
rocm-debug-agent: error: amd_dbgapi_process_attach(...) failed (rc=-10)

Surfacing the KFD version in the existing sanity step makes mismatches
immediately visible in CI logs and blocks further testing on a runner that
would produce misleading results. CPU-only runners are unaffected because
the KFD section only runs on Linux with a GPU runner.

ISSUE ID: N/A

@therock-pr-bot

therock-pr-bot Bot commented Aug 14, 2026 •

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
📝 PR Description ❌ Fail Error: PR description must reference a JIRA ID, ISSUE ID, or a GitHub closing keyword.
Expected: include a JIRA ID / ISSUE ID line (separator : or -, or omitted; value may be a JIRA key, a number with/without #, or a link), OR a closing keyword + issue reference. Accepted examples:
• JIRA ID : TESTAUTO-6039
• JIRA ID - #330
• JIRA ID #330
• JIRA ID (on separate line)
ROCM-25757
• ISSUE ID : TESTUTO-3334
• ISSUE ID #3334
• ISSUE ID - TESTAUTO-3433
• ISSUE ID (on separate line)
AIRUNTIME-2352
• ISSUE ID : https://github.com/<org_name>/<repo_name>/issues/1234
• Closes #10
• Fixes octo-org/octo-repo#100
• Resolves: #123
• #123
• https://github.com/<org_name>/<repo_name>/issues/123
Current: no valid JIRA/ISSUE/closing-keyword reference found
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ PR Description

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot therock-pr-bot Bot added the Not ready to Review PR has unresolved policy failures — reviews blocked label Aug 14, 2026
@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@lumachad lumachad added ci:run-all-archs Opt-in to building for all architectures on a pull request test_filter:quick If enabled, the PR will run quick tests labels Aug 14, 2026
@lumachad
lumachad requested a review from ndhillon07 August 14, 2026 06:23
@lumachad

Copy link
Copy Markdown
Contributor Author

Quick test for now.

@lumachad

Copy link
Copy Markdown
Contributor Author

Looks like the GPU sanity checks are running even on CPU runners. That doesn't seem right.

@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch 2 times, most recently from 1c763e0 to fbc73b6 Compare August 18, 2026 11:27
@lumachad lumachad changed the title ci: add KFD IOCTL version check to driver/GPU sanity job ci: add KFD IOCTL version check and skip for CPU-only runners Aug 18, 2026
@lumachad
lumachad marked this pull request as ready for review August 18, 2026 17:14
@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch from fbc73b6 to c6d472e Compare August 18, 2026 17:14
@lumachad lumachad removed the test_filter:quick If enabled, the PR will run quick tests label Aug 18, 2026
@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch 2 times, most recently from 2cfe1a8 to c920b0b Compare August 20, 2026 06:55
@lumachad
lumachad requested a review from geomin12 August 21, 2026 07:34
@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch from c920b0b to 43e2b0b Compare August 24, 2026 07:56
Comment thread build_tools/print_kfd_version.py Resolved
@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch from 43e2b0b to f2af63f Compare August 25, 2026 11:25
@lumachad lumachad changed the title ci: add KFD IOCTL version check and skip for CPU-only runners ci: report KFD IOCTL version in driver/GPU sanity check Aug 25, 2026
@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch 2 times, most recently from 3e6a4dd to 3a7ae10 Compare August 26, 2026 07:35
@lumachad
lumachad requested review from ScottTodd and geomin12 August 26, 2026 14:06

@ScottTodd ScottTodd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM % the workflow and comment details.

Comment thread build_tools/print_driver_gpu_info.py Resolved
Comment thread .github/workflows/test_artifacts.yml Resolved
@lumachad lumachad changed the title ci: report KFD IOCTL version in driver/GPU sanity check ci: gate on KFD IOCTL version in driver/GPU sanity check Aug 27, 2026
Comment thread build_tools/print_driver_gpu_info.py Resolved

@geomin12 geomin12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! this is great

@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch 2 times, most recently from 821b3e3 to f995d75 Compare August 28, 2026 13:59
@lumachad lumachad self-assigned this Aug 28, 2026
@lumachad

Copy link
Copy Markdown
Contributor Author

Looks like CI is a bit flakey.

lumachad and others added 4 commits September 1, 2026 11:50
Extends print_driver_gpu_info.py to query the KFD IOCTL version from
/dev/kfd and log it alongside the existing amd-smi and rocminfo output.
The check is informational only: it warns when the version falls outside
the range rocdbgapi requires (>= 1.13, < 2.0) but never fails the step.

Removes the standalone print_kfd_version.py and the separate KFD version
check step from test_component.yml, test_pytorch_wheels.yml, and
test_multi_arch_linux_jax_wheels.yml. Also emits a run_gpu_sanity_check
boolean from fetch_test_configurations.py so the sanity job in
test_artifacts.yml is skipped cleanly for CPU-only families.

Co-Authored-By: Claude <noreply@anthropic.com>
Drop the run_gpu_sanity_check output variable from fetch_test_configurations.py
and test_artifacts.yml per Scott's suggestion. The test_sanity_check job now
gates directly on fromJSON(sanity_component).test_runner, which is already
non-empty only on GPU runners.

Also fix the print_driver_gpu_info.py docstring, which incorrectly claimed
the script always exits 0. Since PR #6604 the run_command() calls use
check=True, so driver failures produce a non-zero exit.

Co-Authored-By: Claude <noreply@anthropic.com>
The check now returns 1 (and the script exits non-zero) when /dev/kfd is
missing, when the ioctl fails, or when the version falls outside the
supported range for rocdbgapi (>= 1.13, < 2.0). Previously these were
logged as warnings but never failed the sanity job.

Co-Authored-By: Claude <noreply@anthropic.com>
A version below the minimum still hard-fails because it is known to
produce broken rocdbgapi output. A version above the rocdbgapi tested
ceiling may still work and should not block testing, so it now logs a
warning and continues.

Co-Authored-By: Claude <noreply@anthropic.com>
@lumachad
lumachad force-pushed the users/lumachad/kfd-version-check branch from f995d75 to bfbeba1 Compare September 1, 2026 10:50
@lumachad
lumachad merged commit f3e9941 into main Sep 1, 2026
252 of 258 checks passed
@lumachad
lumachad deleted the users/lumachad/kfd-version-check branch September 1, 2026 14:21
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Sep 1, 2026
amd-justchen added a commit to amd-justchen/TheRock that referenced this pull request Sep 1, 2026
ROCm#7360 added a module-level `import fcntl` to print_driver_gpu_info.py.
fcntl is a Unix-only module, so the Driver / GPU sanity check step now
crashes on every Windows CI runner with ModuleNotFoundError: No module
named 'fcntl', failing sanity and skipping all Windows component tests.

The KFD ioctl query that needs fcntl only runs on Linux (/dev/kfd), so
move the import into _get_kfd_version() where it is used. No behavior
change on Linux; Windows now runs the sanity check to completion.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
but does not fail.
"""

import fcntl

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import fails on Windows, and sanity checks failed on Windows on this PR's presubmit run: https://github.com/ROCm/TheRock/actions/runs/33499470761/job/99867709885.

Fixing in #7820

cc @amd-justchen , see https://github.com/ROCm/TheRock/blob/main/docs/rfcs/RFC0002-MonoRepo-Gardener-Rotations.md#responsibilities-and-priorities

Responsibilities and priorities

  1. When regressions occur that break CI workflows
    • Communicate that something is broken
    • Identify the culprit commit(s)
    • Notify the authors/reviewers of those commits
    • Revert those changes
    • If it's not obvious how to, facilitate getting changes in to get back to green (see 2)

The general sequence is "comment on the PR about the break" --> "revert" --> "let the author fix"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also add a unit test for this script that at least checks that it can be run on CPU runners without failing at import time (CPU runners won't have the tools, so those calls could be mocked out)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These shouldn't be running on CPU runners at all.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We missed this because the "Windows::release / Test gfx110X-all / Test Sanity Check / Test sanity (shard 1/1) (gfx110X-all)" job that failed ran 2 hours after the CI started and was not blocking. The "unit tests" job runs in < 5 minutes and is blocking, but it only runs on CPU (github-hosted) runners. A unit test that checked that the script could import on windows and linux would have caught this and blocked the merge.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, yeah that makes sense. I had the context wrong at first. Let me craft a PR for that.

amd-justchen added a commit that referenced this pull request Sep 2, 2026
…eck (#7820)

## Motivation

#7360 added a module-level `import fcntl` to
`build_tools/print_driver_gpu_info.py`. `fcntl` is a **Unix-only**
stdlib module,
so the `Driver / GPU sanity check` step (which runs this script) now
crashes at
import time on **every Windows CI runner**:

```
Traceback (most recent call last):
  File "...\build_tools\print_driver_gpu_info.py", line 23, in <module>
    import fcntl
ModuleNotFoundError: No module named 'fcntl'
```

That fails the Windows sanity job and skips every downstream Windows
component
test. The only use of `fcntl` is in `_get_kfd_version()`, which reads
the KFD
ioctl version from `/dev/kfd` and is only reached on the Linux path --
Windows
should never have imported the module.

ISSUE ID: N/A (regression from #7360)

## Technical Details

Move `import fcntl` out of module scope into `_get_kfd_version()`, its
sole user.
That function only runs on Linux, so the Unix-only import now happens
only where
it is valid. No functional change on Linux.

## Test Plan

- Import + run `print_driver_gpu_info.py` on Windows Server 2025, before
vs after.
- Confirm the Linux KFD path is unchanged (import still resolves inside
the
  function that uses it).

## Test Result

- Windows, **before**: `ModuleNotFoundError: No module named 'fcntl'` at
import.
- Windows, **after**: script imports and runs to completion (exit 0),
taking the
  `hipInfo.exe` path and skipping the Linux-only KFD block.
- The equivalent pre-#7360 sanity path (identical apart from this
import) already
passes `Driver / GPU sanity check` and runs the `origami` gfx1201
component test
  green on a Windows R9700 runner.

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lumachad added a commit that referenced this pull request Sep 3, 2026
## Summary

Adds a test that imports `print_driver_gpu_info` on any platform. This
catches top-level use of Unix-only stdlib modules (like `fcntl`) that
would crash a Windows job at import time. The test runs on CPU-only
runners and blocks a merge before any GPU job has a chance to fail.

Follows up on the discussion in #7360.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:run-all-archs Opt-in to building for all architectures on a pull request Not ready to Review PR has unresolved policy failures — reviews blocked

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants