Repository navigation
Move vocab-parallel cross entropy and the norm modules into megatron/core/ops with deprecated aliases - #8656
guihong-nv wants to merge 7 commits into
Conversation
|
/ok to test 82590ed |
|
Something went wrong on our side and the automatic retries didn't recover — please try again. (ref: |
82590ed to
324e72f
Compare
|
/ok to test 324e72f |
…ted names
A forwarder resolved reads through PEP 562 __getattr__ but stored writes on
itself, so mock.patch or monkeypatch.setattr on an old import path patched
the forwarder and silently left the implementation untouched. Forwarders now
use a ModuleType subclass that sends writes to the module a name is read
from. Each write records the value it replaced: deleting the name, which is
how mock.patch and (on pytest 9.2+) monkeypatch.undo remove a patch, restores
that value rather than deleting the implementation's attribute, so nested
patches unwind in order. The import system's binding of a forwarder child
stays local, and a forwarder with several targets refuses writes instead of
guessing.
Megatron's two by-name spec loaders, spec_utils.import_module (--spec and
(module, name) ModuleSpecs) and the inference-counterpart lookup in
argument_utils, read vars(module), which skips a forwarder's __getattr__. They
now fall back to getattr / dir, so a spec recorded under an old path, as
checkpoints do for --use-checkpoint-args, keeps loading after a move.
deprecated_module() also takes exports={name: defining_module} for historical
packages that re-exported names from their modules. Those names resolve from
their defining modules and form the forwarder's __all__, so the canonical
package does not have to keep the legacy surface.
The regression suite gains synthetic tests for both, an EXPORTS table next to
FORWARDED (whose entries must name modules that exist), a check that canonical
package __init__ files re-export nothing, and string constants in the
canonical-path check, so patch targets that still spell an old path are
caught.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: guihong-nv <guihongl@nvidia.com>
…dules into megatron/core/ops Generated by ops_move_loss_norm.py on main plus the forwarder helper commit, the same way as the SSM and attention moves: git mv, import and path-string rewrites, a deprecated forwarder at each old path, and the FORWARDED entries. tensor_parallel/cross_entropy.py -> ops/loss/cross_entropy.py fusions/fused_cross_entropy.py -> ops/loss/fused_cross_entropy.py fusions/fused_layer_norm.py -> ops/norm/fused_layer_norm.py transformer/torch_norm.py -> ops/norm/torch_norm.py By hand: - CrossEntropyTarget and select_cross_entropy move verbatim from models/backends.py to ops/loss/backends.py, the family's selector; models/backends.py imports them from there. - ops/norm/torch_norm.py imports TransformerConfig for type checking only. Outside the transformer package, the runtime import re-entered torch_norm through transformer_layer and broke `import megatron.core`. - The moved fused_layer_norm.py gets its four pylint fixes (unused import, long comment, two docstrings), since CI lints it as a new file. - check_kernel_determinism_coverage.py does not count a deprecated forwarder left in a kernel directory as kernel-bearing; the kernels are registered at their new paths, and the manifest says why megatron/core/ops is not a kernel directory. - README layout and a section on where a new cross-entropy or norm backend goes, CODEOWNERS for ops/norm/torch_norm.py, the allowlist key order, and tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: guihong-nv <guihongl@nvidia.com>
324e72f to
5775864
Compare
|
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. |
|
/ok to test 5775864 |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
Found two localized compatibility regressions: nested patch restoration can delete a canonical attribute, and private HybridStack spec names no longer resolve their inference counterparts. Details and acceptance checks are inline.
Completed the light review across all changed files and directly affected paths. This was static inspection; no tests were 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.
…ssignment A forwarder recorded the value each write replaced on a stack, and treated writing that value back as undoing the write, which is how monkeypatch.undo restores before pytest 9.2. A nested patch that sets the outer patch's original for a while looks the same: it popped the outer patch's record, and its own exit then deleted the implementation's attribute, for good with create=True. mock.patch and pytest 9.2+ monkeypatch choose how to undo from the target's __dict__: a name the target holds is restored by assigning the value they saved, and one it does not hold is deleted. A forwarder now also holds a written name until the write ends, as a patched module does, and keeps only what the implementation held before the write. Inner patches see a local name and unwind by assignment; deleting the name, or assigning that original back, ends the write and restores it. Reads of a written name still come from the implementation, so a patch applied on the canonical path in the meantime stays visible through the old one. The write-through test adds the nested patch that restores the original, nested monkeypatch writes, a create=True patch of a name the implementation lacks, and a canonical patch inside an old-path one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: guihong-nv <guihongl@nvidia.com>
The forwarder fallback in _hybrid_inference_stack_spec read dir(module) and skipped every name with a leading underscore, to avoid the dunder names a forwarder does not serve. That also dropped private specs: a --spec of _foo_stack_spec no longer found _foo_inference_stack_spec, which vars(module) had returned, and the caller fell back to the default inference stack. Skip only dunder names. The spec-loading test checks the lookup through the canonical module and its forwarder for a private spec, an alias, the wide-residual counterpart and a spec without a counterpart. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: guihong-nv <guihongl@nvidia.com>
The rule named only torch_norm.py, keeping each file's previous owners: fused_layer_norm.py came from fusions/, which has no group of its own. Owning the directory puts every norm module, including ones added later, under the same reviewers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: guihong-nv <guihongl@nvidia.com>
|
/ok to test 13c6295 |
Writing through a deprecated forwarder the value the implementation
already holds, as in
mock.patch.object(old_mod, "NAME", old_mod.NAME)
raised KeyError: the write was taken as the end of an earlier write and
deleted a forwarder entry that did not exist yet. A write now ends only
when one is in progress and the value is the one it replaced. A first
write starts one whatever its value, so the delattr that mock.patch and
pytest 9.2+ monkeypatch use to undo it has a write to end.
Adds regression tests for a same-value mock.patch.object, mock.patch
with and without create=True, monkeypatch.setattr, and a plain
self-assignment followed by del.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: guihong-nv <guihongl@nvidia.com>
Resolves the import conflict in deepseek_v4_hybrid_attention.py: main adds OutputRopeParams from megatron.core.ops.attention.csa.output_rope (NVIDIA#8399) next to the LayerNormBuilder import this branch moved to megatron.core.ops.norm.torch_norm; both are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: guihong-nv <guihongl@nvidia.com>
|
/ok to test 6ad0da6 |
kevalmorabia97
left a comment
There was a problem hiding this comment.
The post_training path changes are import-only: ModelOpt's gpu_megatron export/distill/speculative (158 passed) and Megatron-Bridge examples (106 passed) give identical results before and after on nemo:26.08.
What does this PR do?
Gives vocab-parallel cross entropy and normalization a home in
megatron/core/ops,ops/lossandops/norm, following the layout and rules of #7380 and of the SSM and attention moves in #7453 and #8348. Kernel integrations such as #5438 (Liger-Kernel RMSNorm and vocab-parallel cross entropy) then add an adapter module and a selector branch in these families instead of new files infusions/andextensions/. Every old module path remains as a deprecated forwarder.Based on
main, ahead of the SSM move (#7453). It has six commits and a merge ofmain:ops: write patches through deprecated forwarders and forward re-exported names: Move SSM helpers, Mamba2, GDN/GDN2 and GDP into megatron/core/ops with deprecated aliases (2/4) #7453's first commit, unchanged. This move needs it too: without it, a patch written through an old path such asmegatron.core.fusions.fused_layer_normmisses the implementation, and a--specor checkpoint spec recorded under an old norm path stops loading. Move SSM helpers, Mamba2, GDN/GDN2 and GDP into megatron/core/ops with deprecated aliases (2/4) #7453 drops the commit once this PR lands.ops/loss, ops/norm: move vocab-parallel cross entropy and the norm modules into megatron/core/ops: the move.ops: keep a patched name on its forwarder so nested patches undo by assignment. A nested patch that set the outer patch's original back for a while lost the outer patch's record, and its exit deleted the implementation's attribute. A forwarder now holds a written name until the write ends, as a patched module does, somock.patchandmonkeypatchundo inner patches by assignment.training: keep private spec names in the inference-counterpart lookup. The forwarder fallback skipped every underscore name, so a private--specsuch as_foo_stack_specno longer found_foo_inference_stack_spec. It now skips only dunder names.CODEOWNERS: give @NVIDIA/transformer all of megatron/core/ops/norm, rather thantorch_norm.pyalone.ops: let a forwarder's first same-value write start a write. Patching a name through an old path with the value it already held, as inmock.patch.object(old_mod, "NAME", old_mod.NAME), raisedKeyError. A write now ends only when one is in progress and the value is the one it replaced.Merge main into ops-move/loss-norm: keeps both imports where Reduce DSv4 output RoPE activation memory #8399 added one next to a rewritten norm import indeepseek_v4_hybrid_attention.py. Since review started,mainis merged in rather than regenerated, so the reviewed commits stay as they are.It is independent of #8348. The moved code is unchanged apart from imports, so open fixes to the old files, such as #7466's label-smoothing fix in
tensor_parallel/cross_entropy.py, apply to the new paths once this lands.Like the other moves, the branch is generated by a script on
mainplus the helper commit rather than rebased, and is regenerated whenevermainchanges the moved files. Re-running the script reproduces the move commit apart from the manual steps listed below.Layout
megatron/core/)megatron/core/)tensor_parallel/cross_entropy.pyops/loss/cross_entropy.pyfusions/fused_cross_entropy.pyops/loss/fused_cross_entropy.pymodels/backends.py:CrossEntropyTarget,select_cross_entropyops/loss/backends.pyfusions/fused_layer_norm.pyops/norm/fused_layer_norm.pytransformer/torch_norm.pyops/norm/torch_norm.pyops/loss/__init__.pyandops/norm/__init__.pyhold each family's contract and re-export nothing. Transformer Engine's cross entropy and norms stay inextensions/transformer_engine.py, andfused_gated_norm.py(GDN's output norm) stays infusions/.Where #5438's pieces go
fusions/liger_cross_entropy.pyops/loss/liger_cross_entropy.py: an adapter with theCrossEntropyTargetsignaturetarget(logits, labels, tp_group)'liger'arm inLanguageModule.compute_language_model_loss'liger'branch inselect_cross_entropy(ops/loss/backends.py), the selector every provider asks. Since #6766,LanguageModulebinds whatever that selector returns, so the model needs no change'liger'incross_entropy_fusion_implselect_cross_entropyrejects values it does not know, so the value is added there tooLigerMegatronRMSNormimport inextensions/liger_kernel_spec_provider.pyops/norm/liger_norm.py, which importsliger_kernelat module top;LigerSpecProvider.layer_normreturns it, and the provider and theuse_ligerspec plumbing stay inextensions/andmodels/gptHAVE_LIGERflags andtry/except ImportErrorrequire("liger_kernel.megatron", ..., needed_by=...)in the selector branch, so the other values never import Liger and a missing package fails at constructionThe new README section "Adding a cross-entropy or normalization backend" describes this without naming Liger.
Compatibility
(module, name)specs recorded under them load the canonical objects (covered by the identity and pickle tests).from megatron.core.tensor_parallel import vocab_parallel_cross_entropyandfrom megatron.core.models.backends import CrossEntropyTarget, select_cross_entropykeep working and return the same objects.megatron.core.tensor_parallelno longer binds thecross_entropysubmodule as a package attribute. An explicitimport megatron.core.tensor_parallel.cross_entropystill works, through the forwarder. No in-tree code relied on the attribute.select_cross_entropynow readsis_te_min_versionfromops/loss/backends.py, so a test that patchedmegatron.core.models.backends.is_te_min_versionto steer it patches the new module instead. The two in-tree tests that did are updated.How it was generated
Manual steps after the script:
CrossEntropyTargetandselect_cross_entropyverbatim frommodels/backends.pytoops/loss/backends.py;models/backends.py, the TE spec provider andLanguageModuleimport them from there;ops/norm/torch_norm.pyimportsTransformerConfigfor type checking only (it is used in annotations only). Outside the transformer package, the runtime import ran the package__init__, which reachedtransformer_layer, which importstorch_normbefore it had finished loading, andimport megatron.corefailed. Inside the package, the package always initialized first;fused_layer_norm.pygets its four pylint fixes (an unusedimportlibimport, a long comment, two method docstrings), because the linting job checks it as a new file;tools/check_kernel_determinism_coverage.pydoes not count a deprecated forwarder left in a kernel directory as kernel-bearing.fusions/is one, and the forwarders there hold no kernel; the kernels are registered at their new paths. The alternative, manifest entries naming the old paths, would contradict the canonical-path test. A module with anything besides the forwarding call keeps the directory rule (tested). The manifest'sKERNEL_DIRECTORIEScomment also says whymegatron/core/opsis not a kernel directory;CODEOWNERSline keeping@NVIDIA/transformeronops/norm/torch_norm.py, and the re-sorted allowlist key intools/process_group_usage_allowlist.json;VocabParallelCrossEntropy,FusedLayerNorm,WrappedTorchNormandL2Norm; the package export and the selector's old import path; the forwarder rule of the coverage tool; thetorch_norm.pypath in thetest_fused_activations.pydocstring;isorton the changed Python files andblackon those undermegatron/coreandtests.Verify the move:
git diff -M -C --find-copies-harder 577586474~1 577586474shows the moved modules asC092–C099. Two have one import line changed;fused_layer_norm.pyandtorch_norm.pycarry the edits above.ops_move_loss_norm.pyTesting
Run locally on CPU with a Triton stub:
tests/unit_tests/opsandtests/unit_tests/determinism/kernels/test_manifest.py: 385 passed, 7 skipped. Five skipped tests cover the SSM move and run once Move SSM helpers, Mamba2, GDN/GDN2 and GDP into megatron/core/ops with deprecated aliases (2/4) #7453 lands; the other two are the same-value write tests for a forwarder with several targets, which refuses writes.monkeypatchchooses how to undo from the target's__dict__. With the follow-up fixes reverted, the extended tests fail on all three reported cases: a nested patch that sets the original back, a private spec name, and a first write of the value a name already holds.test_backend_provider_contracts.py,tensor_parallel/test_cross_entropy.py,fusions/test_fused_cross_entropy.py,test_kernel_warmup.py,test_mamba_layer.py, and the determinism kernel tests for cross entropy and norm) give identical results on this branch and onmain, plus one new test of the coverage tool's forwarder rule, which passes. What does not pass needs NCCL or a GPU.megatron.core.tools/check_kernel_determinism_coverage.py: no unregistered kernel files. It asks for thedeterminism-exemptlabel because import lines change in registered files (extensions/transformer_engine.py,transformer/multi_latent_attention.py).tools/check_process_group_usage.pyfails onmainitself. On this branch its report is identical.The GPU suites run in CI.
Issue tracking
For PRs from open-source community contributors:
Linked issue: Related to #5438
Contribution process
Pre-checks
jit_fuser/torch.compile, CUDA extension, TE or external-library dispatch, or a scatter/index accumulation), I have added or updated its bit-exact determinism test and registered it intests/unit_tests/determinism/kernels/manifest.py(guide)🤖 Generated with Claude Code