Add HybridModel MTP runtime controls - #6084
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
f8a36eb to
b722c55
Compare
|
/ok to test b722c55 |
|
/ok to test 582f111 |
|
/claude strict-review |
There was a problem hiding this comment.
Strict Review Summary
Findings — CRITICAL: 0, IMPORTANT: 0, SUGGESTION: 1
Assessment: Low risk. Recommend merge after considering the one naming nit.
This is a small, well-scoped, backward-compatible change adding a keyword-only compute_mtp_loss: bool = True control to HybridModel.forward.
Correctness verified:
- The default path (
compute_mtp_loss=True) is provably behavior-identical to the original:mtp_forward_rangains anand compute_mtp_lossterm that isTrueby default → identical to the prior expression.- The changed
elif not in_inference_mode:→elif mtp_forward_ran:at L579 is exactly equivalent in context: it sits underif ... self.mtp_process(L563) and in theelifofif is_spec_decode, somtp_forward_ranthere reduces tonot in_inference_mode.
- The disabled path (
compute_mtp_loss=False) cleanly skips both the MTP forward andprocess_mtp_losswhile leaving the MTP module, parameters, and checkpoint/state-dict structure untouched. - Speculative-decoding / inference paths are correctly unaffected — the spec-decode branch (L565-578) never depended on
mtp_forward_ran, andcompute_mtp_lossonly gates the training/eval block. The dedicated testtest_forward_mtp_loss_control_does_not_disable_speculative_decodingconfirms this. - Non-MTP pipeline ranks are safe (guarded by
self.mtp_process), and the intermediate-stage return (not self.post_process) is covered by a test.
Backward compatibility: New arg is keyword-only (after *) with a safe default, so no existing positional callers break, and no checkpoint/config/default-hyperparameter changes are introduced.
Tests: Good coverage — parametrized enabled/disabled/default, non-MTP rank, pre-post-process boundary, and spec-decode. import types and InferenceMode are properly imported.
Process-group guidance: No new parallel_state.get_*_group() reads introduced.
The single SUGGESTION is a non-blocking naming observation on compute_mtp_loss (the flag also gates the MTP forward pass, not just the loss).
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
|
/ok to test cf3b1d9 |
|
/claude strict-review |
There was a problem hiding this comment.
Strict review passed — no significant issues found. LGTM
Verified:
- Backward compatibility:
run_mtp_forwardis a keyword-only arg (after*) defaulting toTrue; no existing caller passes it, so existing behavior is preserved. - Behavioral equivalence for the default: at the
process_mtp_lossguard siteself.mtp_processis already True andis_spec_decodeis False, so withrun_mtp_forward=True,mtp_forward_ranreduces tonot in_inference_mode— exactly the priorelif not in_inference_modecondition. The refactor also correctly couplesprocess_mtp_lossto whether the MTP forward actually ran. - Spec decode unaffected:
is_spec_decodehandling andmtp_decoder_hidden_statespopulation remain gated only by inference context/controller state, not the new flag. - No new global process-group reads introduced in megatron/core.
- Test coverage exercises defaults, explicit enable/disable, non-MTP pipeline ranks, the pre-post-process return path, and non-interference with speculative decoding.
What does this PR do?
Adds a backward-compatible, per-forward
run_mtp_forwardcontrol toHybridModel. Target user is VeRL for Nemotron 3 models that use MTP.HybridModelcurrently executes its training/evaluation MTP forward whenever MTP layers are present and the model is outsideInferenceMode. Consumers may need to load, save, synchronize, or export MTP parameters without executing the MTP forward or attaching its auxiliary loss on every model call.This is required for native HybridModel integration in veRL. veRL distinguishes between loading MTP parameters, training them, and using them for speculative rollout. Its current workaround monkeypatches
GPTModel, which does not apply toHybridModeland should not become the long-term integration path as GPTModel is deprecated.Behavior
run_mtp_forward=Truepreserves the existing behavior and remains the default.When
run_mtp_forward=False,HybridModel.forward:process_mtp_lossand therefore does not attach the MTP auxiliary loss;InferenceMode, the inference context, and the speculative decoding controller.The control is call-time rather than construction-time because one model instance may alternate between forward-only operations and training updates while preserving the same loaded MTP parameters.