Fix: stop packaging and the profiling-flag smoke on examples-only changes - #1607
Conversation
…nges PR hw-native-sys#1605 changed three files, all under `examples/workers/l3/` — two READMEs and one `main.py`. The category axis correctly skipped `ut`, `ut-a2a3` and `ut-a5`, but `packaging-matrix` (both legs, ~9 min each) and `profiling-flags-smoke` (~11 min) still ran. Neither could have been affected. Those two jobs are the only ones that build and install the product and then exercise it with a fixed, tiny payload rather than reading a test suite as a corpus: `packaging-matrix` runs `tools/verify_packaging.sh`, which checks the import surface and one entry-point script, and `profiling-flags-smoke` rebuilds the runtimes under six `CXX` define permutations and runs one `vector_example`. Nothing under `examples/` reaches either — it is absent from `wheel.packages` and from the sdist `include`, and neither job opens it. So they take a fourth flag, `examples_only`, written in the same shape as the arch and category axes with `NON_CODE` subtracted first. `profiling-flags-smoke` moves off `st_affected`, which was never the right question for it: its inputs are `src/` and the build system, not the scene-test corpus. `tests/` deliberately stays in. `verify_packaging.sh` runs `tests/st/a2a3/.../paged_attention_unroll/test_paged_attention_unroll.py --help` as its entry-point smoke, and `tests/` is in the sdist `include` (`pyproject.toml:64`, with `tests/st` excluded from the wheel only). Only `examples/` is provably absent from both jobs, so only `examples/` is subtracted. One consequence worth naming: a change confined to `examples/{arch}/.../vector_example/` now skips the profiling smoke. That is acceptable because `st-sim-*` and `st-onboard-*` run `vector_example` on both sim and hardware anyway; the smoke's unique coverage is the six build-flag permutations, which no example can affect. Simulated against nine file lists. Newly skipping: hw-native-sys#1605's diff, an `examples/a2a3` kernel, docs-only. Still running: `tests/st/`, `tests/ut/`, `src/common/`, `simpler_setup/build_runtimes.py`, `pyproject.toml`, and any diff mixing `examples/` with `src/`. `docs/ci.md` and `.claude/rules/ci-change-detection.md` move with it, including the paragraph that argued `packaging-matrix` was "not worth a fourth condition" — hw-native-sys#1605 is the counterexample. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
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>
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>
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>
…#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.
`packaging-matrix` ran on any diff that touched `tests/` — a change confined to `tests/ut/py/` booked two packaging jobs that build, install, and smoke the same wheel it did not touch. That was justified in the rules by two claims, and both were false: - "`tests/` is in the sdist `include`" — no sdist configuration exists in `pyproject.toml`; `wheel.packages = ["simpler_setup", "python/simpler"]` is the entire packaging surface. - "`tools/verify_packaging.sh` runs a `tests/st/` file as its entry-point smoke" — true, but it is one fixed file (`paged_attention_unroll/test_paged_attention_unroll.py --help`), and the smoke runs it from the source tree, not from the wheel; a changed payload file is exercised by the scene-test job that reads the same corpus. This completes the symmetry that hw-native-sys#1607 left half-finished: it removed `examples/` from both jobs' reach (profiling's payload lives there), but `tests/` stayed in with a reason that did not survive contact with the workflow. Both product jobs now take the same corpus axis on both sides: ```yaml TESTS_ONLY='^(tests/)' # same shape as EXAMPLES_ONLY='^examples/' packaging-matrix: !non_code_only && !examples_only && !tests_only profiling-flags-smoke: (a2a3_changed || a5_changed) && !examples_only && !tests_only ``` New shape is fail-open like every other axis (unrecognized paths land in `REMAINING` and flip the flag on), and the empty-diff guard sets `tests_only=false` alongside the other corpus flag. The gate table in `.claude/rules/ci-change-detection.md` and the corpus-axis section of `docs/ci.md` are updated with the same correction, and the stale sdist claim is gone from both. The vocabulary exists twice, and the lane was the other half of the defect: `ci-self-cpu.yml` runs a lane-local `detect-changes` whose header says the two must stay in sync, but its `packaging` gate was still bare `non_code_only != true` — it had never received `examples_only` (hw-native-sys#1607), let alone `tests_only` — and its `profiling-flags-smoke` still gated on `st_affected`. Both lanes' `detect-changes` now export the same corpus outputs, fail-open the same way, and gate the two product jobs identically. The rule's §4 now states the dual-vocabulary invariant explicitly, and §7's verification gains an outputs-consistency grep across both workflows. Verified by replaying the exact grep chain against representative file sets before changing anything (per the rule's §7): `tests/`-confined diffs — UT, ST, or mixed, payload file included — skip both jobs; `pyproject.toml`, `simpler_setup/`, `src/`, `conftest.py`, and `ci.yml` itself still run them. A change to the gates must run everything, so this PR books the full matrix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…1635) `packaging-matrix` ran on any diff that touched `tests/` — a change confined to `tests/ut/py/` booked two packaging jobs that build, install, and smoke the same wheel it did not touch. That was justified in the rules by two claims, and both were false: - "`tests/` is in the sdist `include`" — no sdist configuration exists in `pyproject.toml`; `wheel.packages = ["simpler_setup", "python/simpler"]` is the entire packaging surface. - "`tools/verify_packaging.sh` runs a `tests/st/` file as its entry-point smoke" — true, but it is one fixed file (`paged_attention_unroll/test_paged_attention_unroll.py --help`), and the smoke runs it from the source tree, not from the wheel; a changed payload file is exercised by the scene-test job that reads the same corpus. This completes the symmetry that #1607 left half-finished: it removed `examples/` from both jobs' reach (profiling's payload lives there), but `tests/` stayed in with a reason that did not survive contact with the workflow. Both product jobs now take the same corpus axis on both sides: ```yaml TESTS_ONLY='^(tests/)' # same shape as EXAMPLES_ONLY='^examples/' packaging-matrix: !non_code_only && !examples_only && !tests_only profiling-flags-smoke: (a2a3_changed || a5_changed) && !examples_only && !tests_only ``` New shape is fail-open like every other axis (unrecognized paths land in `REMAINING` and flip the flag on), and the empty-diff guard sets `tests_only=false` alongside the other corpus flag. The gate table in `.claude/rules/ci-change-detection.md` and the corpus-axis section of `docs/ci.md` are updated with the same correction, and the stale sdist claim is gone from both. The vocabulary exists twice, and the lane was the other half of the defect: `ci-self-cpu.yml` runs a lane-local `detect-changes` whose header says the two must stay in sync, but its `packaging` gate was still bare `non_code_only != true` — it had never received `examples_only` (#1607), let alone `tests_only` — and its `profiling-flags-smoke` still gated on `st_affected`. Both lanes' `detect-changes` now export the same corpus outputs, fail-open the same way, and gate the two product jobs identically. The rule's §4 now states the dual-vocabulary invariant explicitly, and §7's verification gains an outputs-consistency grep across both workflows. Verified by replaying the exact grep chain against representative file sets before changing anything (per the rule's §7): `tests/`-confined diffs — UT, ST, or mixed, payload file included — skip both jobs; `pyproject.toml`, `simpler_setup/`, `src/`, `conftest.py`, and `ci.yml` itself still run them. A change to the gates must run everything, so this PR books the full matrix.
The leak
#1605 changes three files, all under
examples/workers/l3/— two READMEs and onemain.py. What ran:Why those two are different from every other job
They are the only jobs that build and install the product and then exercise it with a fixed, tiny payload, rather than reading a test suite as a corpus:
packaging-matrixtools/verify_packaging.sh— import surface plus one entry-point scriptprofiling-flags-smokeCXXdefine permutations, runs onevector_exampleNothing under
examples/reaches either: it is absent fromwheel.packagesand from the sdistinclude, and neither job opens it.So they take a fourth flag,
examples_only, in the same shape as the arch and category axes withNON_CODEsubtracted first.profiling-flags-smokealso moves offst_affected, which was never the right question for it — its inputs aresrc/and the build system, not the scene-test corpus.What deliberately stays in
tests/still triggers packaging.verify_packaging.shrunsas its entry-point smoke, and
tests/is in the sdistinclude(pyproject.toml:64; onlytests/stis excluded, and only from the wheel). Justexamples/is provably absent from both jobs, so justexamples/is subtracted — the narrow cut, not the convenient one.One consequence worth naming: a change confined to
examples/{arch}/.../vector_example/now skips the profiling smoke. Acceptable —st-sim-*andst-onboard-*runvector_exampleon both sim and hardware anyway, and the smoke's unique coverage is the six build-flag permutations, which no example can affect.Resulting gates
st-sim-*,st-onboard-*<arch>_changed && st_affectedprofiling-flags-smoke(a2a3_changed || a5_changed) && !examples_onlyut,ut-a2a3,ut-a5non_code_only != true && ut_affectedpackaging-matrixnon_code_only != true && !examples_onlyTesting
Simulated the filter against nine file lists:
examples_onlymain.pyunderexamples/)examples/a2a3/.../kernels/aiv/x.cpptests/st/.../test_foo.pytests/ut/py/test_x.pysrc/common/log/host_log.cppsimpler_setup/build_runtimes.pypyproject.tomlexamples/mixed withsrc/a5/yaml.safe_loadonci.yml; everyif:re-read after rewiringmkdocs build --strict— exit 0;markdownlint-cli2cleandocs/ci.mdand.claude/rules/ci-change-detection.mdupdated, including the paragraph that arguedpackaging-matrixwas "not worth a fourth condition" — Docs: disambiguate "overlap" in the dual-domain L3 examples #1605 is the counterexample to my own call in Add: test-category axis so a suite-only change runs only its own suite #1590This PR touches
ci.yml, so all axes report affected and the full matrix fires here — the intended self-check.