Fix: gate packaging and the profiling smoke off tests-only changes - #1635
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 reports ChangesCI corpus-axis gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChangeDetection
participant packaging-matrix
participant profiling-flags-smoke
ChangeDetection->>packaging-matrix: Pass examples_only and tests_only outputs
ChangeDetection->>profiling-flags-smoke: Pass examples_only and tests_only outputs
packaging-matrix-->>ChangeDetection: Skip corpus-only changes
profiling-flags-smoke-->>ChangeDetection: Skip corpus-only changes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.claude/rules/ci-change-detection.md:
- Around line 75-90: Update ci-self-cpu.yml’s detect-changes job to export
examples_only and tests_only outputs matching .github/workflows/ci.yml, then add
both gates to the packaging and profiling-flags-smoke job conditions so diffs
confined to examples/ or tests/ are skipped consistently with main CI.
In @.github/workflows/ci.yml:
- Around line 842-850: Update the CI path-filter logic that sets tests_only and
controls packaging-matrix and profiling-flags-smoke so changes to the fixed
smoke payloads under
tests/st/a2a3/tensormap_and_ringbuffer/paged_attention_unroll and the relevant
profiling SIMPLER_* payload paths do not skip both smoke jobs. Add explicit
exceptions while preserving the existing behavior for unrelated tests-only
changes.
In `@docs/ci.md`:
- Around line 136-139: The earlier detect-changes output summary in docs/ci.md
lists only examples_only; update it to include tests_only as well. Ensure the
summary presents both corpus-axis outputs consistently with the documented
detect-changes contract, without changing the surrounding behavior or
explanations.
🪄 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: c13eeb80-48f8-409f-b5a8-43e931016699
📒 Files selected for processing (3)
.claude/rules/ci-change-detection.md.github/workflows/ci.ymldocs/ci.md
ChaoWao
force-pushed
the
ci/tests-only-axis
branch
from
August 2, 2026 08:17
878cc54 to
5c5c786
Compare
`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>
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.
Summary
packaging-matrixran on any diff that touchedtests/— a change confined totests/ut/py/(e.g. #1634) booked two packaging jobs that build, install, and smoke the same wheel it did not touch. The rules justified this with two claims; both were false:tests/is in the sdistinclude" — no sdist configuration exists inpyproject.toml. The entire packaging surface iswheel.packages = ["simpler_setup", "python/simpler"];tests/is not in the wheel at all.verify_packaging.shruns atests/st/file as its entry-point smoke" — true, but it is one fixed file (paged_attention_unroll/test_paged_attention_unroll.py --help, run 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 #1607 left half-finished: it removed
examples/from both product jobs' reach (profiling's payload lives there), buttests/stayed in with a reason that did not survive contact with the workflow.The change
New axis is fail-open like every other (unrecognized paths flip it on), and the empty-diff guard sets
tests_only=falsealongside the other corpus flag. Rules +docs/ci.mdupdated with the correction; the stale sdist claim is deleted from both.Testing
tests/ut/...,tests/st/..., mixed, and the payload file itself → both jobs skip (UT/ST jobs still run as before)pyproject.toml,simpler_setup/,src/, rootconftest.py,.github/workflows/ci.yml→ both run.gitignore-only → both skipdetect-changesoutputs consumedci.yml, which is deliberately outsideNON_CODE— the full matrix is expected and correct