fix: scope get_full_cu_seqlens cache key by device and inference mode - #2728
Conversation
Signed-off-by: Dongmin Ra <dongmin.ra@navercorp.com>
for more information, see https://pre-commit.ci
Greptile SummaryThis PR fixes a cache-key collision in Confidence Score: 5/5Safe to merge — the fix is minimal, targeted, and well-tested with no regressions introduced. The change is a one-liner key extension with clear semantics. torch.device is hashable and comparable by value, and torch.is_inference_mode_enabled() is a stable API. The two new tests cover exactly the described failure scenarios. No P0 or P1 issues were found. No files require special attention. Important Files Changed
Reviews (10): Last reviewed commit: "Merge branch 'main' into fix/get_full_cu..." | Re-trigger Greptile |
|
@cyanguwa When you have a moment, could you please take a look at this PR? Thanks:) |
|
@cyanguwa This PR is pretty straightforward. Would you mind taking a quick look? Thank you:) |
|
@cyanguwa Hi:) could you look into this PR? thank you. |
|
@ptrendx The review hasn’t been progressing—would it be possible to change the reviewer? |
|
/te-ci torch L1 |
cyanguwa
left a comment
There was a problem hiding this comment.
Thanks for the PR and sorry about the delay in reviewing! I'll run the CI and merge it. Will make another small PR to properly integrate the new test to our qa/ scripts later.
…NVIDIA#2728) * fix: scope get_full_cu_seqlens cache key by device and inference mode Signed-off-by: Dongmin Ra <dongmin.ra@navercorp.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Dongmin Ra <dongmin.ra@navercorp.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
…NVIDIA#2728) * fix: scope get_full_cu_seqlens cache key by device and inference mode Signed-off-by: Dongmin Ra <dongmin.ra@navercorp.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Signed-off-by: Dongmin Ra <dongmin.ra@navercorp.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Charlene Yang <8636796+cyanguwa@users.noreply.github.com>
The file has two tests. test_cu_seqlens_cache_isolated_across_devices_for_forward needs two CUDA devices and therefore always skips in L0, which is where the file was just wired in. That test is the actual regression guard for NVIDIA#2728 - the cu_seqlens cache key not being scoped by device - so leaving it permanently skipped defeats the purpose of connecting the file at all. L1 is the only suite that guarantees more than one GPU. It is a plain pytest run, no torchrun, matching how attention/test_cp_utils.py is invoked there. Cost is about 2 s. Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com>
…broken one (#3287) * [PyTorch] Fix the selective activation checkpointing test tests/pytorch/layernorm_mlp/test_selective_activation_checkpoint.py fails on all 16 parametrizations on main. The failure is in the test, not in the feature: outputs and all six parameter gradients are bit-exact between the checkpointed and the non-checkpointed path, and checkpointing does save memory. The test asserted `ln_fwd_mem > 6 * sln_fwd_mem`. That threshold was never reachable, because the ratio is fixed by the test's own configuration rather than by anything in TE. Both peaks are derivable and the derivations reproduce the measurements exactly: ln_fwd_mem = layers * (2*s*f + 2*s*h + 2*s) * itemsize sln_fwd_mem = ((layers+1)*s*h + 2*s*f + 2*s) * itemsize For `small` @ 128 this predicts 7876608 B and 1377280 B; measured 7876608 B and 1377280 B. With f = 4h and layers = 12 the ratio is 120/21 = 5.714, matching the measured 5.715. Reaching 6 would need a different model shape, e.g. layers = 16 gives 160/25 = 6.4. The tensor lists in both branches of _LayerNormMLP._forward are unchanged since the test was added, so this is not a regression. - Assert on the memory that recompute actually frees - fc1_out and act_out, derived from the model config - instead of the ratio. The checkpointed peak still holds the transient of one layer, so the expectation covers layers - 1. This keeps the assertion independent of layer count and model shape. - Check outputs and gradients before the memory check. Previously a numerical regression would surface as a memory-ratio failure and the correctness comparison would never run, which is exactly what happens on main today. - Drop `assert ln_bwd_time < sln_bwd_time`. The margin is as low as 13% on an idle GPU (huge @ 128: 11.2 ms vs 12.8 ms), which makes it a CI flake. - Skip parametrizations that do not fit in device memory. large @ 65536 and huge @ 65536 need more than 32 GiB for the non-checkpointed model alone and raise OutOfMemoryError on 48 GiB cards. Verified on RTX 5880 Ada: 12 passed, 4 skipped, was 16 failed. Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com> * [CI] Connect orphaned pytorch test files to qa Six pytorch test files are not referenced by any script in qa/ and have therefore never run in CI: test_qk_norm.py test_float8_current_scaling_exact.py attention/test_cu_seqlens_cache.py test_nvfp4_fsdp2_hooks.py test_fused_router_perf.py layernorm_mlp/test_selective_activation_checkpoint.py `git log -S<name> --all -- qa/` shows none of them was ever added and later removed, and none of the PRs that introduced them touched qa/. They were simply never wired up. All are single-GPU and self-skip on unsupported hardware, so they belong in L0. test_float8_current_scaling_exact.py marks its classes with skipif(not fp8_available), test_nvfp4_fsdp2_hooks.py requires sm_100+, and the one multi-device case in test_cu_seqlens_cache.py checks device_count() first. - Add entries in L0_pytorch_unittest for test_qk_norm.py, test_float8_current_scaling_exact.py, attention/test_cu_seqlens_cache.py and layernorm_mlp/test_selective_activation_checkpoint.py. - Move test_nvfp4_fsdp2_hooks.py into tests/pytorch/nvfp4/, which L0 already runs as a whole directory. It has no local imports, so the move is inert. - Also run attention/test_cu_seqlens_cache.py in L1. Its cross-device test needs two GPUs and would otherwise stay permanently skipped, which would defeat the point of connecting the file - that test is the regression guard for #2728. test_fused_router_perf.py is left out. It is gated behind TE_RUN_PERF_TESTS and, despite its name, has no perf assertions at all - only torch.testing.assert_close on correctness, with timings going to record_property. Ungating the correctness half is worth doing but deserves its own decision. Measured on RTX 5880 Ada: 45 passed, 5 passed, 1 passed + 1 skipped, and 16 skipped respectively, about 13 s of added L0 time. Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com> --------- Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com>
Description
Fixed an issue where the cu_seqlen tensor was incorrectly retrieved from the cache.
(batch_size, max_seqlen)were used as the cache key when retrieving cu_seqlens.(batch_size, max_seqlen)is used.Type of change
Changes
Checklist: