fix(cuda_graph): preserve explicit empty cuda_graph_scope as [] when clearing modules#4738
Closed
ko3n1g wants to merge 1 commit into
Conversation
…clearing modules set_cuda_graph_modules() cleared the deprecated per-layer cuda_graph_scope field to None on the current MCore API. This dropped an explicit cuda_graph_scope=[] (e.g. a Hydra/CLI override meaning "no per-layer scope") to None through the perf-script override chain (validate_cuda_graph_configuration -> clear_cuda_graph_modules -> set_cuda_graph_modules). Set it to [] instead, matching the legacy branch and the load_megatron_model expectation.
Contributor
Author
|
Closing — re-running the heal loop from scratch with a matured agent (fast exec_in_env env-diagnosis, no pre-fed command). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
On this branch, the unit test
tests/unit_tests/recipes/test_override_precedence.py::TestFullChainSanity::test_I_combined_overrides_end_to_endfails with
AssertionError: assert None == []— an explicitmodel.cuda_graph_scope=[]override doesnot survive the perf-script override chain (it is dropped to
None).Root cause
set_cuda_graph_modules()insrc/megatron/bridge/utils/cuda_graph.py, when clearing the per-layermodules, set
config.cuda_graph_scope = None. That destroys an explicit empty-list scope: a callerpassing
cuda_graph_scope=[](via a Hydra/CLI override) means "no per-layer scope", and that intentmust survive as
[], not becomeNone.Fix
Preserve the empty-list convention — set
config.cuda_graph_scope = []instead ofNonewhenclearing modules (1 file, +6/-1).
Tested
Reproduced and verified by running the exact test on the aws-h100 cluster via a Kubeflow TrainJob
(
nvcr.io/nvidia/nemo:26.04, with the branch'smegatron.corefrom the3rdparty/Megatron-LMsubmodule):
yuya/mb4583-cuda-graph-local-scope1 failed(assert None == [])1 passedOpened on behalf of an automated implement-and-heal agent that reproduced the failure, authored the
fix, and verified it green on a real GPU cluster before this PR was raised. Draft for the branch
owner's review.