[doc] fix: note the megatron commit MTP needs for recompute_granularity=full - #7346
Merged
wuxibin89 merged 1 commit intoAug 11, 2026
Conversation
…ty=full
`docs/advance/mtp.md` pins megatron dev at `23e092f41` (2025-12-09) for MTP + CP.
That commit predates `ffd66a3e6` ("Roll input IDs for MTP labels",
NVIDIA/Megatron-LM#3457, 2026-06-03), which is what threads `padding_mask` through
`MultiTokenPredictionLayer._checkpointed_forward`.
Without verl-project#3457, `MultiTokenPredictionLayer.forward` passes `padding_mask=` to a
`_checkpointed_forward` that does not declare it, so MTP with
`recompute_granularity=full` raises a TypeError on the first step. Following the doc
as written therefore still crashes for that combination. Released `megatron-core`
0.18.0 and 0.18.2 do not carry the fix either (NVIDIA/Megatron-LM#4933).
Verified by parsing the file with `ast` at each ref rather than grepping, since the
call site a few lines below the signature also contains the string: `4c6360260`
(2026-05-22) lacks `padding_mask` in the signature, `ffd66a3e6` is the first commit
that has it, and `dev`/`main` have it today while both 0.18.x tags do not.
The existing pin is left in place: I have not validated MTP + CP on a newer dev
commit, so I would rather add the constraint than silently move a pin someone else
verified. Happy to bump it if you have a newer commit you trust.
Raised in verl-project#7326, where the suggestion was to document the required
Megatron fix instead of carrying a compatibility shim in verl.
Signed-off-by: gaohongkui <gaohongkui1021@163.com>
wuxibin89
approved these changes
Aug 11, 2026
kahlun
pushed a commit
to kahlun/verl
that referenced
this pull request
Aug 20, 2026
…ty=full (verl-project#7346) ### What does this PR do? Follow-up to verl-project#7326, where the review outcome was that verl should document the required Megatron fix rather than carry a compatibility shim. This does that. `docs/advance/mtp.md` pins megatron dev at [`23e092f41`](https://github.com/NVIDIA/Megatron-LM/tree/23e092f41ec8bc659020e401ddac9576c1cfed7e) (2025-12-09) for MTP + CP. That commit predates [`ffd66a3e6`](NVIDIA/Megatron-LM#3457) ("Roll input IDs for MTP labels", NVIDIA/Megatron-LM#3457, 2026-06-03) by about six months, and verl-project#3457 is what threads `padding_mask` through `MultiTokenPredictionLayer._checkpointed_forward`. Without it, `MultiTokenPredictionLayer.forward` passes `padding_mask=` to a `_checkpointed_forward` that does not declare the parameter, so **MTP with `recompute_granularity=full` raises a `TypeError` on the first step** — i.e. following this doc as written still crashes for that combination. Released `megatron-core` 0.18.0 and 0.18.2 do not carry the fix either, tracked upstream as NVIDIA/Megatron-LM#4933. ### Checklist Before Starting - [x] Search for similar PRs. Queries run: [`mtp.md megatron dev commit`](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+mtp.md+megatron+dev+commit), [`mtp recompute_granularity`](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+mtp+recompute_granularity), [`3457 megatron`](https://github.com/verl-project/verl/pulls?q=is%3Apr+is%3Aopen+3457+megatron) — no open PR touches this. - [x] Format the PR title as `[{modules}] {type}: {description}` ### Test Documentation only, so the thing to verify is that the factual claim is right. I checked the signature of `MultiTokenPredictionLayer._checkpointed_forward` at each ref by parsing the file with `ast` rather than grepping — grep is misleading here because the call site a few lines below the signature also contains the string `padding_mask`, which is how I first got this wrong: | ref | date | `padding_mask` in signature | |---|---|---| | `4c6360260` | 2026-05-22 | no | | **`ffd66a3e6`** (verl-project#3457) | **2026-06-03** | **yes** — first commit that has it | | `23e092f41` (currently pinned in this doc) | 2025-12-09 | no | | `core_v0.18.0` | 2026-06-22 | no | | `core_v0.18.2` (newest release) | 2026-07-20 | no | | `dev` @`43124b60c` | 2026-08-08 | yes | | `main` @`6518b75ec` | 2026-08-09 | yes | The bisect was done over the file's commit history via `gh api "repos/NVIDIA/Megatron-LM/commits?path=megatron/core/transformer/multi_token_prediction.py&sha=main"`, then fetching the blob at each candidate. Repo checks run: `python3 tests/special_sanity/check_docs_time_info.py` (✅), `PR_TITLE=... python3 tests/special_sanity/check_pr_title.py` (✅). Both added links return HTTP 200. ### API and Usage Example No API change. ### Design & Code Changes One bullet in `docs/advance/mtp.md` §1 "Scope of Support" gains the additional constraint for `recompute_granularity=full`, plus the `Last updated` date. **I deliberately did not move the pin itself.** I have not validated MTP + CP on a newer dev commit, and I would rather add the constraint than silently replace a pin someone else verified. If you have a newer dev commit you trust for MTP + CP, bumping it would make this bullet simpler and I'm happy to do that instead. ### Checklist Before Submitting - [x] Read the Contribute Guide. - [ ] Apply pre-commit checks — `pre-commit` was not installable in my environment; I ran the repo's own sanity checks directly (above). Note that `pre-commit (3.12)` is currently failing on `main` for an unrelated file (`tests/trainer/ppo/test_reinforce_pp_multiturn_on_cpu.py`, a `ruff-format` diff from verl-project#7300), so a red pre-commit here would not be from this change. - [x] Add / Update the documentation — this PR *is* the documentation change. - [ ] Add unit or end-to-end test(s) — not applicable to a docs-only change. --- **AI assistance disclosure:** prepared with AI assistance (Claude). I reviewed every changed line and verified each factual claim above myself. Signed-off-by: gaohongkui <gaohongkui1021@163.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
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.
What does this PR do?
Follow-up to #7326, where the review outcome was that verl should document the required Megatron fix rather than carry a compatibility shim. This does that.
docs/advance/mtp.mdpins megatron dev at23e092f41(2025-12-09) for MTP + CP. That commit predatesffd66a3e6("Roll input IDs for MTP labels", NVIDIA/Megatron-LM#3457, 2026-06-03) by about six months, and #3457 is what threadspadding_maskthroughMultiTokenPredictionLayer._checkpointed_forward.Without it,
MultiTokenPredictionLayer.forwardpassespadding_mask=to a_checkpointed_forwardthat does not declare the parameter, so MTP withrecompute_granularity=fullraises aTypeErroron the first step — i.e. following this doc as written still crashes for that combination. Releasedmegatron-core0.18.0 and 0.18.2 do not carry the fix either, tracked upstream as NVIDIA/Megatron-LM#4933.Checklist Before Starting
mtp.md megatron dev commit,mtp recompute_granularity,3457 megatron— no open PR touches this.[{modules}] {type}: {description}Test
Documentation only, so the thing to verify is that the factual claim is right. I checked the signature of
MultiTokenPredictionLayer._checkpointed_forwardat each ref by parsing the file withastrather than grepping — grep is misleading here because the call site a few lines below the signature also contains the stringpadding_mask, which is how I first got this wrong:padding_maskin signature4c6360260ffd66a3e6(#3457)23e092f41(currently pinned in this doc)core_v0.18.0core_v0.18.2(newest release)dev@43124b60cmain@6518b75ecThe bisect was done over the file's commit history via
gh api "repos/NVIDIA/Megatron-LM/commits?path=megatron/core/transformer/multi_token_prediction.py&sha=main", then fetching the blob at each candidate.Repo checks run:
python3 tests/special_sanity/check_docs_time_info.py(✅),PR_TITLE=... python3 tests/special_sanity/check_pr_title.py(✅). Both added links return HTTP 200.API and Usage Example
No API change.
Design & Code Changes
One bullet in
docs/advance/mtp.md§1 "Scope of Support" gains the additional constraint forrecompute_granularity=full, plus theLast updateddate.I deliberately did not move the pin itself. I have not validated MTP + CP on a newer dev commit, and I would rather add the constraint than silently replace a pin someone else verified. If you have a newer dev commit you trust for MTP + CP, bumping it would make this bullet simpler and I'm happy to do that instead.
Checklist Before Submitting
pre-commitwas not installable in my environment; I ran the repo's own sanity checks directly (above). Note thatpre-commit (3.12)is currently failing onmainfor an unrelated file (tests/trainer/ppo/test_reinforce_pp_multiturn_on_cpu.py, aruff-formatdiff from [algo] fix: carry running_return through observation spans in REINFORCE++ (#7278) #7300), so a red pre-commit here would not be from this change.AI assistance disclosure: prepared with AI assistance (Claude). I reviewed every changed line and verified each factual claim above myself.