[megatron] fix: make MTP work with recompute_granularity=full on megatron-core 0.18.x - #7326
[megatron] fix: make MTP work with recompute_granularity=full on megatron-core 0.18.x#7326gaohongkui wants to merge 2 commits into
Conversation
afa25b6 to
e667002
Compare
HollowMan6
left a comment
There was a problem hiding this comment.
#7328 should have already fixed this (bug 2), can you try the main branch again?
…tron-core 0.18.x
## What does this PR do?
Adds a compatibility shim so Multi-Token Prediction can be used together with
`recompute_granularity=full` on every released megatron-core, and makes the existing
signature-based branch in `patch_mtp_layer_checkpointed_forward` visible instead of silent.
### The bug
megatron-core 0.18.x is internally inconsistent: `MultiTokenPredictionLayer.forward` calls
self._checkpointed_forward(..., padding_mask=padding_mask) # multi_token_prediction.py
while `_checkpointed_forward` itself does not declare `padding_mask`. Any MTP run with
`recompute_granularity == 'full'` therefore dies at the first forward:
TypeError: MultiTokenPredictionLayer._checkpointed_forward() got an unexpected
keyword argument 'padding_mask'
Upstream cause: two landings that do not compose -- NVIDIA/Megatron-LM#2645 added the
call-site kwarg, verl-project#4593 refactored the method without it. Tracked as
NVIDIA/Megatron-LM#4933 (open since 2026-05-22).
It is fixed on megatron-core `main`, but `core_v0.18.2` was tagged 2026-07-20 -- two months
after that issue was filed -- and is still the newest release, so **every released
megatron-core hits this**. verl users cannot currently combine MTP with full activation
recomputation without patching megatron themselves.
### Why the existing patch does not cover it
`patch_mtp_layer_checkpointed_forward` skips any layer whose `_checkpointed_forward` does not
start with `forward_func`. That gate matches megatron-core 0.14-0.17 only; 0.18+ renamed the
first parameter to `hidden_states`, so on every 0.18+ install the patch silently does nothing:
when `target_layers` is non-empty but `patched_count == 0`, neither log line is printed, so
there is no way to tell whether it applied. Skipping is in fact *correct* on 0.18+ (megatron
now keeps non-tensor args out of the checkpoint natively, via its own `custom_forward`
closure) -- but it should say so.
## Changes
- `_patch_padding_mask_kwarg()`: when the layer's `_checkpointed_forward` lacks `padding_mask`,
rebind it to accept and drop the kwarg. `padding_mask` is dropped rather than forwarded
because there is no parameter to forward it to on these versions, and verl never constructs
one, so it is always `None` in practice. A non-`None` value raises `NotImplementedError`
rather than being silently ignored -- ignoring it would treat padded positions as real
tokens. Idempotent per layer.
- `patch_mtp_layer_checkpointed_forward()`: log the skip-by-signature and shim counts, so
"patch did not apply" is observable rather than inferred from missing output.
The style follows the existing precedent a few lines above in the same file, which already
probes `signature(self.mtp.forward)` for `padding_mask` before passing it.
## Test
`tests/models/test_mtp_checkpointed_forward_shim_on_cpu.py` -- 5 CPU cases, no GPU, no
distributed init, covering all three megatron-core signature generations:
- megatron-core `main` signature (declares `padding_mask`) -> shim not installed, kwarg still
reaches the method
- 0.18.x signature -> reproduces the bare `TypeError` first, then asserts the shim accepts
`padding_mask=None` and forwards the remaining args unchanged
- non-`None` mask -> `NotImplementedError` instead of silent corruption
- 0.14-0.17 `forward_func` signature -> left to the existing recompute patch
- shim is idempotent
Also verified end-to-end on 4x8 H20 with Qwen3.6-35B-A3B (MoE, 40 layers, 256 experts),
TP2/PP2/CP4/EP8, 64K context, `mtp_num_layers=1`, `recompute_granularity=full`,
megatron-core 0.18.2 + torch 2.11: without the shim training aborts at the first forward with
the `TypeError` above; with it, MTP trains and `mtp_losses/*` is reported normally
(30 steps, loss 0.447 -> 0.318).
Reported by / verified with AI assistance (Claude).
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: gaohongkui <gaohongkui1021@163.com>
e667002 to
9d0bcc3
Compare
|
Thanks for the pointer — I checked, and I think #7328 fixes a different failure. I've also rebased this branch onto current The two are different failure modes:
Why it still reproduces on main. In megatron-core 0.18.0 and 0.18.2, 1093: def _checkpointed_forward(
1095: hidden_states: Tensor,
1096: decoder_input: Tensor,
1097: attention_mask: Optional[Tensor] = None,
1098: context: Optional[Tensor] = None,
1099: context_mask: Optional[Tensor] = None,
# no padding_mask parameterand then calls it with one anyway: 1301: hidden_states = self._checkpointed_forward(
1305: padding_mask=padding_mask,verl's existing patch cannot intercept that, because of the guard in if not params or params[0] != "forward_func":
continueOn 0.18.x Note that main does already guard Cheapest way to check without a cluster: this PR ships Happy to also run the full multi-node MTP + |
HollowMan6
left a comment
There was a problem hiding this comment.
I was using the latest commit of megatron-lm dev branch and can't reproduce this: https://github.com/NVIDIA/Megatron-LM/tree/dev Is this caused by some specific commit in megatron-lm main?
…MTP shim test `test_shim_not_installed_when_padding_mask_already_supported` asserted `layer._checkpointed_forward is before`. Attribute access on a method that has not been rebound builds a fresh bound-method object each time, so that identity check fails even when the shim correctly did nothing -- which is what cpu_unit_tests hit. The shim rebinds by assigning an instance attribute, so "was not rebound" is expressed directly as `"_checkpointed_forward" not in vars(layer)`. The idempotency test is unaffected: after patching, the attribute holds a plain function, and accessing it returns that same object, so `is` is valid there. Signed-off-by: gaohongkui <gaohongkui1021@163.com>
|
Thanks for the approval. CI ran on the rebase and I've triaged the failures — one was mine and is now fixed in
Attribute access on a method that has not been rebound builds a fresh bound-method object every time, so I verified the fix by extracting the real
The four For what it's worth on blast radius: the shim only rebinds |
That matches what I'd expect: I checked the signature of
So the affected population is anyone on a released megatron-core 0.18.x — which is what I deliberately won't claim which commit fixed it on That is also why the shim self-disables rather than always patching: it only rebinds when the signature lacks Separately, on CI: |
|
It just hasn't shipped: So the one question that decides this PR is yours to answer: does verl intend to work against released megatron-core 0.18.x? If yes, this shim covers the gap until a release picks up |
There was a problem hiding this comment.
Okay, if NVIDIA/Megatron-LM#3457 can fix all these, then this PR is not needed, just need to note that the Megatron fixing PR is needed
|
Understood, and agreed — I've opened #7346 with the doc note instead. One thing worth flagging while doing it: Thanks for the review — closing this one out was the right call. |
…ty=full (#7346) ### 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.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 #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`** (#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 #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>
…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>
What does this PR do?
Makes Multi-Token Prediction usable together with
recompute_granularity='full'on everyreleased megatron-core, and makes the existing signature-based branch in
patch_mtp_layer_checkpointed_forwardvisible instead of silent.The bug
megatron-core 0.18.x is internally inconsistent:
MultiTokenPredictionLayer.forwardcallswhile
_checkpointed_forwarditself does not declarepadding_mask. Any MTP run withrecompute_granularity == 'full'dies at the first forward:Upstream cause: two landings that do not compose — NVIDIA/Megatron-LM#2645 added the call-site
kwarg, #4593 refactored the method without it. Tracked as NVIDIA/Megatron-LM#4933, still open.
It is fixed on megatron-core
main, butcore_v0.18.2was tagged 2026-07-20 — two months afterthat issue was filed — and is still the newest release, so every released megatron-core hits
this. I have proposed a backport as NVIDIA/Megatron-LM#6367; until a release carries it, verl
users cannot combine MTP with full activation recomputation without patching megatron themselves.
Why the existing patch does not cover it
patch_mtp_layer_checkpointed_forwardskips any layer whose_checkpointed_forwarddoes notstart with
forward_func. That gate matches megatron-core 0.14–0.17 only; 0.18+ renamed thefirst parameter to
hidden_states, so on every 0.18+ install the patch silently does nothing —when
target_layersis non-empty butpatched_count == 0, neither log line is printed, so thereis no way to tell whether it applied.
Skipping is in fact correct on 0.18+: megatron now keeps non-tensor args out of the
checkpoint natively, via its own
custom_forwardclosure. But it should say so rather than beinferred from missing output.
Changes
_patch_padding_mask_kwarg()— when the layer's_checkpointed_forwardlackspadding_mask,rebind it to accept and drop the kwarg.
padding_maskis dropped rather than forwarded becausethere is no parameter to forward it to on these versions, and verl never constructs one, so it
is always
Nonein practice. A non-Nonevalue raisesNotImplementedErrorrather than beingsilently ignored — ignoring it would treat padded positions as real tokens. Idempotent per
layer (after installation the shim's own signature declares
padding_mask, so a second call isa no-op).
patch_mtp_layer_checkpointed_forward()— log the skip-by-signature and shim counts.Style follows the existing precedent a few lines above in the same file, which already probes
signature(self.mtp.forward)forpadding_maskbefore passing it.Once a megatron-core release carries #6367, the signature check makes this shim skip itself
automatically, and it can be deleted.
Not a duplicate
Searched verl issues and PRs for
patch_mtp_layer_checkpointed_forward,mtp_patch checkpointed_forward,forward_func signature mtp— no hits. Theforward_funcgateis still present on
main(verl/models/mcore/mtp_patch.py), so nothing has superseded this.Test
tests/models/test_mtp_checkpointed_forward_shim_on_cpu.py— 5 CPU cases, no GPU, no distributedinit, covering all three megatron-core signature generations:
mainsignature (declarespadding_mask) → shim not installed, kwarg stillreaches the method
TypeErrorfirst, then asserts the shim acceptspadding_mask=Noneand forwards the remaining args unchangedNonemask →NotImplementedErrorinstead of silent corruptionforward_funcsignature → left to the existing recompute patchAll 5 pass locally.
Also exercised end to end on 4x8 H20 with Qwen3.6-35B-A3B (MoE, 40 layers, 256 experts),
TP2/PP2/CP4/EP8, 64K context,
mtp_num_layers=1,recompute_granularity='full', megatron-core0.18.2 + torch 2.11: without the shim training aborts at the first forward with the
TypeErrorabove; with it, MTP trains and
mtp_losses/*is reported normally (30 steps, loss 0.447 → 0.318).AI assistance (Claude) was used for the investigation and to draft the patch and tests; the diff
was reviewed line by line before submitting.