fix: remove mlm workspace#2136
Conversation
|
/ok to test 85fbdad |
85fbdad to
e1b3b1b
Compare
|
/ok to test e1b3b1b |
e1b3b1b to
7741c47
Compare
|
/ok to test 7741c47 |
|
/ok to test a856567 |
|
/ok to test 0c3dd8b |
|
/ok to test 142e33f |
|
@kajalj22 cc: @terrykong This PR helps get you further. The nv-grouped-gemm dependency needed to also be updated in the lock file. Tests are still failing due to what is being installed. I'll see if I can help you get closer but a problem I think is that RL was installing Megatron-LM as a workspace, which will include it by default in the uv environment. It's also relying on some things that other projects typically do not rely on because they are technically not part of Megatron-Core itself like The way RL is written or at least the way our tests are written, it assumes Megatron-LM is not an optional dependency. I may try to still make Megatron-LM a workspace still to keep things consistent but just point to code in MBridge instead. That may be easiest. |
|
I updated my PR to your PR to re-add Megatron-LM as a workspace. I am attempting to see if adding Megatron-LM as a symlink to MBridge's Megatron-LM reference will work better. |
|
Yeah the CI gets further with that change to still make Megatron-LM a workspace but symlinking the code from Megatron-Bridge's Megatron-LM submodule. https://github.com/NVIDIA-NeMo/RL/actions/runs/23710543221/job/69073536800?pr=2158 However, it fails somewhere in Megatron-LM. I'd recommend check with Terry whether this approach seems feasible. If so, then resolving your merge conflicts and merging latest main branch of nemo-rl to your PR. There were some recent changes and perhaps those fixed this issue. |
Sounds good, I will check with Terry, thank you for the help! |
3ca7089 to
6718424
Compare
|
/ok to test 6718424 |
6718424 to
919b9ae
Compare
|
/ok to test 919b9ae |
eb45bc6 to
b465fdc
Compare
|
/ok to test b465fdc |
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
yuki-97
left a comment
There was a problem hiding this comment.
thanks Kajal, some minor comments.
|
/ok to test 3872c4e |
Signed-off-by: Kajal Jain <kajalj@nvidia.com>
The MLM workspace setup.py previously provided a git-pinned source for nvidia-resiliency-ext. Without it, uv falls back to PyPI which has no cp313 wheels, breaking resolution. Signed-off-by: Kajal Jain <kajalj@nvidia.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Signed-off-by: Kajal Jain <kajalj@nvidia.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kajal Jain <kajalj@nvidia.com>
References to 3rdparty/Megatron-LM-workspace/Megatron-LM updated to 3rdparty/Megatron-Bridge-workspace/Megatron-Bridge/3rdparty/Megatron-LM across runtime init, k8s configs, tests, docs, and tools. Signed-off-by: Kajal Jain <kajalj@nvidia.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kajal Jain <kajalj@nvidia.com>
The removed Megatron-LM-workspace/setup.py directly listed onnxscript as a dependency. With that workspace gone, onnxscript is only available under megatron-core[dev] and megatron-core[lts] extras but the mcore extra installs plain megatron-core. transformer_engine imports onnxscript at module load time, causing --mcore-only tests to fail with ModuleNotFoundError. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kajal Jain <kajalj@nvidia.com>
megatron-bridge unconditionally imports modelopt at module load time (auto_bridge.py imports is_quantized). The old MLM workspace's setup.py bundled nvidia-modelopt as a direct dependency; with that workspace removed, modelopt is no longer available under --extra mcore. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kajal Jain <kajalj@nvidia.com>
The is_megatron_installed module was provided by the removed Megatron-LM-workspace package (via py_modules in setup.py). Replace the module imports with direct imports that fail fast under set -e. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kajal Jain <kajalj@nvidia.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Kajal Jain <kajalj@nvidia.com>
|
/ok to test 38c16b6 |
What does this PR do?
Removes the
3rdparty/Megatron-LM-workspace/directory, which was a proxy workspace that re-packaged megatron-core with all its dev dependencies bundled into a flatinstall_requires. Instead, megatron-core is now sourced directly from the MBridge submodule at3rdparty/Megatron-Bridge-workspace/Megatron-Bridge/3rdparty/Megatron-LM.Why
The MLM workspace was a maintenance burden — it had a hand-maintained
CACHED_DEPENDENCIESlist insetup.pythat had to stay in sync with megatron-core'spyproject.toml. Removing it simplifies the dependency tree and eliminates a source of drift.What changed
3rdparty/Megatron-LM-workspace/(setup.py, pyproject.toml, is_megatron_installed.py, submodule pointer).gitmodulesentry and workspace member for the old submodulemegatron-coresource inpyproject.tomlto MBridge's Megatron-LM pathsys.pathhack innemo_rl/__init__.pyto point to the new Megatron-LM location (formegatron.trainingimportability)onnxscriptandnvidia-modelopt[torch]to themcoreextra — these were previously pulled in as transitive deps of the old workspace's fatinstall_requiresand are required at module load time bytransformer_engineandmegatron-bridgerespectivelynvidia-resiliency-extgit source override (needed for cp313 wheel resolution)is_megatron_installedmodule was apy_modulesentry in the deleted setup.py)Before your PR is "Ready for review"
Pre checks: