Repository navigation
Conversation
Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
he-yufeng
left a comment
There was a problem hiding this comment.
Read all twelve files against the trigger semantics and the repo's tag history. The direction is right and the gap is real: before this, v0.3.14rc1/v0.3.14a1 matched v* and passed the !contains('-') guard, so they would have taken the production PyPI path while pre-release.yaml only watched hyphenated tags.
The part I wanted to verify before saying anything: v[0-9]+.[0-9]+.[0-9]+ looks like a regex planted in a glob field, but GitHub's filter syntax does support + as "one or more of the preceding character" and [] classes, so the whitelist is valid as written, and filters are full-name matches so v0.3.14.rc1 correctly falls out of the stable set. I also checked the tag history: every stable tag ever pushed fits the whitelist (vX.Y.Z and .postN only), so nothing historical would have been orphaned.
The test's github_filter_to_regex mirrors the documented semantics (* to [^/]*, ** to .*, +? passed through as quantifiers, classes verbatim, fullmatch anchor), which is the right reference model for tag patterns; both directions are asserted. One gap to be aware of if the file evolves: ! negation is not modeled (a negated pattern would be translated as a literal !), fine today since no tag pattern uses it.
Two notes, neither blocking:
-
After this lands, PEP 440 short forms (
a1/b1/dev1/+local) and typo'd stable tags likev0.3.14post1match no workflow at all. That flips the failure mode from publishing the wrong thing to silently skipping, which is the correct trade, and the regex-validated dispatch backfills in MUSA/ROCm/NPU are the recovery path. Worth one sentence in RELEASING.md so the next person who tagsv0.3.14post1knows why nothing ran instead of re-tagging blindly. The six workflows without a dispatch backfill have no recovery other than re-tagging; acceptable, but it makes that doc line more valuable. -
Heads-up on ordering with #4574: it also rewrites the release-npu.yaml tag classification (its
normalize-versiongate on the dispatch path), and this PR rewrites the same file's push trigger plus adds a Validate step. The two rules are consistent in outcome (hyphenated rc tags go to pre-release/TestPyPI, clean tags go to PyPI), but whichever lands second will need a rebase and a quick re-read of the combined file. May be worth sequencing them deliberately rather than letting git sort it out.
Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
|
Thanks for checking the tag history. There is no RELEASING.md in the repo, so I put the note in the trigger comments of release.yaml and pre-release.yaml instead (b924f18): tag forms like v0.3.14a1 or v0.3.14post1 match no workflow and need a re-tag. On #4574, agreed. Happy to rebase this one and re-read release-npu.yaml if that lands first. The red CTest job is not from this PR. Main itself is failing to build conductor_py.cpp after #4588 renamed npu to device while #4469 still uses hit.npu. |
koriyoshi2041
left a comment
There was a problem hiding this comment.
The tag-routing fix is sound, and the focused filter suite passes at b924f189. One release-integrity gap from #4584 remains: a valid stable tag is still not checked against the wheel metadata before publication.
At this head, mooncake-wheel/pyproject.toml declares 0.3.13. A push of v0.3.14 matches every new stable filter. _build-wheel.yaml sets VERSION=0.3.14, but the only step that writes that value back to pyproject.toml is guarded by inputs.version-override != ''; the normal tag-triggered workflows do not pass an override. scripts/build_wheel.sh then builds the unchanged 0.3.13 package, and _publish-wheel.yaml uploads it. The new test exercises routing only, so this mismatch remains green.
Could the stable path either fail before building when github.ref_name (normalized without v) differs from [project].version, or deliberately stamp the tag version and verify the built wheel metadata before upload? A negative test with a valid-but-mismatched tag would keep this distinct from the prerelease-classification coverage.
…ffer A stable tag such as v0.3.14 pushed while mooncake-wheel/pyproject.toml still says 0.3.13 built and published 0.3.13. Every stable build path now checks the tag against pyproject.toml before building: the shared _build-wheel (tag builds without a version override) and _build-efa-wheel (release profile) workflows, plus the NPU, MUSA and ROCm build jobs, including their workflow_dispatch backfills. The test runs each workflow's check step against a matching and a mismatched pyproject.toml.
|
@koriyoshi2041 agreed, went with failing before the build (6392802). Each stable build path now compares the tag (minus the |
koriyoshi2041
left a comment
There was a problem hiding this comment.
Verified the requested follow-up at exact head 63928026.
The version guard now covers all nine stable publication paths: the six reusable-wheel callers are mapped to _build-wheel.yaml / _build-efa-wheel.yaml, while MUSA, NPU, and ROCm carry the check in their build jobs. I independently ran python3 -m unittest scripts.ci.tests.test_release_tag_filters -v: all 5 tests pass, including matching final/post-release tags and stale pyproject.toml negatives. git diff --check origin/main...HEAD also passes. This closes the release-integrity gap from my earlier review: a valid stable tag can no longer silently publish wheel metadata from a stale project version.
One correction to the CI diagnosis, non-blocking for this patch: in run 38933536186 the CTest job does not reach scenario.cpp. Its first compilation (crc_checksum.cpp.o) fails because sccache cannot start after the Actions cache endpoint returns HTTP 503 ServerBusy / Egress is over the account limit. The CI Gate failure is downstream of that infrastructure error, not a source failure from this PR.
|
right, I misread that run. CTest in 38933536186 stops at the first compile, |
Issue #4584: rc tags without a hyphen were published to PyPI as stable
1. Root cause
Every stable release workflow (
release*.yaml) triggered ontags: ['v*'].The only thing keeping pre-releases out was the job guard
!contains(github.ref_name, '-'), which works for semver-style tags(
v0.3.14-rc1) but not for PEP 440 tags such asv0.3.14rc1. Those have nohyphen, so they went through the stable build-and-publish path to production
PyPI. Meanwhile
pre-release.yamlonly matched hyphenated tags (v*-rc*,etc.), so a
v0.3.14rc1tag never reached the TestPyPI pre-release pipeline.The manual
workflow_dispatchbackfill checks inrelease-musa.yaml,release-rocm.yamlandrelease-npu.yamlused the same "no hyphen" test, sothey had the same gap.
2. Fix
patterns:
v[0-9]+.[0-9]+.[0-9]+andv[0-9]+.[0-9]+.[0-9]+.post[0-9]+. Postreleases are still allowed because the project ships them (for example
0.3.12.post1). Any tag with a pre-release, dev or local suffix no longer
starts these workflows. I left the existing
!contains(..., '-')guards inplace; they are now redundant but do no harm.
^v[0-9]+\.[0-9]+\.[0-9]+(\.post[0-9]+)?$instead of the hyphen test.if:expression can't run a regex. Iadded a
Validate release tagstep to its build job using the same regex.publish-releasedepends onbuild, so a rejected tag also blocks thepublish.
pre-release.yamltag filters are relaxed fromv*-rc*/v*-alpha*/v*-beta*/v*-pre*tov*rc*/v*alpha*/v*beta*/v*pre*. Thenew patterns cover everything the old ones did, and
v0.3.14rc1now goesto the TestPyPI flow.
testpypi_wheel_gate.py normalize-versionalreadyaccepts the hyphen-free forms (checked:
v0.3.14rc1->0.3.14rc1).I chose a positive allowlist for stable tags over adding more
contains()checks to the guards. This keeps the stable path from depending on a list of
every possible pre-release spelling.
3. Files changed
.github/workflows/release.yaml,release-cuda13.yaml,release-efa.yaml,release-efa-cuda13.yaml,release-efa-non-cuda.yaml,release-non-cuda.yaml: tag filter..github/workflows/release-musa.yaml,release-rocm.yaml: tag filter anddispatch validation regex.
.github/workflows/release-npu.yaml: tag filter and a newValidate release tagstep..github/workflows/pre-release.yaml: broader pre-release tag filters.scripts/ci/tests/test_release_tag_filters.py(new): turns each workflow'sGitHub tag filters into regexes and checks which tags match.
4. Risk / uncertainty
vX.Y.ZandvX.Y.Z.postN. Any otherstable tag shape, such as a four-part
v1.2.3.4or.postNwritten anotherway, would no longer trigger a release. I found no such tags in docs or
history, but this shallow clone has no tags, so I couldn't list the
upstream tag history to confirm.
v0.3.14a1/v0.3.14b1/.dev1are nowblocked from stable release, but they do not trigger
pre-release.yamleither. That only matters if the project starts using those spellings.
(
*,**,+,?,[...]). It is not a full reimplementation ofGitHub's matcher.
5. Verification
python3 -m unittest scripts/ci/tests/test_release_tag_filters.py: passeswith the change. Against the original workflows (stashed) it fails, with
the rc tags matching the stable workflows and missing pre-release.
python3 -m unittest discover -s scripts/ci/tests: all 20 tests pass.This is the same command CI runs in
ci.yml.v0.3.14andv0.3.12.post1are accepted;v0.3.14rc1andv0.3.14-rc1are rejected.ruff format --checkandruff checkpass on the new test.pre-commitis not installed here, so Idid not run it.