Repository navigation
Bump FLA to 0.5.1 and integrate GDN2 kernels - #5765
Conversation
|
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. |
|
/claude review |
There was a problem hiding this comment.
Light review — one inline nit and two items to double-check.
1. uv.lock not updated. pyproject.toml bumps flash-linear-attention from ~=0.4.0 to flash-linear-attention[cuda]>=0.5.1, but uv.lock is not part of this PR. Per the contribution workflow, uv.lock must be regenerated (uv lock inside the container) when pyproject.toml dependencies change, otherwise uv sync --locked fails in CI. Please confirm the lock file is committed (it may just be missing from the diff view).
2. chunk_gdn2 not reset to None on import failure — see inline comment on the except ImportError block.
Otherwise the change looks clean and well-tested: test_module_construction, test_gpu_forward_backward, the parametrized _prepare_input_for_gated_delta_rule coverage, and the dedicated test_parallel_gated_delta_net2_correctness give good coverage of the new GDN2 path across TP/SP/CP.
|
/claude review |
|
The refactored method returns a single dict that includes the output gate under the key # tests/unit_tests/ssm/test_gated_delta_net.py:352
kernel_inputs, gate_out = gdn._prepare_input_for_gated_delta_rule(
qkv, gate, gate_feats, A_log_mock, dt_bias_mock, batch, seq_len
)Unpacking a dict into two names raises Suggested reconciliation to match the implementation (which kernel_inputs = gdn._prepare_input_for_gated_delta_rule(
qkv, gate, gate_feats, A_log_mock, dt_bias_mock, batch, seq_len
)
gate_out = kernel_inputs["gate"]
assert set(kernel_inputs.keys()) == expected_keys | {"gate"}Relatedly, the docstring is inaccurate: the signature is |
|
/claude review |
|
/ok to test b22c5cf |
| from fla.modules.convolution import causal_conv1d | ||
| from fla.modules.l2norm import l2norm | ||
| from fla.ops.gated_delta_rule import chunk_gated_delta_rule | ||
| from fla.ops.gdn2.chunk import chunk_gdn2 |
There was a problem hiding this comment.
The matching except ImportError block (lines 51-56) sets causal_conv1d, l2norm, and chunk_gated_delta_rule to None, but does not set chunk_gdn2 = None. If FLA is installed but too old to have fla.ops.gdn2.chunk (or FLA is absent entirely), this import fails and chunk_gdn2 is never bound. The GDN2 guard in __init__ (assert chunk_gdn2 is not None) then raises NameError: name 'chunk_gdn2' is not defined instead of the intended friendly AssertionError message about requiring flash-linear-attention >= 0.5.1.
Please add chunk_gdn2 = None to the except ImportError block:
except ImportError:
causal_conv1d = None
l2norm = None
chunk_gated_delta_rule = None
chunk_gdn2 = None
HAVE_FLA = FalseThere was a problem hiding this comment.
Light review — the changes add a GDN2 variant to the gated delta net layer, with good test coverage (parametrized construction, forward/backward, correctness, and jit-helper tests for both variants).
One correctness issue flagged inline: the new chunk_gdn2 import isn't defaulted to None in the except ImportError block, so the GDN2 availability guard would raise NameError instead of the intended AssertionError when FLA is missing or too old.
Everything else looks consistent — config plumbing, FLOPs accounting, and sharded_state_dict all handle the new variant symmetrically.
b22c5cf to
3aec018
Compare
santhnm2
left a comment
There was a problem hiding this comment.
Approved for transformer changes
| if self.experimental_attention_variant in ("gated_delta_net", "gdn2"): | ||
| # gdn2 may also be enabled for GDN layers built via the hybrid layer pattern | ||
| # symbol 'G', where linear_attention_freq is unused; the GPT experimental | ||
| # attention route raises a clear error downstream if it is missing. | ||
| if self.experimental_attention_variant == "gated_delta_net": | ||
| assert ( | ||
| self.linear_attention_freq is not None | ||
| ), f"linear_attention_freq must be set for linear gated_delta_net." |
There was a problem hiding this comment.
I think it's ok to keep because the GPT experimental attention path is still active even if no new features are being added; we can remove if and when the GPT path is deleted entirely
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
|
/ok to test 63aebd9 |
Signed-off-by: Xuanteng Huang <xuantengh@nvidia.com>
|
/ok to test a5051b2 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31445864937 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31447097726 |
What does this PR do?
_GDNBaseConvergence: see the wandb run.
Fixes #4955