Correct the split-PR skill's stacking rules for copy-pr-bot CI - #6593
Conversation
The skill's stacking guidance predated actually running a stacked series through Megatron-LM's CI and was wrong or incomplete in ways that bite: - It said to CREATE dependent PRs with base pull-request/<N>; that ref does not exist until a vetter runs /ok to test, so creation fails. Correct flow: create everything on main, vet, then retarget. - Its only merge warning was the auto-close hazard. The worse failure is merging while mirror-based: GitHub merges into the bot's scratch ref, nothing lands on main, and the MERGED PR cannot be reopened. Rule: retarget to main before merging, always. - It did not explain the copy-pr-bot trust model that stacking piggybacks on (vetted-snapshot refs, staleness on push, /ok to test <sha>), the squash-merge rebase cascade children need after each parent lands, the lint job checking touched core/tests files against main's formatter pins, or how to linearize a PR with two dependencies. Adds a CI-model section, splits the workflow into create -> stack for review -> merge bottom-up phases, and folds the failure modes into the constraints. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: ilml <tolong@nvidia.com>
|
/ok to test e89c8e89bcedbcaa53c47b90b0e08bfd6dcab1cd |
@ilml, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
copy-pr-bot deletes the pull-request/<N> mirror when PR N merges or closes, and GitHub then auto-closes every PR based on that ref; such a PR cannot be reopened while the ref is missing, so a single parent merge can domino-close the rest of the stack faster than anyone can react. Move the un-stacking step to the moment the parent is approved -- the last point at which the child can safely leave the mirror -- and extend the suggested watchdog to flag an approved parent that still has mirror-based children. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ilml <tolong@nvidia.com>
wujingyue
left a comment
wujingyue
left a comment
There was a problem hiding this comment.
Could we make this PR much more concise? I think the /ok to test part is really what was missed. Other than that, #5496 (merged 1.5 months ago) seems to be sufficient.
In addition, you may want to mention why https://docs.github.com/en/pull-requests/how-tos/create-pull-requests/creating-stacked-pull-requests doesn't work for us at this moment for posterity.
The pull-request/<N> mirror refs only exist after a vetter's /ok to test, are merged into if a mirror-based PR is merged, and are deleted (auto-closing dependents) when the base PR merges or closes. Create PRs on main, stack after vetting, never merge mirror-based, and retarget dependents to main on base-PR approval. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ilml <tolong@nvidia.com>
ilml
changed the title
|
/ok to test be9de0a |
yes agree, fixed! |
wujingyue
left a comment
wujingyue
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the PR!
wujingyue
left a comment
wujingyue
left a comment
There was a problem hiding this comment.
why https://docs.github.com/en/pull-requests/how-tos/create-pull-requests/creating-stacked-pull-requests doesn't work for us at this moment? because we can't create branches directly in megatron, we have to rely on copy-pr-bot to create/copy branches for us.
This should be added to SKILL.md so agents know.
|
/ok to test a0a24c88c355072a6a90de1546f79dd6802eb92d |
@ilml, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributors cannot push branches to NVIDIA/Megatron-LM, and a PR's base must be an upstream branch, so the documented stacked-PR flow of basing each PR on the previous feature branch does not apply; the copy-pr-bot pull-request/<N> mirrors are the only usable stack bases. Also drop the Important-guidelines bullet on retargeting, which duplicated the answer-first constraint (review feedback). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ilml <tolong@nvidia.com>
|
/ok to test a0a24c8 |
2 participants
What does this PR do?
Corrects the
mcore-split-prskill's stacked-PR guidance to match how copy-pr-bot CI actually works:main;pull-request/<N>mirror refs only exist after a vetter's/ok to test, so stack (gh pr edit <child> --base pull-request/<parent>) only after vetting.pull-request/*ref — the squash lands in the bot's scratch ref, notmain, and the PR ends up MERGED and unreopenable.mainas soon as the base PR is approved — the bot deletes the mirror ref when the base PR merges or closes, and GitHub then auto-closes every PR based on it.Docs/skill only, no code changes.
why https://docs.github.com/en/pull-requests/how-tos/create-pull-requests/creating-stacked-pull-requests doesn't work for us at this moment?
because we can't create branches directly in megatron, we have to rely on copy-pr-bot to create/copy branches for us.