Add: test-category axis so a suite-only change runs only its own suite - #1590
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCI change detection now includes separate scene-test and unit-test affected flags. Workflow jobs use these flags alongside existing non-code and architecture gates, and the CI documentation describes the updated scheduling behavior. ChangesCI test-category gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChangedFiles
participant DetectChanges
participant CIJobs
ChangedFiles->>DetectChanges: changed paths
DetectChanges->>DetectChanges: compute st_affected and ut_affected
DetectChanges->>CIJobs: publish category outputs
CIJobs->>CIJobs: evaluate job conditions
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ChaoWao
force-pushed
the
ci/test-category-axis
branch
2 times, most recently
from
July 30, 2026 03:16
f637218 to
8889108
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 760-761: Update the workflow step that computes affected files to
capture git diff output separately while preserving its exit status, including
when the pull-request base or head SHA is unavailable or invalid. Use only git
diff’s status to select the empty FILES fallback, then always emit st_affected
and ut_affected outputs through the existing output-writing logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9cab668-9d2d-4227-a4d8-907674fb9e37
📒 Files selected for processing (3)
.claude/rules/ci-change-detection.md.github/workflows/ci.ymldocs/ci.md
ChaoWao
force-pushed
the
ci/test-category-axis
branch
from
July 30, 2026 11:36
8889108 to
5935746
Compare
`tests/st/` + `examples/` and `tests/ut/` execute disjoint trees — `pytest examples tests/st` versus `pytest tests/ut` plus the C++ ctest — and no unit test reads `examples/` or `tests/st/`. The only cross-references are a faked nodeid string in `tests/ut/py/test_resource_failure_summary.py` and a comment in `test_error_code_names.cpp`; neither opens a file. So a diff confined to one suite cannot break the other, and until now every such diff ran both. Over the last 60 commits on main, 8 were scene-test/example-only and 3 were unit-test-only — around 18% paying for a suite that could not have regressed, including time on the self-hosted a2a3 and a5 pools. `st_affected` and `ut_affected` are written in the same shape as the existing arch flags: a partition is unaffected only when EVERY changed file belongs exclusively to a sibling partition, with `NON_CODE` subtracted first. Copying the shape is what buys the two properties that matter. Unrecognised paths survive both filters and turn the flag on, so a new directory over-runs CI rather than silently skipping it. And shared test infrastructure — the root `conftest.py`, `pyproject.toml`, `simpler_setup/`, `tests/lint/` — belongs to no single partition, so it matches no sibling pattern and flips every flag true, without needing to be enumerated anywhere. The fail-open guard also learns that a *failed* `git diff` is the same as an empty one. `run:` is `bash -e`, so a non-zero diff — unresolved base/head SHA — aborted the step before the guard and emitted no outputs at all; a failed `detect-changes` then leaves every downstream `needs:` unsatisfied, which SKIPS the matrix rather than running it. Fail-closed, the opposite of the intent. Only the exit status decides now, and a failure folds into the empty case. The UT jobs stay off the arch axis. `docs/ci.md` principle 5 argues that unit tests cover shared contracts and a falsely-skipped regression outweighs the minutes saved, which is a judgement about silicon attribution and still holds. Test category is a different question, and the answer differs. `packaging-matrix` keeps `non_code_only` alone. Neither `tests/` nor `examples/` ships in the wheel, so it over-runs on a tests-only diff, but it is cheap and GitHub-hosted and a fourth condition buys little. Simulated against thirteen file lists before pushing. `tests/st/`-only and `examples/`-only now reach scene tests alone; `tests/ut/py/`-only and `tests/ut/cpp/a5/`-only reach unit tests alone; `src/common/`, the root `conftest.py`, `pyproject.toml`, `simpler_setup/`, a mixed st+ut diff, an empty diff and an unresolvable-SHA diff all still reach both. `docs/ci.md` and `.claude/rules/ci-change-detection.md` move with it — the rule now describes three layered axes, states the shared shape as the reason a fourth axis must copy it rather than invent one, and covers the failed-diff case under "fail open once, at the top". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 31, 2026
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 31, 2026
The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (hw-native-sys#1425, contained in hw-native-sys#1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. Fault injection therefore always runs on a device that has never provisioned — which is what actually addresses the interaction, rather than merely quarantining it. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until hw-native-sys#1425 is fixed**: in-session ordering separates the cases, but a fault on a device that has already provisioned still costs minutes, so they must not share a device. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in hw-native-sys#1601 and `detect-changes` reworked in hw-native-sys#1589 / hw-native-sys#1590 / hw-native-sys#1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 31, 2026
The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (hw-native-sys#1425, contained in hw-native-sys#1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. This is what will make merging the dedicated CI step back into the sweep safe once hw-native-sys#1425 is fixed; today `-m` already separates them, so it matters for local full runs. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. The conversion also moves it from the Resource phase to the L2 phase, which runs after it, so it provisions on a device the fault-injection cases have already finished with. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until hw-native-sys#1425 is fixed**: the two phases run their jobs in parallel across devices, so only separate device pools keep an SDMA provisioning away from a fault injection. `.claude/skills/testing/SKILL.md` moves with the mechanism throughout — not only the mirror-CI command, but the two instructions that told readers to extract `--ignore` sets from `ci.yml` and to grep it when a test passes alone and fails in the sweep. Both now name the marker; following the skill reproduces CI rather than a mechanism that no longer exists. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in hw-native-sys#1601 and `detect-changes` reworked in hw-native-sys#1589 / hw-native-sys#1590 / hw-native-sys#1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ChaoWao
added a commit
to ChaoWao/simpler-fork
that referenced
this pull request
Jul 31, 2026
The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (hw-native-sys#1425, contained in hw-native-sys#1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. This is what will make merging the dedicated CI step back into the sweep safe once hw-native-sys#1425 is fixed; today `-m` already separates them, so it matters for local full runs. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. The conversion also moves it from the Resource phase to the L2 phase, which runs after it, so it provisions on a device the fault-injection cases have already finished with. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until hw-native-sys#1425 is fixed**: the two phases run their jobs in parallel across devices, so only separate device pools keep an SDMA provisioning away from a fault injection. `.claude/skills/testing/SKILL.md` moves with the mechanism throughout — not only the mirror-CI command, but the two instructions that told readers to extract `--ignore` sets from `ci.yml` and to grep it when a test passes alone and fails in the sweep. Both now name the marker; following the skill reproduces CI rather than a mechanism that no longer exists. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in hw-native-sys#1601 and `detect-changes` reworked in hw-native-sys#1589 / hw-native-sys#1590 / hw-native-sys#1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ChaoWao
added a commit
that referenced
this pull request
Jul 31, 2026
…#1609) The SDMA quarantine was a pair of `--ignore=<path>` arguments. `pytest --ignore` on a path that no longer exists **exits 0 and says nothing** — measured — so moving either demo directory would have dropped the quarantine silently and landed both tests back beside the `aicore_op_timeout` fault-injection test. That collision is the whole hazard: provisioning the SDMA workspace creates 48 device-only STARS streams that sit in the device fault domain, so a later AICore fault on that device costs ~306 s instead of ~0.3 s (#1425, contained in #1406). `@pytest.mark.sdma` replaces the path matching, and carries the whole consequence rather than one arbitrary piece of it. One declaration now drives three things: - **The Worker is built with `enable_sdma=True`.** Both pytest construction sites in `conftest.py` and both standalone sites in `scene_test.py` read it, the latter from `cls.pytestmark` so `python test_x.py` behaves like the pytest path. - **The L2 Worker pool stops mixing capabilities.** The key gains the flag and reuse tests it, because an enable_sdma Worker holds its STARS streams for life: handing it to a test that did not ask for them would spread the teardown hazard to every later L2 case on that device. The existing same-device retire loop then performs the swap, so no new teardown path. - **SDMA sorts last.** `sort_key` gains a term, keyed off the marker rather than the class because the fault-injection tests are plain functions with no `_st_level`. This is what will make merging the dedicated CI step back into the sweep safe once #1425 is fixed; today `-m` already separates them, so it matters for local full runs. With the capability expressible, `prefetch_async_demo` becomes an ordinary L2 `@scene_test` class. It was a hand-rolled Worker only because `CASES` had no channel to `Worker.__init__`, and it paid for that by forfeiting golden comparison, case parametrization, `--rounds`, `--case` and the dispatcher's device allocation. 160 lines become 89, and its verification — `torch.equal(out, src)` — is now a one-line `compute_golden`. The framework's default orchestration includes turned out to be sufficient, so no new passthrough was needed there. The conversion also moves it from the Resource phase to the L2 phase, which runs after it, so it provisions on a device the fault-injection cases have already finished with. `sdma_async_completion_demo` keeps its hand-rolled L3 Worker for now and takes the marker for ordering and CI selection only; converting it means expressing a comm domain through `CASES`, which is a larger change and independent of this one. CI selects with `-m sdma` / `-m "not sdma"`, so the sweep and the dedicated step can no longer drift apart. **The dedicated step stays until #1425 is fixed**: the two phases run their jobs in parallel across devices, so only separate device pools keep an SDMA provisioning away from a fault injection. `.claude/skills/testing/SKILL.md` moves with the mechanism throughout — not only the mirror-CI command, but the two instructions that told readers to extract `--ignore` sets from `ci.yml` and to grep it when a test passes alone and fails in the sweep. Both now name the marker; following the skill reproduces CI rather than a mechanism that no longer exists. Separately, `docs/capability-survey.md` carried four `ci.yml:<line>` citations, three already invalidated by this session's edits — `:607` lands on a blank line, `:884` on a `cmake --build`, `:628-643` on a dep_gen comment, after the qwen step was deleted in #1601 and `detect-changes` reworked in #1589 / #1590 / #1607. Line numbers into a file this active cannot be maintained, so all four become greppable anchors. `grep -rIn 'ci\.yml:[0-9]'` is now empty repo-wide.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
tests/st/+examples/andtests/ut/execute disjoint trees:pytest examples tests/st ...pytest tests/ut ...+ctest --test-dir tests/ut/cpp/buildAnd no unit test reads
examples/ortests/st/. The only cross-references are a faked nodeid string intests/ut/py/test_resource_failure_summary.pyand a comment intest_error_code_names.cpp— neither opens a file.So a diff confined to one suite cannot break the other, and until now every such diff ran both. Measured on the last 60 commits to
main: 8 scene-test/example-only, 3 unit-test-only — about 18% paying for a suite that could not have regressed, including time on the self-hosted a2a3 and a5 pools.Shape
st_affected/ut_affectedcopy the existing arch pattern exactly — a partition is unaffected only when every changed file belongs exclusively to a sibling partition, withNON_CODEsubtracted first:Copying the shape is what buys the two properties that matter:
REMAINING, and turns the flag on. A new directory over-runs CI rather than silently skipping it.conftest.py,pyproject.toml,simpler_setup/,tests/lint/belong to no single partition, so they match no sibling pattern and flip every flag true. Nothing has to list them.Resulting gates
st-sim-*,st-onboard-*<arch>_changed && st_affectedprofiling-flags-smoke(a2a3_changed || a5_changed) && st_affected— it runspytest examples/{arch}/.../vector_example/, so it is st-familyut,ut-a2a3,ut-a5non_code_only != true && ut_affectedpackaging-matrixnon_code_only != true(unchanged)The UT jobs stay off the arch axis.
docs/ci.mdprinciple 5 argues that unit tests cover shared contracts and a falsely-skipped regression outweighs the minutes — that is a judgement about silicon attribution and it still holds. Test category is a different question and the answer differs.packaging-matrixkeepsnon_code_onlyalone. Neithertests/norexamples/ships in the wheel (wheel.packages = ["simpler_setup", "python/simpler"]), so it over-runs on a tests-only diff — cheap, GitHub-hosted, not worth a fourth condition. Stated indocs/ci.mdrather than left implicit.Testing
Simulated the full three-axis filter against eleven file lists:
st_affectedut_affectedtests/st/...onlyexamples/a2a3/...onlyexamples/a5/...onlytests/ut/py/...onlytests/ut/cpp/a5/...onlysrc/common/log/host_log.cppconftest.pypyproject.tomlsimpler_setup/scene_test.pymkdocs.ymlyaml.safe_loadonci.ymlparsesmkdocs build --strict— exit 0markdownlint-cli2— 0 errors ondocs/ci.mdand the ruleif:re-read after rewiring; the||inprofiling-flags-smokeis parenthesised so&&binds as intendedPer the rule's own §6, the check that matters is what ran: this PR touches
ci.yml, so all three axes report "affected" and the full matrix should fire here.