Skip to content

fix(ci): guard Unix-only fcntl import in Windows driver/GPU sanity check - #7820

Merged
amd-justchen merged 1 commit into
ROCm:mainfrom
amd-justchen:justchen/fix-windows-sanity-fcntl-import
Sep 2, 2026
Merged

amd-justchen merged 1 commit into
ROCm:mainfrom
amd-justchen:justchen/fix-windows-sanity-fcntl-import

Conversation

@amd-justchen

Copy link
Copy Markdown
Contributor

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-ci: gate on KFD IOCTL version in driver/GPU sanity check #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

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>
@therock-pr-bot

therock-pr-bot Bot commented Sep 1, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: build_tools/print_driver_gpu_info.py; no test file found
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 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 commented Sep 1, 2026

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@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.

Would have preferred a revert instead of a fix-forward.

This looks correct though, so okay. (tested on my Windows machine too)

@amd-justchen

Copy link
Copy Markdown
Contributor Author

Validated end-to-end on a Windows gfx1201 (Radeon AI PRO R9700) runner by dispatching test_artifacts.yml with the harness checkout pointed at this commit (repository=amd-justchen/TheRock, ref=efff817):

https://github.com/ROCm/TheRock/actions/runs/33566391044 -> success

  • Before (at f3e9941, current main): the Driver / GPU sanity check step crashes at import with ModuleNotFoundError: No module named 'fcntl', failing sanity and skipping all Windows components.
  • After (this commit): the same step runs hipInfo.exe and passes:
HEAD is now at efff817 fix(ci): guard Unix-only fcntl import in driver/GPU sanity check
=== Sanity check: driver / GPU info ===
=== hipInfo.exe ===
Name:        AMD Radeon AI PRO R9700
gcnArchName: gfx1201

Sanity check passed and the origami component test passed (overall run green).

@amd-justchen

Copy link
Copy Markdown
Contributor Author

Would have preferred a revert instead of a fix-forward.

This looks correct though, so okay. (tested on my Windows machine too)

Fair enough! My agent happened to have discovered it while trying to get a new runner up. Seems like a reasonable fix.

@amd-justchen

Copy link
Copy Markdown
Contributor Author

https://github.com/ROCm/TheRock/actions/runs/33566263802/job/100067935852?pr=7820
Given comments about Windows testing passing and CI showing Linux GPU tests passing will merge this.

@amd-justchen
amd-justchen merged commit 648a501 into ROCm:main Sep 2, 2026
120 of 127 checks passed
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Sep 2, 2026
amd-justchen added a commit to ROCm/rocm-systems that referenced this pull request Sep 2, 2026
…#7820)

Re-point the pinned ROCm/TheRock ref from 1f1f06b to 648a501, which includes ROCm/TheRock#7820 (guard Unix-only fcntl import in the Windows driver/GPU sanity check) so the newer TheRock does not break the Windows test lane.
amd-justchen added a commit to ROCm/rocm-systems that referenced this pull request Sep 2, 2026
Bump the pinned `ROCm/TheRock` ref from `b45a060` (2026-08-26) to
`648a501` (2026-09-02) across the TheRock CI workflows.

Per Luis Machado's recommendation on #9738: rocm-systems' TheRock ref
needs bumping to something recent so those builds pass as they do on
TheRock `main`.

`648a501` is the current TheRock `main` tip and includes
[ROCm/TheRock#7820](ROCm/TheRock#7820) (guard
Unix-only `fcntl` import in the Windows driver/GPU sanity check), so
this newer ref does not break the Windows test lane.

Updates all 15 workflow files that share the develop TheRock pin (21
occurrences: checkout `ref:` lines and rccl `therock_ref` input
defaults). The intentionally separate multi-arch pin (`3c4ec01`) is left
unchanged.

Ref: #9738
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants