Make param_index_map always use unpacked (full numel) offsets - #4328
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. |
|
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. |
| param_world_start, | ||
| param_world_end, | ||
| _, | ||
| ) in buffer.param_index_map.items(): |
There was a problem hiding this comment.
Note to self: ask Qiyu about this line (previously was using packed indices, I think it should be unpacked indices).
1ab944e to
9f5a33b
Compare
|
/claude review |
There was a problem hiding this comment.
LGTM — the refactoring is consistent: param_index_map now always stores unpacked offsets, packed offsets are correctly isolated in nvfp4_packed_param_index_map, and all callers (including distrib_optimizer.py) are updated. The removal of get_unpacked_index_map() is clean.
Minor note: tests/unit_tests/distributed/test_param_and_grad_buffer.py has no NVFP4-specific test cases, so this refactoring is not directly exercised by unit tests. Consider adding a test that validates the index maps for a buffer with NVFP4 params (e.g., verifying that param_index_map stores unpacked offsets and nvfp4_packed_param_index_map stores packed offsets).
|
/claude review |
There was a problem hiding this comment.
Good refactoring — the semantics of param_index_map are much cleaner now, and the test coverage is solid.
One test coverage gap: all NVFP4 test cases either mark all params as NVFP4 or none. In practice, NVFP4 buffers often contain a mix (e.g., NVFP4 linear weights alongside bf16 layernorm/embedding weights). A mixed test would exercise the code path where packed and unpacked offsets diverge differently for different params in the same buffer.
The _make_buffer helper already supports this via nvfp4_param_indices. A test like this would fill the gap:
def test_nvfp4_mixed_params(self):
"""Test buffer with a mix of NVFP4 and non-NVFP4 params."""
param_shapes = [
('linear.weight', (100, 100)), # NVFP4
('layernorm.weight', (100,)), # non-NVFP4 (bf16)
]
buffer, params = self._make_buffer(param_shapes, nvfp4_param_indices={0})
# Non-NVFP4 param should have same span in both maps.
packed_start, packed_end, _ = buffer.nvfp4_packed_param_index_map[params[1]]
unpacked_start, unpacked_end, _ = buffer.param_index_map[params[1]]
assert packed_end - packed_start == unpacked_end - unpacked_start == 100
# NVFP4 param should have half the span in packed map.
packed_start, packed_end, _ = buffer.nvfp4_packed_param_index_map[params[0]]
unpacked_start, unpacked_end, _ = buffer.param_index_map[params[0]]
assert packed_end - packed_start == 5000 # numel // 2
assert unpacked_end - unpacked_start == 10000 # full numel6235b88 to
289c8dc
Compare
|
/claude review |
|
/claude review |
ee8b4ce to
40fd5b7
Compare
40fd5b7 to
4faad48
Compare
|
/claude review |
4faad48 to
a9e14c1
Compare
a9e14c1 to
9fe2c5b
Compare
- param_index_map now always stores full-numel offsets (not packed) - Add nvfp4_packed_param_index_map for packed offsets (NVFP4 only) - self.numel / self.numel_unpadded always refer to full numel - Add self.nvfp4_packed_numel / self.nvfp4_packed_numel_unpadded - Remove self.grad_numel (always equals self.numel) - Remove get_unpacked_index_map() (param_index_map is used directly) - Unify bucket metadata helpers: _update_bucket_metadata takes args instead of nonlocal, _finalize_bucket wraps both main + packed - Extract _create_bucket to deduplicate second loop - Bucket splitting now uses full numel consistently - Update distributed optimizer to use param_index_map directly - Add NVFP4 index map unit tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9fe2c5b to
fd8ee03
Compare
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/24613605144 |
* origin/main: (286 commits) Rename MambaModel/MambaStack to HybridModel/HybridStack (NVIDIA#4099) Fix Megatron initialization with extra_args_provider (NVIDIA#4327) Fix RL to once again work with --skip-train (NVIDIA#4249) Add activation logging and tokens per expert logging (NVIDIA#3842) Make param_index_map always use unpacked (full numel) offsets (NVIDIA#4328) FA4 Inference (NVIDIA#4186) Fix RL reward due to stop token (NVIDIA#4096) cp: Fix UT timeout (NVIDIA#4310) (NVIDIA#4373) feat(ckpt): add --async-ckpt-use-cpu-shm argument (NVIDIA#4355) Update copy-pr-bot.yaml [skip ci] Docs: improve docstrings and comments in example training loop (NVIDIA#4041) Add QK layernorm support for dot-product attention in MambaModel (NVIDIA#4067) Fix bug with non-partial rollouts (NVIDIA#3964) [docs] ci: use parent-relative json_url for version picker (NVIDIA#4367) Add tables and histogram for RL staleness (NVIDIA#4097) Port DeepSeek Sparse Attention to `MambaModel` (NVIDIA#3553) docs: bump versions1.json to 0.17.0 (latest) (NVIDIA#4360) Fix potential coredump issue that occurs when saving a checkpoint (NVIDIA#1871) ci(gb200): add 1-node mr-github functional test variants (NVIDIA#4334) fix: wait for async P2P send before deallocating output tensor (NVIDIA#4047) ... # Conflicts: # megatron/core/transformer/cuda_graphs.py
…#4328) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…#4328) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: yhgalaxy <yhgalaxy@outlook.com>
…#4328) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jon Barker <jbarker@aws-cmh-slurm-1-vscode-02.cm.cluster>
…#4328) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
For NVFP4 buffers, param_index_map previously stored packed offsets (numel // 2). This was inconsistent with non-NVFP4 buffers and required callers to use get_unpacked_index_map() to get full-numel offsets.
Now param_index_map always stores unpacked offsets regardless of buffer type. The packed offsets are stored in nvfp4_packed_param_index_map, which is the NVFP4-specific concern. This also removes the now-trivial get_unpacked_index_map() method.