Repository navigation
Test Filter Standardization POC - Changes to integrate rocBLAS - #3574
tony-davis wants to merge 26 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates the Test Filter Standardization POC from rocBLAS into TheRock by introducing a generic test runner that uses CTest with GPU-based filtering. The changes enable rocBLAS and MIOpen tests to use the new standardized filtering approach instead of their component-specific test scripts.
Changes:
- Introduced a new generic test runner (
test_filters.py) that supports multiple components with GPU architecture-based test filtering - Updated rocBLAS and MIOpen test configurations to use the new generic test runner
- Added
TEST_COMPONENTenvironment variable to the GitHub Actions workflow - Updated rocm-libraries subproject commit reference
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rocm-libraries | Updates subproject commit to include the Test Filter Standardization POC |
| build_tools/github_actions/test_executable_scripts/test_filters.py | Adds new generic test runner with CTest-based filtering by GPU architecture and test category |
| build_tools/github_actions/fetch_test_configurations.py | Updates rocBLAS and MIOpen to use the new generic test_filters.py script |
| .github/workflows/test_component.yml | Adds TEST_COMPONENT environment variable to pass job name to test scripts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Use AMDGPU_FAMILIES from environment variable, extract gfx<xxx> part | ||
| gpu_arch = "" | ||
| if AMDGPU_FAMILIES: |
There was a problem hiding this comment.
Where do we pass the AMDGPU_FAMILIES environment? I don't see it in your rocBLAS PR being explicitly set. I think we should explicitly set this variable so that we know it will run on the architecture that we will stop testing Math CI on
There was a problem hiding this comment.
Refer to PR #3595, which will be the implementing PR. This PR will ultimately be a one-liner after that one merges.
That line being:
https://github.com/ROCm/TheRock/pull/3574/changes#diff-e9b6090803ee2e3d395eeec96bba30b24e418907e6e8d03b065377af65d2df64R56
- test_component.yml: keep both TEST_COMPONENT (branch) and ROCM_KPACK_DEBUG (main) - rocm-libraries: take main's submodule commit 86247be Made-with: Cursor
There was a problem hiding this comment.
i think i mentioned this in miopen PR, but we should rename this to test_runner.py or something similar
test_filter is kind of confusing
There was a problem hiding this comment.
This file isn't really in this PR, it's in the implementing PR #3595. Once that PR merges, the rebase will make this PR really only one line (the change to use the filtering). This is branched from that PR, and depends on it merging first, which is why it's still in draft.
| import subprocess | ||
| import re | ||
| import os | ||
|
|
There was a problem hiding this comment.
This file isn't really in this PR, it's in the implementing PR #3595. Once that PR merges, the rebase will make this PR really only one line (the change to use the filtering). This is branched from that PR, and depends on it merging first, which is why it's still in draft.
| sys.exit(1) | ||
|
|
||
|
|
||
| def find_matching_gpu_arch(gpu_arch, available_gpu_archs): |
There was a problem hiding this comment.
this can go into github_actions_util!
There was a problem hiding this comment.
This file isn't really in this PR, it's in the implementing PR #3595. Once that PR merges, the rebase will make this PR really only one line (the change to use the filtering). This is branched from that PR, and depends on it merging first, which is why it's still in draft.
| [ | ||
| "--output-on-failure", | ||
| "--parallel", | ||
| "8", |
There was a problem hiding this comment.
can we make this variable? i know some components have specific parallelization fields
There was a problem hiding this comment.
This file isn't really in this PR, it's in the implementing PR #3595. Once that PR merges, the rebase will make this PR really only one line (the change to use the filtering). This is branched from that PR, and depends on it merging first, which is why it's still in draft.
| "-V", # Always run in verbose mode | ||
| # Shards the tests by running a specific set of tests based on starting test (shard_index) and stride (total_shards) | ||
| "--tests-information", | ||
| f"{SHARD_INDEX},,{TOTAL_SHARDS}", |
There was a problem hiding this comment.
can we add timeout option?
There was a problem hiding this comment.
This file isn't really in this PR, it's in the implementing PR #3595. Once that PR merges, the rebase will make this PR really only one line (the change to use the filtering). This is branched from that PR, and depends on it merging first, which is why it's still in draft.
DEPENDS ON #3595. THIS IS ULTIMATELY A ONE-LINE PR ONCE THAT PR IS MERGED.
Motivation
This PR is to integrate the Test Filter Standardization POC done in rocBLAS to TheRock
Relates to AIROCBLAS-45.
Technical Details
The POC in rocBLAS is done here: ROCm/rocm-libraries#4659.
This PR modifies test_rocblas.py to call the required test categories with CTest.
Dependencies
Based on #3595
Made with Cursor