Skip to content

fix(ci): enforce test_type_for_family strictly in fetch_test_configurations - #8828

Merged
geomin12 merged 1 commit into
mainfrom
users/geomin12/fix-mi455-force
Oct 8, 2026
Merged

geomin12 merged 1 commit into
mainfrom
users/geomin12/fix-mi455-force

Conversation

@geomin12

@geomin12 geomin12 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

The test_type_for_family flag (e.g., "quick" for MI455/gfx125x) was not being strictly honored by fetch_test_configurations.py. Now it overrides the TEST_TYPE env var regardless of what's passed, ensuring families with limited hardware always run the appropriate test type.

follow-up PR: fetch_test_configurations.py is abstracting a lot from configure_multi_arch_ci.py. I'm planning to combine them so configure does everything and we don't miss cases between

…ations

The test_type_for_family flag (e.g., "quick" for MI455/gfx125x) was not
being strictly honored by fetch_test_configurations.py. Now it overrides
the TEST_TYPE env var regardless of what's passed, ensuring families with
limited hardware always run the appropriate test type.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@geomin12 geomin12 added the ci:skip Skip all CI builds/tests for this PR label Oct 7, 2026
@therock-pr-bot

therock-pr-bot Bot commented Oct 7, 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 Oct 7, 2026
@therock-pr-bot

therock-pr-bot Bot commented Oct 7, 2026

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.

@dysu-amd

dysu-amd commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

From my understanding of the context around this:

The issue:

  • configure_multi_arch_ci.py only considers test_type_for_family when test_runs_on is nonempty
  • When GPU tests are disabled, eg. submodule bumps, test_runs_on is empty
  • Therefore test_type_for_family is not set when GPU tests are disabled, leading the issue that this PR addresses
    • In the above submodule PR run, the standard filter is selected instead
    • There's not yet been a nightly run for gfx1250 tests yet, but it's expected to correctly use the quick filter since test_runs_on should be nonempty

Instead of having fetch_test_configurations.py also need to check if a gfx has an override, could we just always have configure_multi_arch_ci.py set test_type_for_family regardless of whether or not test_runs_on is empty? Would that address the issue? The specific line in question: https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/configure_multi_arch_ci.py#L1596

follow-up PR: fetch_test_configurations.py is abstracting a lot from configure_multi_arch_ci.py. I'm planning to combine them so configure does everything and we don't miss cases between

100% agree :D

@geomin12

geomin12 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

From my understanding of the context around this:

The issue:

  • configure_multi_arch_ci.py only considers test_type_for_family when test_runs_on is nonempty

  • When GPU tests are disabled, eg. submodule bumps, test_runs_on is empty

  • Therefore test_type_for_family is not set when GPU tests are disabled, leading the issue that this PR addresses

    • In the above submodule PR run, the standard filter is selected instead
    • There's not yet been a nightly run for gfx1250 tests yet, but it's expected to correctly use the quick filter since test_runs_on should be nonempty

Instead of having fetch_test_configurations.py also need to check if a gfx has an override, could we just always have configure_multi_arch_ci.py set test_type_for_family regardless of whether or not test_runs_on is empty? Would that address the issue? The specific line in question: https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/configure_multi_arch_ci.py#L1596

follow-up PR: fetch_test_configurations.py is abstracting a lot from configure_multi_arch_ci.py. I'm planning to combine them so configure does everything and we don't miss cases between

100% agree :D

this is true! however, it requires a bit more effort which I will be doing in a follow-up PR

this helps resolve immediate testing needed for mi455

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

I suppose this is ok if a follow up is coming. Tbh I did not know of this script's existence until now, I'm far from familiar with all the many ci scripts and the interactions between them

@geomin12
geomin12 merged commit 05add19 into main Oct 8, 2026
27 of 39 checks passed
@geomin12
geomin12 deleted the users/geomin12/fix-mi455-force branch October 8, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:skip Skip all CI builds/tests for this PR Not ready to Review PR has unresolved policy failures — reviews blocked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants