fix(scatter_add): accumulate duplicate destination indices via unique_indices flag - #11
Open
ryan-williams wants to merge 1 commit into
Open
Conversation
…ices flag The tile-wide gather/accumulate/scatter path is last-write-wins for repeated destination indices within a 128-row tile, silently dropping all but one contribution. This is the natural embedding-gradient backward, where repeats (common tokens, BOS/EOS) are ubiquitous. The existing test never caught it: its input generator builds per-tile permutations, so tiles are unique by construction. Add unique_indices: bool = True. When False, k_tile_size=1 makes each index its own sequential tile, so nl.sequential_range accumulates duplicates correctly. Mirror the kwarg on scatter_add_torch_ref (harness signature parity) and add a dup-index regression test (sampled with replacement, within- and cross-tile).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
scatter_addsilently drops contributions whenindexrepeats a destination row inside thesame 128-row tile. That is the common case for embedding gradients, so
grad_table.scatter_add_(0, token_ids, grads)currently returns wrong gradients for anybatch containing a repeated token.
This PR adds an opt-in
unique_indices=Falsepath that accumulates correctly. The default isunchanged, so existing callers and the existing fast path are untouched.
The bug
scatter_adddoes a tile-wide gather → add → scatter over 128-row tiles. Within a tile thepre-image is gathered once, so duplicate destination rows all read the same pre-image and the
scatter back is last-write-wins — every duplicate contribution but one is discarded, with no
error.
index=[0, 0, 1, 2, 2, 2],src=ones:scatter_addtorch.scatter_add_The docstring noted the precondition in passing ("indices within a tile of 128 rows should be
unique for correctness"), but nothing enforced or detected the violation.
Why the existing test does not catch it:
test_scatter_add.py's input generator buildsper-tile permutations (
np.random.permutation(bs_slen)), so every tile is unique byconstruction and the duplicate path is never exercised.
The fix
Add
unique_indices: bool = True. WhenFalse, setk_tile_size = 1so each index becomes itsown sequential tile;
nl.sequential_rangethen makes each add observe prior writes, soduplicates accumulate.
Truepreserves today's behavior and performance — no existing caller changes.unique_indices=Falseis correct but slower (1 row per tile instead of 128). We chose theminimal change that makes correct results reachable; a faster approach (sorted segments,
atomics) would be a better long-term fix if you want one, and we would be happy to follow
your lead there.
scatter_add_torch_reffor test-harness signature parity.Tests
Adds
test/integration/nkilib/experimental/misc/test_scatter_add_dup_indices.py, following theconventions of the existing
test_scatter_add.py(sameOrchestrator/UnitTestFramework/torch_ref_wrapperstructure), with indices sampled with replacement so duplicates occurboth within and across tile boundaries:
bs_slendim_sizesrc_rowsfloat32bfloat16float32float32Validation
--test-mode simulation): existingtest_scatter_add.pyplus the newdup-index tests, 10 passed / 4 skipped.
trn2.3xlarge,neuronx-cc2.26.6360.0): the fixed kernel called fromJAX accumulates duplicate indices correctly, matching a reference implementation.
jax.custom_vjp(NKIgatherforward + this kernel backward), which differentiates correctly under
jax.gradon device. Withthe stock kernel the same test produces incorrect gradients on batches with repeated tokens.
Notes
Found during Trainium bring-up for a JAX/Equinox training stack, where the embedding backward is
the first place duplicate indices show up. Happy to adjust naming, defaults, or the test layout to
match your conventions.