cleanup perf-changelog#1161
Conversation
|
Thanks for the contribution! For vLLM & SGLang, please ensure that your recipes is similar to the official vLLM recipes and/or the SGLang cookbook If it is not, please create a PR first before we can merge your PR into the master branch. Let's ensure that the documentation is first class such that the entire ML community can benefit from your hard work! Thank you PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. If re-running failed jobs is attempted, PR authors are responsible for ensuring it passes. See GitHub's docs on re-running failed jobs: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/re-run-workflows-and-jobs#re-running-failed-jobs-in-a-workflow As a rule of thumb, generally, PR authors should request a review & get a PR approval from the respective companies' CODEOWNERS before requesting a review from core maintainers. If additional help is needed, PR authors can reach out to core maintainers over Slack. |
| - config-keys: | ||
| - 70b-fp8-*-vllm | ||
| description: |
There was a problem hiding this comment.
🔴 Reordering perf-changelog.yaml deletes 771 non-whitespace lines, but utils/process_changelog.py:17-41 raises ValueError on any non-whitespace deletion to this file, and .github/workflows/run-sweep.yml invokes that check on every push to main whose head-commit message lacks [skip-sweep]. The PR title cleanup perf-changelog carries no such marker, so once squash-merged the post-merge run-sweep setup job will fail at the very first deleted entry. To merge safely, ensure the merge-commit message contains [skip-sweep], or relax process_changelog.py to permit reorder-only diffs. (PR-mode setup is gated on sweep-enabled/full-sweep-enabled labels, which is why this is invisible in the PR's own CI.)
Extended reasoning...
What the bug is
utils/process_changelog.py enforces an append-only invariant on perf-changelog.yaml. In get_added_lines (lines 17-41), it walks the unified diff line-by-line and unconditionally raises:
if line.startswith("-") and not line.startswith("---"):
deleted_content = line[1:]
if deleted_content.strip():
raise ValueError(
f"Deletions are not allowed in {filepath}. "
f"Only additions to the changelog are permitted. "
f"Found deleted line: {deleted_content}"
)This same rule is documented in AGENTS.md:160-161: "The file is read in chronological order: oldest at the top, newest at the bottom. New entries MUST be appended to the END of the file — never insert in the middle or prepend."
Why this PR trips it
This PR is described as a reorder ("Sorted perf-changelog.yaml from lowest PR number at the top to highest PR number at the bottom") plus 3 entry removals. The diff is 770 insertions / 771 deletions — every reordered entry shows up as both a deletion and a re-insertion. The very first hunk alone deletes lines 1-77 (the original dsv4-fp4-b200-sglang block, the dsr1-fp8-h100-dynamo-trt/...sglang block, etc.). All of these are non-whitespace deletions.
How run-sweep gets invoked on merge
.github/workflows/run-sweep.yml is triggered by push to main on paths: perf-changelog.yaml. Its setup job has two enabling conditions:
- PR mode, gated on the
sweep-enabled/full-sweep-enabledlabels — this is why nothing fires in the PR's own CI. - Push-to-main mode, gated on
github.event_name != 'pull_request' && !contains(github.event.head_commit.message, '[skip-sweep]').
The push-mode setup step runs python utils/process_changelog.py with BASE_REF=github.event.before and HEAD_REF=github.event.after, i.e. it diffs the merge commit against the pre-merge main. That diff contains all 771 deletions.
Why nothing in the PR catches it
- The PR description cites
pytest utils/matrix_logic/test_validation.py(61 passed) — that test only exercises the PydanticChangelogEntryschema; it does not invokeprocess_changelog.py's deletion gate. (And in this snapshot of the repo it can't even be collected —pydanticis not installed.) - The PR-mode setup needs a sweep label, which this PR doesn't carry.
- So the failure only materializes after merge, on the push-to-main run.
Step-by-step proof of the failure
- Maintainer squash-merges PR cleanup perf-changelog #1161 with the default title
cleanup perf-changelog(no[skip-sweep]marker). - GitHub fires
pushonmainwithpaths: perf-changelog.yamlmatched. run-sweep.yml'ssetupjob evaluates itsif: not a PR ✓, head-commit message lacks[skip-sweep]✓ → job runs.- Setup step exports
BASE_REF=$\{\{ github.event.before \}\},HEAD_REF=$\{\{ github.event.after \}\}and execspython utils/process_changelog.py --base-ref $BASE_REF --head-ref $HEAD_REF. get_added_lineswalks the unified diff. The first non-whitespace-line it sees (e.g.-- config-keys:from the deleted top-of-filedsv4-fp4-b200-sglangblock) hits theraise ValueError(...)at line 33.- The script exits non-zero. The setup job fails. Run-sweep is broken on
main.
How to fix
Three options, in roughly increasing invasiveness:
- Easiest — when squash-merging, edit the merge-commit message to include
[skip-sweep]. This is the established escape hatch (see commit13d527afor prior art). - Loosen
process_changelog.pyto detect reorder-only diffs (e.g. compare set/multiset of stripped non-whitespace lines pre- and post-merge, allow the diff if they match modulo the 3 intentional removals). - Split the cleanup: append-only
pr-linkcorrections in one PR, and a separate explicit[skip-sweep]PR for the reorder.
Worth flagging to the merger so option (1) isn't accidentally skipped.
Clean and sort
perf-changelog.yamlPR linksDescription
Clean up
perf-changelog.yamlso entries are ordered by PR number and check correct PR numbers (XXX)Summary
perf-changelog.yamlfrom lowest PR number at the top to highest PR number at the bottom.pr-linkmetadata.minimaxm2.5-fp8-mi355x-vllmentry from closed PR#1002pr-link210to207.pr-linkmetadata, and whitespace cleanup.Steps Performed
perf-changelog.yamlinto top-level changelog entries using^- config-keys:as the entry boundary.pr-linkstatus:pr-linkTBD,XXX, andXXXXgit blame,git log, and GitHub PR metadata viaghto map entries back to the correct merged PRs.pr-linkvalues to real mergedSemiAnalysisAI/InferenceXPR links.#1002MiniMax entry because the merged#1003entry contains the fuller/current description.pr-link, per follow-up cleanup direction.Validation
210207-3pr-linkvalues.ChangelogEntryPydantic validation passed for all207entries.git diff --checkpassed.python -m pytest utils/matrix_logic/test_validation.py -qpassed:61 passed.