[models] fix: support CPU-only checkpoint conversion - #3853
Conversation
|
Progress update for issue #3295 CPU-only conversion:
Validation on rebased HEAD
Residual risk: this validates CPU-only model construction/conversion setup for the scoped |
|
/ok to test 761a2f1 |
| if getattr(config, "mtp_num_layers", None): | ||
| from megatron.core.models.gpt.gpt_layer_specs import get_gpt_mtp_block_spec | ||
|
|
||
| use_transformer_engine = not _should_use_local_layer_spec_for_cpu_only_initialization(config) |
There was a problem hiding this comment.
nit: The existing mtp_block_spec tests (lines 461, 494, 497, 526 in test_gpt_provider.py) all still assert use_transformer_engine=True — they pass because the test providers default to use_cpu_initialization=False. Consider adding one test that sets use_cpu_initialization=True with CUDA mocked away, so the use_transformer_engine=False path through get_gpt_mtp_block_spec and get_gpt_decoder_layer_specs is covered.
There was a problem hiding this comment.
Added this coverage in test_mtp_block_spec_uses_local_decoder_spec_for_cpu_only_initialization.
The test forces use_cpu_initialization=True with no visible CUDA, drives the empty layer_specs fallback, and asserts both get_gpt_decoder_layer_specs() and get_gpt_mtp_block_spec() receive use_transformer_engine=False.
|
Review: [models] fix: support CPU-only checkpoint conversion Clean PR - the backend selection, TE feature disabling, and layer-spec fallback logic all look correct. The context manager in One gap: the Suggested test cases: No perf tests impacted. |
| ModelT = TypeVar("ModelT", bound=MegatronModule) | ||
|
|
||
|
|
||
| def _cuda_is_available() -> bool: |
There was a problem hiding this comment.
can you help to move utils functions to utils file?
There was a problem hiding this comment.
Moved the CPU-only helper functions out of model_provider.py into src/megatron/bridge/models/utils.py, with model_provider.py importing them from there. I also updated the affected mixin tests to patch the relocated helper module.
761a2f1 to
73080e4
Compare
|
Review follow-up pushed in Changes:
Validation:
Residual risk: validation is focused on the CPU-only conversion/provider path and review-requested MTP fallback; full CI is still the broader safety net. |
|
/ok to test 73080e4 |
| @@ -374,6 +374,58 @@ def test_default_layer_spec_default_case(self, mock_te_full_spec, mock_te_spec): | |||
| mock_te_spec.assert_called_once_with(provider) | |||
There was a problem hiding this comment.
add a cpu only functional conversion tests please. Can be L1.
There was a problem hiding this comment.
Added L1 CPU-only functional conversion coverage in 2e33ee6250c0bfb3b7154aecf1859043a20f4b2a.
What changed:
- Added
tests/functional_tests/test_groups/converter/test_cpu_only_conversion.py, which creates a tiny local HF Llama model and runsAutoBridge.to_megatron_model(wrap_with_ddp=False, use_cpu_initialization=True)with CUDA hidden. - Added
tests/functional_tests/launch_scripts/h100/active/L1_Launch_converter_cpu_only.shso this is picked up as an H100 L1 functional launch script.
cw validation:
11787115completed0:0: new L1 launch script passed (1 passed) and completed the CPU-only tiny Llama conversion.
Log:/lustre/fsw/portfolios/coreai/projects/coreai_dlalgo_llm/users/yuya/logs/issue3295/issue3295-functional-cpu-container_11787115.log11787138completed0:0:uv run --no-sync pre-commit run --all-filespassed.
Log:/lustre/fsw/portfolios/coreai/projects/coreai_dlalgo_llm/users/yuya/logs/issue3295/issue3295-functional-precommit-container_11787138.log
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
73080e4 to
2e33ee6
Compare
|
/ok to test 2e33ee6 |
|
/ok to test f0788ae |
| ) | ||
| spec = decoder_layer_specs[-1] | ||
| return get_gpt_mtp_block_spec(config, spec, use_transformer_engine=True, vp_stage=vp_stage) | ||
| return get_gpt_mtp_block_spec(config, spec, use_transformer_engine=use_transformer_engine, vp_stage=vp_stage) |
There was a problem hiding this comment.
can a model converted without TE work with TE training?
|
Closing this PR without merge. The scoped CPU-only path explored here gets model construction farther, but the compatibility tradeoff is too large to merge right now. If no GPU is available, we cannot use Transformer Engine, so this path has to fall back to the local spec. That means the imported model can have issues or differ from the model a user later trains on GPU with TE enabled. We should defer this approach until there is a stronger request for CPU-only checkpoint conversion and a better compatibility design that preserves the intended GPU/TE training behavior. |
Summary
Fixes #3295.
persist_layer_normbehavior during CPU-only initialization.Validation
11784762completed0:0/lustre/fsw/portfolios/coreai/projects/coreai_dlalgo_llm/users/yuya/logs/issue3295/issue3295-cpu7_11784762.loguv sync --frozen --no-install-project --no-install-package megatron-core6 passed, 30 deselectedAutoBridge.to_megatron_model(wrap_with_ddp=False, use_cpu_initialization=True)completed withREPRO_DONE11784769completed0:0/lustre/fsw/portfolios/coreai/projects/coreai_dlalgo_llm/users/yuya/logs/issue3295/issue3295-pc2_11784769.loguv run --no-sync pre-commit run --all-files