Repository navigation
Fix import error by DSA spec - #8414
Conversation
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
|
This PR has been automatically converted to draft because all PRs must start as drafts. When you are ready for review, click Ready for Review to begin the review process. This will:
See the contribution guide for more details. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
The path-copying change looks sound in this light static review. One non-blocking coverage suggestion:
Add a regression test for non-deepcopyable builders
The new derivation logic deliberately avoids copying optional-dependency builders. The existing DSA spec tests check module selection, upstream nonmutation, and mixer differences, but do not exercise a builder that rejects deepcopy. With dependencies installed, those assertions also pass under the old implementation, leaving the import-failure mechanism unpinned by a unit test.
Suggested addition to that test file, using its existing imports:
from dataclasses import replace
from megatron.core.transformer.experimental_attention_variant.dsa_layer_specs import (
_with_dsa_gqa_attention,
)
class _UncopyableBuilder:
def __call__(self, *args, **kwargs):
raise AssertionError("Spec derivation must not instantiate builders")
def __deepcopy__(self, memo):
raise TypeError("cannot pickle 'cell' object")
@pytest.mark.parametrize("upstream", [hybrid_stack_spec, gated_delta_product_stack_spec])
def test_dsa_derivation_does_not_copy_optional_builders(upstream):
mixer = replace(upstream.submodules.mamba_layer, module=_UncopyableBuilder())
source = replace(upstream, submodules=replace(upstream.submodules, mamba_layer=mixer))
derived = _with_dsa_gqa_attention(source)
assert derived.submodules.mamba_layer is mixer
assert derived.submodules.attention_layer is not source.submodules.attention_layerPreservation and acceptance: This proposed test should return normally for both upstream stacks; reverting to whole-stack deepcopy should raise the synthetic TypeError. Retain the existing assertions that both derived stacks use DSGroupedSelfAttention, neither upstream attention module changes, and the two mixers remain different. Also check that separate derivations receive distinct indexer specs and retain the upstream dense-attention builder for warmup. The invariant is selective copying without instantiating builders or mutating upstream specs—not restoring whole-stack deepcopy.
Suggested code requires author verification. Tests were not executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
|
/ok to test 41855ab |
|
/ok to test 57f1ed7 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/39796311996 |
Seventeen upstream commits since 9e5aad0, none touching a file this branch changes; no conflicts. The merge brings in NVIDIA#8414 ("Fix import error by DSA spec"), which repairs the import of megatron.core.transformer.experimental_attention_variant.dsa_layer_specs that made the Installation Test fail on main from 2026-10-21 up to and including 9e5aad0, and therefore on this branch's head 09cced3. Also in: NVIDIA#8404 removed eight gpt functional-test cases (the corpus-parity gate now covers 297 configs), NVIDIA#7394 added two MoE router flags (outside the data group; the data-arguments snapshot is unchanged), NVIDIA#7926 removed an inference flag. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: asolergi-nv <asolergibert@nvidia.com>
…tegration-20261021 Picks up NVIDIA#8414, which fixes the DSA layer-spec import failure inherited from main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Lawrence McAfee <lmcafee@nvidia.com>
What does this PR do?
This PR fixes the CI import checking error caused by the DSA spec introduced in #7156.