Skip to content

fix(rocke): scope installed compiler test policy - #14868

Merged
shumway merged 3 commits into
developfrom
users/jshumway/aick-2910-installed-ctest-environments
Nov 2, 2026
Merged

shumway merged 3 commits into
developfrom
users/jshumway/aick-2910-installed-ctest-environments

Conversation

@shumway

@shumway shumway commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

A strict installed-CTest environment test exposed that compiler-tool discovery and test enforcement policy were bundled together. ROCKE_REQUIRE_COMPILER_TOOLS=1, which is consumed by the MLA resource guards, was also being propagated to the unrelated platform pytest entry.

This PR separates those concepts, scopes enforcement to its consumer, strengthens the generated-manifest contracts for standalone and provider layouts, and documents the design for future developer and agent changes.

Risk Assessment

Risk level 3. This changes installed CTest environment generation used by provider CI, but it does not change kernel generation, dispatch, runtime behavior, or public APIs. The behavior is covered by exact generated-manifest tests for standalone and provider layouts, with and without ROCM_PATH.

Related

ISSUE ID : AICK-2910
Fixes #14875

Device / Architecture Coverage

This is architecture-independent host-side CMake and test wiring. No GPU or architecture-specific validation is required.

The generated policy currently injects compiler paths and enables mandatory MLA compiler checks on Linux when ROCM_PATH is available. Windows keeps its native contract and does not receive Unix-derived paths or an inferred requirement. Native Linux and Windows PR CI should validate their respective host behavior.

Testing Summary

  • Reproduced the original failure against origin/develop: the installed platform pytest environment unexpectedly contained ROCKE_REQUIRE_COMPILER_TOOLS=1.
  • Verified the corrected standalone and provider generated manifests.
  • Ran repository formatting, lint, and CMake checks through pre-commit.

Testing Checklist

  • Base regression reproduction — python -m pytest platform/tests/test_engine_linkage.py::test_installed_pytest_gets_distinct_artifact_environment_variables -q at origin/develop — Status: Failed as expected, 1 failed, due to the unexpected requirement variable
  • Patched installed-environment contracts — python -m pytest platform/tests/test_engine_linkage.py::test_installed_ctest_entries_get_scoped_environment_contracts -q — Status: Passed, 2 passed
  • Repository hooks — pre-commit run --from-ref develop --to-ref HEAD — Status: Passed
  • PR CI — GitHub PR checks — Status: Pending

Flags / Guardrails

No product feature flag is needed. The enforcement variable remains conditional on the installed compiler-tool environment and is now attached only to the CTest entry whose tests consume it.

An explicitly inherited ROCKE_REQUIRE_COMPILER_TOOLS=1 remains a supported caller override.

Adjacent Tests Considered

The complete platform/tests/test_engine_linkage.py file was attempted in isolation. Its changed environment-contract cases passed, while two unrelated pre-existing differential-runner cases failed because python_batch_worker was not importable in that isolated invocation. The focused regression test does not depend on that import path.

No byte-identity, golden, or GPU numeric lanes are required because emitted IR and kernel behavior are unchanged.

Technical Changes

  • Represent compiler-tool discovery and compiler-test enforcement as separate CMake environment lists.
  • Give rocke_pytest compiler discovery paths without MLA-specific enforcement policy.
  • Give rocke_library_mla_pytest both discovery paths and ROCKE_REQUIRE_COMPILER_TOOLS=1 when the compiler environment is active.
  • Validate exact, duplicate-free environments for both CTest entries in standalone and provider layouts.
  • Document installed CTest environment contracts, native Linux/Windows expectations, explicit caller overrides, and the focused validation command.
  • Add source-level breadcrumbs so future changes naturally find the contract test.

@therock-pr-bot

therock-pr-bot Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

⏳ Waiting on Required Checks

⏳ Required checks have not all concluded yet. This bot run is exiting without failing so those checks can report independently.

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⏳ Required checks are still running or have not started yet.
This PR is not ready for review until they complete.

📖 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 Nov 2, 2026 •

Copy link
Copy Markdown

⏳ Waiting for required checks to start or finish:

  • ⏳ pre-commit

@cgmillette

Copy link
Copy Markdown
Contributor

Automated develop-health notice (rocKE merge monitor)

Hi @shumway, this PR looks like the fix for #14875 ([rocke] test_engine_linkage.py::test_installed_pytest_gets_distinct_artifact_environment_variables fails on develop since #14626 (ROCKE_REQUIRE_COMPILER_TOOLS set for rocke_pytest)).

Please add "Fixes #14875" to the description, so the issue closes when this PR merges. The rocKE merge monitor has moved this PR to the top of its review and merge queues.

@cgmillette

Copy link
Copy Markdown
Contributor

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

PR #14868 — scope installed compiler test policy (shumway, AICK-2910) — 🟠 First-round: contract test not in CI

Prior item Status
Author checklist: develop repro, contract test, PR CI 🟢 Reviewed
• develop repro holds: one extra variable (ran)
• new test: 2 ✓ at head via the docs' command; 2 ✗ on develop (ran)
• PR CI box: still running
cgmillette (11-02) develop notice: "Fixes #14875" 🟢 ADDRESSED
• description now says "Fixes #14875" (read)
• fixes a develop failure no CI lane sees (ran)
AnaghaRaoAMD (11-02) APPROVED 🟢 APPROVED 5f34693a
• current head; no comments
Bots / CI 🟡 Running
• fails: Multi-Arch CI Summary — cancelled first run, superseded
• running: gfx94X provider tests (2 shards), Windows gfx110X build, PR bot; 44 ✓
• merge: no conflicts; blocked
Our checks 🟢 Clean
• merge vs b088ac5a: clean; build + ctest 34 ✓ / 1 skip; goldens 0 changed (ran)
• manifests, 2 layouts × 3 host cases: only rocke_pytest changes; docs hold (ran)
• tests 9 ✓ / 1 skip; rules 1–3 clean (ran)
Code quality 🟠 Should-fix
• discovery and policy split cleanly in CMake (read)
• exact contract test, but no CI lane runs it: N2 (read)
• a third-entry policy leak still passes: N2 (ran)
# Sev file:line Finding
N1 🟡 platform/CMakeLists.txt:529 Open #14329 still appends the removed _rocke_compiler_env
• impact: merged in either order, its tiling entry silently loses compiler paths (ran: combined tree)
• fix: whichever lands second appends _rocke_compiler_tool_environment
N2 🟠 platform/CMakeLists.txt:443, tests/test_engine_linkage.py:210 Contract guard runs in no CI lane and checks two entries
• impact: no lane reported #14875 (read); a third-entry leak passes too (ran)
• fix: build-tree CTest the superbuild runs (prototyped: 2 ✓); assert which entries carry the flag

Utility: 🟢 Positive — scopes the strict flag to its only consumer; pins both layouts.


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

First-round review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with develop, build + ctest, differential goldens, changed tests at head and on develop, generated CTest manifests (both layouts, three host cases), CI at head, code quality. Flags whether a closer look is needed; not an approval or rejection.

@tenpercent

Copy link
Copy Markdown
Contributor

approving to stop the bleed, proceed with caution

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.

🟢 Approval recommended

No active review issues block the separation of compiler-tool discovery from MLA-specific test enforcement.

No open issues 🎉

What changed in this PR

Separates compiler-tool discovery from test enforcement in rocKE’s installed CTest configuration, removing MLA-specific enforcement from platform pytest while preserving it for MLA checks.

Changes:

  • Split discovery and enforcement environments and scope each to its consumers.
  • Expand exact, duplicate-free environment checks across standalone and provider layouts, with and without ROCM_PATH.
  • Document native-host contracts, caller overrides, and focused validation.
File Description
dnn-providers/​hip-kernel-provider/​rocke/​TESTING.md Documents installed environment contracts and validation.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​tests/​test_engine_linkage.py Expands manifest assertions for both layouts and pytest entries.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​CMakeLists.txt Separates compiler paths from MLA-only enforcement.
dnn-providers/​hip-kernel-provider/​rocke/​platform/​AGENTS.md Adds environment-policy guidance and links to the contract documentation.

Review effort: Max

@shumway
shumway enabled auto-merge (squash) November 2, 2026 18:13
@cgmillette
cgmillette disabled auto-merge November 2, 2026 18:19
@cgmillette

Copy link
Copy Markdown
Contributor

Automated merge-queue status (rocKE merge monitor)

Hi @shumway, I turned auto-merge off on this PR for now. Before a rocKE PR lands, the rocKE merge monitor checks it on top of today's develop branch, and this version would not land cleanly:

  • our latest review of this exact version found problems to fix first, and they are still open

The open items from our review, each with its fix:

What to do: address the open items in our latest review (#14868 (comment)). After that, turn auto-merge back on; the monitor re-checks every push.

Nothing else changed: the monitor does not approve, reject or merge PRs. It only turned auto-merge off to keep develop green. This comment is updated in place.

@shumway
shumway enabled auto-merge (squash) November 2, 2026 18:20
@tenpercent

Copy link
Copy Markdown
Contributor

Nonblocking suggestion: enroll the contract test in host CI

dnn-providers/hip-kernel-provider/rocke/platform/tests/test_engine_linkage.py:121

The expanded contract test provides meaningful regression coverage, but remains excluded from installed pytest by platform/CMakeLists.txt:443 and is not registered with build-tree CTest. The current CI paths inspected therefore do not execute this guard.

Consider registering this focused source-only test with build-tree CTest, or invoking it in an existing host CI step. This is a follow-up coverage improvement, not a blocker: the PR documents its manual execution, and I verified that both layout cases pass at the current head and fail against unpatched code for the leaked strict flag.

Review: pi / gpt-6.1 sol / high — reviewed head 5f34693ae120.

@shumway
shumway merged commit 0e707b6 into develop Nov 2, 2026
145 of 153 checks passed
@shumway
shumway deleted the users/jshumway/aick-2910-installed-ctest-environments branch November 2, 2026 18:57
@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

5 participants