Skip to content

fix(rocke): skip GDN/KDA tests on Windows CI - #14710

Merged
AviralGoelAMD merged 1 commit into
developfrom
users/avirgoel/rocke/win-skip-gdn-tests
Oct 30, 2026
Merged

AviralGoelAMD merged 1 commit into
developfrom
users/avirgoel/rocke/win-skip-gdn-tests

Conversation

@AviralGoelAMD

@AviralGoelAMD AviralGoelAMD commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

ISSUE ID : AICK-2860

What was wrong

The Windows hipkernelprovider CI shard runs close to its time limit. One GDN case, test_lowers_and_compiles_without_scratch in library/tests/test_gdn_decode_fused.py, runs close to its 60 s per-test limit on Windows runners. Normal runner variance can push it over.

What changed

Area / files Crux of change
rocke/platform/CMakeLists.txt On Windows, do not register the host entry rocke_library_gdn_pytest. A comment names the slow case and when to re-enable.
ROCKE_ENGINE_test_categories_external.yaml A comment next to each listed rocke_library_gdn_pytest says it is not registered on Windows.
rocke/TESTING.md Known gap G6 now states the Windows gap.
  • The configure log prints a line saying the entry was skipped on Windows.
  • Other platforms register the same tests as before.
  • The GDN/KDA numeric files in rocke_library_gpu_pytest are unchanged. They need gfx950/gfx942 devices, and the Windows GPU lanes use other GPUs, so those files already skip at collection and cost nothing there.

Why it works

CTest entries are generated at configure time. Under if(WIN32) the GDN host entry is never written, so the Windows shard does not run the slow case. No test code changes.

How we validated

Gate Result
Registration probe Ran the library-test registration region of platform/CMakeLists.txt through cmake -P, once with WIN32 unset and once with WIN32 set. Compared origin/develop against this head.
Non-Windows Output byte-identical to develop.
Windows Exactly one entry fewer: rocke_library_gdn_pytest. rocke_library_gpu_pytest is identical to develop.
Label parser Ran develop's shared/ctest/parse_ctest_categories.py with the YAML and the Windows entry list: exit 0.
Lint and tests pre-commit on the changed files passes. platform/tests/test_gpu_references.py passes.

Notes

  • The parser silently ignores a YAML name that has no registered test (verified). Nothing breaks, but on Windows the entry just disappears from reports. That is why the YAML and TESTING.md now say so.
  • No Windows run exists. Only the registration logic was simulated.
  • The whole host entry is disabled, although only one file holds the slow case. A narrower option is to drop only test_gdn_decode_fused.py on Windows, if keeping the other GDN host coverage matters more.
  • The shard job limit or shard count is a separate follow-up outside rocKE.

@AviralGoelAMD
AviralGoelAMD marked this pull request as ready for review October 29, 2026 22:41
@AviralGoelAMD
AviralGoelAMD requested a review from a team as a code owner October 29, 2026 22:41
@therock-pr-bot

therock-pr-bot Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🔎 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

Copy link
Copy Markdown

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

@AviralGoelAMD
AviralGoelAMD force-pushed the users/avirgoel/rocke/win-skip-gdn-tests branch from 55fe799 to 1788fd2 Compare October 29, 2026 22:52
@AviralGoelAMD
AviralGoelAMD requested review from a team as code owners October 29, 2026 22:52
The Windows CI shard for the hip-kernel provider runs close to its time
limit. One GDN case, test_lowers_and_compiles_without_scratch in
test_gdn_decode_fused.py, runs close to the 60 s per-test limit on
Windows runners.

Do not register rocke_library_gdn_pytest when building on Windows.
Registration on other platforms is unchanged. Re-enable it on Windows
once that case is cheaper or split out. The GDN/KDA numeric files in
rocke_library_gpu_pytest are unchanged: they need gfx950/gfx942 devices
and already skip at collection on the Windows GPU lanes.

Note the Windows gap next to the entry in the external categories YAML
and in TESTING.md known gap G6.

Copilot AI 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.

🔵 Needs a closer look

No Windows configure/install/CTest evidence verifies that removing the entire GDN/KDA host entry preserves the intended remaining GPU test collection, so human validation is needed.

No open issues 🎉

What changed in this PR

This pull request prevents the slow rocKE GDN/KDA host test entry from being registered on Windows, reducing CI timeout risk while preserving registration on other platforms and documenting the resulting coverage gap.

Changes:

  • Conditionally skips rocke_library_gdn_pytest during Windows CMake configuration.
  • Documents the Windows-specific omission in the CTest category configuration and testing documentation.
  • Leaves GPU numeric test registration unchanged.
File Description
dnn-providers/​hip-kernel-provider/​rocke/​TESTING.md Documents the Windows GDN/KDA host-test coverage gap and retained GPU coverage.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​CMakeLists.txt Adds the Windows-specific omission of the GDN/KDA host CTest entry and configure-time status message.
dnn-providers/​hip-kernel-provider/​ROCKE_ENGINE_test_categories_external.yaml Explains why the GDN host entry is absent from Windows CTest output.

Review effort: Lite

@AviralGoelAMD
AviralGoelAMD enabled auto-merge (squash) October 30, 2026 00:53
@cgmillette

Copy link
Copy Markdown
Contributor

Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max

PR #14710 — skip GDN/KDA tests on Windows CI (AviralGoelAMD, AICK-2860) — 🟡 First-round: no closer look needed — the Windows-only skip is correct and costs little coverage, but it names the wrong slow test (N1); no Windows shard has run at this head yet

Prior item Status
AviralGoelAMD (body, 10-29): registration probe (Linux byte-identical to develop; Windows one entry fewer; rocke_library_gpu_pytest identical); label parser exits 0; narrower option "drop only test_gdn_decode_fused.py" 🟡 Partly. The probe and parser results reproduce exactly (ran; see Our checks). The narrower option would not stop the recorded timeouts (N1)
Copilot (10-29): no Windows configure/install/CTest evidence that dropping the whole GDN/KDA host entry keeps the intended GPU test collection 🟡 OPEN until Windows CI runs. The registration logic is right and the GPU entry is unchanged (ran: our probe). No Windows configure or CTest list exists at this head yet: the gfx110X build stage is still running. Multi-Arch CI Summary is a required check, so auto-merge waits for that run. Its CTest list should lack only rocke_library_gdn_pytest
AnaghaRaoAMD (10-29) APPROVED at 6f8077db (head) 🟢 At head (single commit), given before any Windows shard ran. GitHub still reports REVIEW_REQUIRED (a code-owner review)
Bots / CI 🟡
• Policy bot pass (10-29). At head 6f8077db, pre-commit, clang-tidy, Component CI, the hipDNN superbuild (Linux and Windows), the Multi-Arch ASAN run and base freshness pass.
• The red Multi-Arch CI Summary is not this PR's. It belongs to the first Multi-Arch run at this head, which was cancelled at 23:10Z when a second run started. Its summary failed only because its Linux and Windows build-and-test jobs were cancelled. No math-libs stage failed (read: summary log).
• Second run: Linux gfx94X hipkernelprovider passes on both shards (49/49 each), and rocke_library_gdn_pytest still runs and passes there. The Windows gfx110X math-libs stage has been building since 23:39Z, so no Windows test shard has run at this head (read: CI logs).
• Today the Windows gfx110X hipkernelprovider shards of #14379 and #14494 ran out their 30-minute step limit, so this PR's Windows run may need a re-run whatever it changes (read: CI logs)
Our checks 🟢
• Merge (ran): clean with develop 587f9942 (7 behind, 1 ahead; merge dacbbfc5). The three changed files are identical on the merge, and develop's seven newer commits touch no GDN/KDA file. Native build OK, ctest 33/33 (1 skip). Differential golden OK at llvm20/22/23/24 and canonical (0 changed, 0 missing). Rules 1–3 clean; no links or tracker IDs added.
• Registration probe (ran): we ran the installed-CTest region of platform/CMakeLists.txt from develop, head and the merge through cmake -P, with WIN32 unset and set. Without WIN32, head and merge match develop byte for byte. With it, only rocke_library_gdn_pytest (its add_test and properties) is gone and the configure message prints. rocke_library_gpu_pytest, with its seven GDN/KDA numeric files, is unchanged.
• Label parser (ran): develop's shared/ctest/parse_ctest_categories.py with this YAML exits 0 on all four entry lists, and on Windows the only label change is the missing GDN line. As the YAML comment says, both parser paths skip a listed name that has no registered test (:241, :147). G6's other claims hold. rocke_golden_static keeps the KDA chunkwise IR hashes; the representative golden has no GDN decode entries. The GDN/KDA numeric files skip on the Windows GPU lane (read: #14379's gfx110X log)
# Sev file:line Finding
N1 🟡 platform/CMakeLists.txt:746, :769; TESTING.md:753 Problem: the comment, the configure message, G6 and the body name only test_gdn_decode_fused.py::test_lowers_and_compiles_without_scratch as the slow case. Both compile-all tests time out on Windows, and most recorded timeouts are in the other file. test_gdn_decode_spec.py::TestEmission::test_registered_tiles_compile compiles every registered tile through COMGR, and it timed out on develop (#14633) and on #14379 today. The fused test timed out once, on #13965 (read: CI logs, #14633).
Consequence: the body's narrower option (drop only the fused file) would not have stopped either spec-file failure, and neither would a higher limit on that one test. Re-enabling "once that case is cheaper" would bring them back. Dropping the whole entry is the better stopgap, and Windows loses little. Wherever CI runs these compile-all tests, every spill check skips for lack of llvm-objdump: 296 skips in this head's gfx94X log, and the Windows runner has no llvm-objdump either. So on Windows they show only that each tile compiles, which Linux shows too. The other GDN files have no subprocess or platform-specific code and keep running on Linux.
Fix: name both compile-all tests in the comment, the configure message and G6, and drop the narrower option from the body. In the PR body, reference #14633 with Refs, not Fixes. A skip meets neither of the issue's criteria (the lane passes on Windows with margin; coverage unchanged), so the issue stays open for the real fix

Utility: 🟢 Positive — Windows stopgap for recurring GDN-lane timeouts; little coverage lost; cause misnamed.


Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted

First-round review: a local qwen3.6 model made the independent first pass, and the orchestrator (Claude Opus 5.5) verified it. On a clean merge with develop, it ran the native build and ctest and both differential golden gates. It ran the CTest registration region of develop, this head and the merge through cmake -P with WIN32 unset and set, and passed each entry list to the shared label parser. It read both Linux gfx94X hipkernelprovider logs at this head, the first run's summary log, and today's Windows gfx110X logs from #14379 and #14494. It also traced why the GDN compile-all tests skip their spill checks. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection.

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

LGTM. Please plan to revert when #14633 is solved.

@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants