Apply-MegatronAdaptor-NPU-migration-to-clean-branch - #385
Conversation
There was a problem hiding this comment.
Code Review
This pull request transitions the NPU training stack from MindSpeed to MegatronAdaptor and TransformerEngineNPU, updates the Megatron-LM commit, and applies several patches for optimizer steps, memory pool regions, and vLLM speculative draft updates. The reviewer's feedback is highly actionable and should be addressed: the new megatron_bridge.patch is orphaned in the Dockerfile and lacks path resolution in the environment update script; cloning dependencies from main without pinning commits makes the Docker build fragile; and the run-qwen3-4B-npu.sh script contains several hardcoded absolute paths and NPU device IDs that break portability.
| RUN git clone https://gitcode.com/Ascend/MegatronAdaptor.git /root/MegatronAdaptor && \ | ||
| git clone https://gitcode.com/Ascend/TransformerEngineNPU.git /root/TransformerEngineNPU |
There was a problem hiding this comment.
Cloning MegatronAdaptor and TransformerEngineNPU from the main branch without pinning a specific commit or tag makes the Docker build non-reproducible and fragile. Any breaking change in their upstream repositories will break the build. Please define build arguments for their commits (e.g., MEGATRON_ADAPTOR_COMMIT and TRANSFORMER_ENGINE_NPU_COMMIT) and check them out, similar to Megatron-LM.
Documentation build overview
48 files changed ·
|
| done && \ | ||
| git -C /root/Megatron-LM apply --whitespace=nowarn \ | ||
| /tmp/npu_patch/megatron_comm.patch && \ | ||
| /tmp/npu_patch/megatron_mainline.patch && \ |
There was a problem hiding this comment.
Could we simplify the Megatron patch layering here? The two patches modify several of the same core files, making future mainline updates prone to apply conflicts or duplicated behavior; please consider a single composite patch or a minimal non-overlapping NPU overlay.
|
|
||
| + self._select_weight_update_target(is_draft) | ||
| self.weight_transfer_engine.start_weight_update() | ||
| self._weight_update_active = True |
There was a problem hiding this comment.
Could you clarify why thist file are being synchronized here?It seems not used by Dockerfile.npu,so it may be better to remove these unrelated changes or update these in a separate PR.
| + return True | ||
| + | ||
| + | ||
| +_register_auto_classes() |
|
|
||
| git -C "${WORKSPACE}/MindSpeed" apply --whitespace=nowarn "${PATCH_DIR}/mindspeed.patch" | ||
| pip install --no-deps --no-build-isolation -e ${WORKSPACE}/MegatronAdaptor | ||
| pip install --no-deps --no-build-isolation -e ${WORKSPACE}/TransformerEngineNPU |
There was a problem hiding this comment.
Do we need to clone these two repos or not? I think this step could use more detail.
| # MegatronAdaptor must run before Megatron imports so its dummy NPU modules | ||
| # are bound in Megatron tensor-parallel modules. | ||
| import megatron_adaptor # noqa: F401 | ||
| from . import npu_attention_patch # noqa: F401 |
There was a problem hiding this comment.
Why is this line needed here? Isn't the import in actor.py already sufficient?
| ARG MEGATRON_COMMIT=1dcf0dafa884ad52ffb243625717a3471643e087 | ||
| ARG MEGATRON_BRIDGE_COMMIT=3fd3768045422d0aa5c97e90a4e6c659aea9acb9 | ||
| ARG MINDSPEED_COMMIT=fc63de5c48426dd019c3b3f39e65f5bdf56e4086 | ||
| ARG MBRIDGE_COMMIT=89eb10887887bc74853f89a4de258c0702932a1c |
There was a problem hiding this comment.
Could you please remove the megatron_comm.patch and mindspeed.patch entries from docker/npu_patch/series.conf? This keeps the declared patch stack consistent with the Dockerfile and available patch files.
a283316 to
e91f80c
Compare
Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
6c86097 to
0b8002e
Compare
| pip install --no-deps --no-build-isolation -e /root/Megatron-LM && \ | ||
| pip install --no-deps --no-build-isolation -e /root/TransformerEngineNPU && \ | ||
| pip install --no-deps --no-build-isolation -e /root/MegatronAdaptor && \ | ||
| pip install --no-deps --no-build-isolation -e /root/MindSpeed |
There was a problem hiding this comment.
NPU CI (#193) likely fails during image build: Dockerfile.npu removes the MindSpeed clone but still runs pip install -e /root/MindSpeed.
Drop that line? This migration also requires image-build (not smk-only on the legacy MindSpeed image), and the common docker/patch/latest/megatron.patch is Dockerfile-only and not in series.conf.
There was a problem hiding this comment.
Offline weight updates depend on MindSpeed, so the MindSpeed code has been re-added.
7db37fb to
971e1af
Compare
Signed-off-by: wangxiaoxin-sherie <wangxiaoxin7@huawei.com>
Summary
Files Changed
vime/backends/megatron_utils/vime/utils/docker/scripts/examples/tests/train.pytrain_async.py