Repository navigation
feat(models): add native Nemotron-H implementation - #3426
Conversation
f9fe0ec to
8b4cfd9
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6c97e62. Configure here.
garrett361
left a comment
There was a problem hiding this comment.
LGTM, just minor comments. I had forgotten that nemotron is nope.
|
|
||
|
|
||
| class NemotronHConfig(PretrainedConfig): | ||
| """Configuration for NemotronH (Nemotron-3-Super-120B-A12B) hybrid Mamba-Transformer-MoE model. |
There was a problem hiding this comment.
Moving to dropping the config doc strings everywhere? Would be happy to have one fewer thing to keep in sync.
| @property | ||
| def layer_types(self) -> list[str]: | ||
| return self.layers_block_type |
| base_model_prefix = "model" | ||
| supports_gradient_checkpointing = True | ||
| _no_split_modules = ["NemotronHMambaLayer", "NemotronHMoELayer", "NemotronHAttentionLayer"] | ||
| _no_split_modules = ["NemotronHDecoderLayer"] |
There was a problem hiding this comment.
is anything actually using _no_split_modules anymore? Not a question specific to this PR.
| _tp_plan = {"lm_head": "colwise_gather_output"} | ||
| _pp_plan = {"lm_head": (["hidden_states"], ["logits"])} |
There was a problem hiding this comment.
similar non-nemotron-specific question: are we using these anywhere?
|
|
||
| def set_context_parallel_attributes(self, cp_group: dist.ProcessGroup, cp_rank: int, cp_world_size: int) -> None: | ||
| self.model.set_context_parallel_attributes(cp_group, cp_rank, cp_world_size) | ||
| def set_context_parallel_attributes( |
There was a problem hiding this comment.
nit: we have a lot of very similar set_context_parallel_attributes type functions throughout, would be nice to simplify this (nemotron and beyond)
|
|
||
| first_output = mamba(first, cu_seqlens) | ||
| second_output = mamba(second, cu_seqlens) | ||
| torch.testing.assert_close(first_output[:, 3:], second_output[:, 3:]) |
There was a problem hiding this comment.
worth testing the grads match where we expect them to, also? maybe not
There was a problem hiding this comment.
I think not, and actually don't know how much we need this test. More of a test of fla.modules.conv than anything else, which isn't our responsibility (though of course good to know if broken). Up to you.
Removes duplicate and unused entries from `pyproject.toml` (rev-infra P9). The set of installed packages does not change for any extra combination (checked below). | Removed | Reason | |---|---| | second `orjson>=3.10` in `dependencies` | Duplicate of `orjson>=3.11.0` a few lines above. | | `pandas>=2.0` | Nothing imports it: no `import pandas` / `pd.` in `src/`, `tools/`, `scripts/`, `tests/`, `examples/`, `skills/`, `packages/`. It was added for the nano bridge (7089ba1). It stays installed because `datasets` depends on it. | | `pyarrow>=21.0.0` | No direct use anywhere in those directories. The one parquet writer, `tools/convert_traces_to_hf_dataset.py`, calls `datasets.Dataset.to_parquet`, and `datasets` depends on `pyarrow` itself. It stays installed. | | `nixl-cu13==0.10.1` in the `disagg` extra | `disagg` already lists `nixl`, and the `[[tool.uv.dependency-metadata]]` override for `nixl` 0.10.1 requires exactly `nixl-cu13==0.10.1`. The `gpu` extra keeps its explicit pin, because `transports/weights/nixl/agent.py` imports `nixl_cu13` directly. | | `mamba-ssm` dependency group | Duplicate of the `gpu` extra entry, which was added with the native Nemotron-H model (#3426). Every user of the group also syncs `--extra gpu` or `--all-extras`, so I dropped `--group mamba-ssm` from `Dockerfile.cuda` (2 places), `scripts/docker-entrypoint.sh` and the `pre_run_command` in `configs/advanced/nemotron-3-super/swe.toml`. The `[tool.uv.sources]` git pin and the build dependencies for mamba-ssm stay. | Behavior change: - None for installs: same locked packages and versions. - `uv sync --group mamba-ssm` no longer works; use `--extra gpu`, which every repo caller already passes. - `pandas` and `pyarrow` lose their direct lower bounds. They are still pulled in by `datasets`. ## Checks - `uv lock`: resolves 564 packages, the same set as before (568 `[[package]]` entries before and after). `uv lock --check` passes. The `uv.lock` diff only touches prime-rl's own metadata. I reverted the reordering of `[options.exclude-newer-package]` that my uv version (0.12.10) introduced. - I ran `uv export --frozen --all-packages` and compared the package lists before and after, ignoring the `# via` comment lines. They are identical for: no extras, `--extra disagg`, `--extra gpu`, `--extra dashboard`, `--all-extras`, and `--all-extras --all-groups`. `--extra disagg` still yields `nixl==0.10.1` and `nixl-cu13==0.10.1`, and `--extra gpu` still yields `mamba-ssm` at the pinned rev. - No Python changes, so no unit tests are affected. GPU: not needed. The Docker image build in CI covers the Dockerfile and entrypoint edits. ## Conflicts Text conflicts only in `pyproject.toml` / `uv.lock`, as with any dependency PR. None of the open PRs that touch these files (e.g. #3829, #3803, #3767, #3660) edit the lines removed here. A re-lock after rebase is enough. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Metadata and sync-flag cleanup only; lockfile exports are unchanged and runtime installs should match prior behavior. > > **Overview** > Cleans up **duplicate and redundant dependency declarations** in `pyproject.toml` and aligns `uv.lock`, Docker, and Slurm sync commands with the slimmer manifest. **No intended change to installed packages** for existing extra combinations (`gpu`, `disagg`, `--all-extras`, etc.). > > **Removed from core `dependencies`:** duplicate `orjson>=3.10`, unused direct pins on `pandas` and `pyarrow` (they still come in via `datasets`). **Removed from `disagg` extra:** explicit `nixl-cu13==0.10.1` because `nixl` metadata already pulls that version; the **`gpu` extra keeps** its explicit `nixl-cu13` pin for direct imports. **Dropped the `mamba-ssm` dependency group** as a duplicate of the `gpu` extra entry—`Dockerfile.cuda`, `scripts/docker-entrypoint.sh`, and the Nemotron SWE `pre_run_command` no longer pass `--group mamba-ssm`. > > **Workflow note:** `uv sync --group mamba-ssm` alone is no longer supported; use `--extra gpu` (or `--all-extras`), which callers already use. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit f0f2ff6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->

Nemotron-H now runs as a native PyTorch module through PrimeRL Mamba-2, shared attention, and unified MoE layers. Both Nano (regular MoE) and Super (latent MoE) layouts are retained. Packed convolution and SSD reset at document boundaries, and Mamba owns the full-sequence/head redistribution required by Ulysses context parallelism. Checkpoint conversion maps official per-expert and latent-expert weights into the canonical runtime layout.
Updated against main (
daa72a91d), retaining current fusion, normalization, and CP interfaces. Restored LoRA adaptermixernames and installed Mamba SSD through the GPU extra. The pinned upstream source uses Triton without requiring the optional selective-scan CUDA extension. Configuration guards and HF execution comparisons were removed; existing packed-boundary tests exercise real GPU kernels. Router compute followstrainer.model.moe_router_dtype(float32by default, optionallybfloat16); the model does not force an override.NemotronHModelandNemotronHForCausalLMboth inheritNemotronHPreTrainedModel, following the existing pattern used by other architectures. The shared Nemotron base retains the config type and architecture-specific conversion/CP hooks. Unused TP/PP plans, no-split metadata, checkpointing flags, redundant attributes, and obsoleteuse_mamba_kernelshandling are removed. Shared model construction, LoRA, LM-head patching, and weight-transfer code remain unchanged. Broader removal of the model abstractions is deferred to a follow-up PR.Garrett's config-docstring and unused-HF-attribute comments are addressed. The broader context-parallel setter refactor and additional gradient tests remain deferred.
Validation
All GPU validation used Slurm-allocated Hopper H200s. Current revision
f6d7a4033restores the shared Nemotron base while retaining the attribute cleanup. Ruff lint/format checks, factory construction/dtype/inherited-hook checks, and all 15 existing model tests passed (Slurm job 343).uv sync --all-extras --all-packages, Ruff lint, and format checks passed.The 20-step run below was completed at
a7f798048(Slurm job 339), before restoring the original shared base. The preceding revision00746d6d2was also validated: direct inheritance and the original factory passed construction checks; compiled two-GPU Mamba and fused-QKV selective-AC checks passed again (job 341). All 15 targeted unit tests passed (job 340). The two-step end-to-end Nano run completed (job 342), with mismatch KL 0.00475226 and 0.00522814, finite gradients, and three successful NCCL weight updates including startup. Compiled Mamba/selective-AC and eight checkpoint-resume probes also passed again on this code (jobs 337–338). The end-to-end run usednvidia/NVIDIA-Nemotron-3-Nano-30B-A3B-BF16on eight Slurm-allocated H200s: four trainer GPUs with EP=4 and four inference replicas. The native trainer used FlashAttention 3, compilation, selective activation checkpointing, and NCCL weight updates. Reverse-text used batch size 64, group size 8, sequence length 1024, and maximum completion length 512. Optimization and reduction dtypes retained their defaults.Launched with
uv run rl @ rl-module-cleanup.toml. The config setstrainer.env_vars.TRITON_CACHE_DIRto node-local/tmpto avoid shared-home artifact read failures; no diagnostic hooks are present in this run.The automated review requests a math-environment table. This run uses reverse-text and does not satisfy that environment-specific requirement: the current math task requires a policy-aware container runtime, which is unavailable on the validation nodes.
20/20 steps completed, with 21 successful NCCL weight reloads (startup plus every update). Mismatch KL: mean 0.0059826873, maximum 0.0076148757, final 0.0071559078. All step KL values and gradient norms were finite. Super latent behavior was exercised by small model tests and distributed probes; no full-size Super run was performed.
CI at
a7f798048: Ruff, slim install, and CodeQL passed; remaining jobs were running or queued at the last check. On the previous commit (a60d27578): all 15 Nemotron tests passed. The GPU unit job had five failures in unchanged DeepSeek-V4 tests (packed-batch assertions and unsupported FP8 kernels on that runner); CPU unit tests, Ruff, slim install, and CodeQL passed.Note
High Risk
Large rewrite of a hybrid MoE/Mamba training path, checkpoint layout, and distributed Mamba CP, with dependency pins that affect the whole GPU stack.
Overview
Replaces the Hugging Face–wrapped Nemotron-H stack with a native hybrid model: one
NemotronHDecoderLayerper block type (Mamba, attention, MoE), a first-partyNemotronHMamba2that uses FLA packed causal conv plusmamba_ssmTriton SSD, and Ulysses CP via sequence↔head all-to-all inside the Mamba module (the sharedcp_mambahelper is deleted).Checkpoint and config:
NemotronHConfigis realigned to current HF field names andlayer_types; conversion is explicit per layer type instead of inferring from key presence (the old router-biasMapValueshift is gone). Install/docs now pullmamba-ssmfrom a pinned git rev through the gpu extra, withtilelang==0.1.12pinned for vLLM compatibility; trainer init no longer disablesuse_mamba_kernelsor monkey-patches HF Mamba.Tests drop HF parity checks and instead exercise native packed boundaries, CP wiring on
NemotronHMamba2, and existing KL/conversion coverage.Reviewed by Cursor Bugbot for commit f6d7a40. Bugbot is set up for automated code reviews on this repo. Configure here.