Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 68 additions & 1 deletion slime/backends/megatron_utils/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from megatron.core.optimizer.optimizer import MegatronOptimizer
from megatron.core.optimizer_param_scheduler import OptimizerParamScheduler
from megatron.core.pipeline_parallel import get_forward_backward_func
from megatron.core.utils import get_model_config
from megatron.core.utils import get_model_config, unwrap_model
from megatron.training.global_vars import get_args
from megatron.training.training import get_model

Expand All @@ -33,6 +33,68 @@
logger = logging.getLogger(__name__)


def _iter_critic_output_layers(model: Sequence[DDP]):
for chunk_id, module in enumerate(unwrap_model(model)):
output_layer = getattr(module, "output_layer", None)
if output_layer is not None:
yield chunk_id, output_layer
Comment on lines +36 to +40

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_iter_critic_output_layers() yields chunk_id, but the returned chunk_id is never used by callers. This makes the helper harder to read than necessary. Either drop chunk_id from the yield, or include it in the warning/logging so it’s actionable when multiple pipeline chunks exist.

Copilot uses AI. Check for mistakes.


def _critic_output_layer_needs_reinit(args: Namespace, model: Sequence[DDP], role: str) -> bool:
if role != "critic" or args.load is None:
return False

from megatron.core.dist_checkpointing.serialization import load_tensors_metadata
from megatron.training.checkpointing import get_load_checkpoint_path_by_args

checkpoint_path = Path(get_load_checkpoint_path_by_args(args))
if not (checkpoint_path / ".metadata").is_file():
return False

checkpoint_metadata = load_tensors_metadata(str(checkpoint_path))
for _chunk_id, output_layer in _iter_critic_output_layers(model):
for name in ("weight", "bias"):
param = getattr(output_layer, name, None)
if param is None:
continue

param_name = f"output_layer.{name}"
ckpt_tensor_metadata = next(
(
tensor_metadata
for key, tensor_metadata in checkpoint_metadata.items()
if key == param_name or key.endswith(f".{param_name}")
),
None,
)
Comment on lines +61 to +69

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_critic_output_layer_needs_reinit() does an O(N) scan over the entire checkpoint_metadata dict for each parameter via next(... for key, tensor_metadata in checkpoint_metadata.items() ...). For large distributed checkpoints this can add noticeable startup latency. Consider trying an exact-key lookup first (e.g., checkpoint_metadata.get(param_name)), and only falling back to a suffix scan if needed, or building a one-time suffix->metadata index to avoid repeated full scans.

Copilot uses AI. Check for mistakes.
expected_shape = tuple(param.shape)
checkpoint_shape = tuple(ckpt_tensor_metadata.global_shape) if ckpt_tensor_metadata is not None else None
if checkpoint_shape == expected_shape:
continue

reason = (
"missing from checkpoint metadata"
if checkpoint_shape is None
else f"shape mismatch checkpoint={checkpoint_shape} runtime={expected_shape}"
)
logger.warning(
"Will reinitialize critic %s after checkpoint load because it is %s",
param_name,
reason,
)
return True

return False


@torch.no_grad()
def _reinitialize_critic_output_layer(model: Sequence[DDP]) -> None:
for _chunk_id, output_layer in _iter_critic_output_layers(model):
output_layer.weight.data.normal_(mean=0.0, std=0.02)
if output_layer.bias is not None:
output_layer.bias.data.zero_()


def get_optimizer_param_scheduler(args: Namespace, optimizer: MegatronOptimizer) -> OptimizerParamScheduler:
"""Create and configure the optimizer learning-rate/weight-decay scheduler.

Expand Down Expand Up @@ -776,6 +838,7 @@ def initialize_model_and_optimizer(

model, optimizer, opt_param_scheduler = setup_model_and_optimizer(args, role)
model[0].role = role
reinit_critic_output_layer = _critic_output_layer_needs_reinit(args, model, role)
clear_memory()
iteration, _ = load_checkpoint(
model,
Expand All @@ -784,6 +847,10 @@ def initialize_model_and_optimizer(
checkpointing_context={},
skip_load_to_model_and_opt=False,
)
if reinit_critic_output_layer:
_reinitialize_critic_output_layer(model)
if (args.fp16 or args.bf16) and optimizer is not None:
optimizer.reload_model_params()

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new checkpoint-metadata-based reinitialization path for the critic output layer is subtle and easy to regress (e.g., loading a checkpoint that doesn’t contain output_layer.* or has a different shape). It would be good to add a regression test that exercises loading a critic from a checkpoint missing/mismatching the value head and asserts training can proceed and the output layer parameters are reinitialized as intended.

Suggested change
optimizer.reload_model_params()
reload_model_params = getattr(optimizer, "reload_model_params", None)
if callable(reload_model_params):
reload_model_params()
else:
logger.warning(
"Critic output layer was reinitialized after checkpoint load, but optimizer %s "
"does not implement reload_model_params(); continuing without refreshing "
"mixed-precision optimizer parameter copies.",
type(optimizer).__name__,
)

Copilot uses AI. Check for mistakes.
clear_memory()

return model, optimizer, opt_param_scheduler, iteration
Loading