Fix per-feature boundary lost by ZCH index dedup (#4525) - #4525
Open
yingufan wants to merge 1 commit into
Open
Conversation
Contributor
|
@yingufan has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115457342. |
yingufan
force-pushed
the
export-D115457342
branch
from
August 17, 2026 00:25
2240557 to
3ac3a76
Compare
yingufan
added a commit
to yingufan/torchrec
that referenced
this pull request
Aug 17, 2026
Summary: `fbgemm.jagged_unique_indices` dedups a whole hash range at once, and `_create_dedup_indices` puts every feature of a table into one range, so the op returns the group's total unique count spread evenly over the group's (feature, batch) slots rather than the real per-feature counts. A table binding a single feature is unaffected. A table binding two features of very unequal length gets ids relabeled across the boundary. Managed collision modules read the feature an id arrives under as a control signal, so this is a correctness bug rather than just wrong bookkeeping. On IG Reels ESR the `media_embbedding_cache` HASH_ZCH table binds a ~33 long write feature and a ~1280 long `_readonly` history feature; both come back as ~656, so roughly 620 history ids per step are relabeled as candidates and inserted past the read-only gate. The table reaches 100% occupancy within ~100 steps and, with eviction disabled, only collides from then on. Re-attribute every surviving row to the first feature it appeared in and regroup the values so each feature is contiguous again. Rows shared by two features still collapse into one, so the dedup saving is kept. Gated on a table actually binding more than one feature, so the common path is untouched. Only per-feature totals are made exact: the layout within a feature stays approximate, as it already was, because reverse_indices undoes it downstream. Also declares the `reverse_indices` field that `_dedup_indices` already appends to. `mc_modules` shadows `EmbeddingCollectionContext` with a local dataclass that lacks it, so a standalone sharded `ManagedCollisionCollection` with dedup on raised `AttributeError`; only the `ManagedCollisionEmbeddingCollection` path worked, since that context derives from the `embedding.py` class instead. Differential Revision: D115457342
yingufan
force-pushed
the
export-D115457342
branch
from
August 19, 2026 01:22
3ac3a76 to
9c3f888
Compare
yingufan
added a commit
to yingufan/torchrec
that referenced
this pull request
Aug 19, 2026
Summary: When a managed collision table has more than one feature, index dedup can move ids from one feature to another. All features of a table share one hash range (see `_create_dedup_indices`), so `fbgemm.jagged_unique_indices` dedups them together. The op cannot tell which feature each surviving id came from, so it just splits the total evenly over the table's (feature, batch) slots. That is fine for a table with one feature. With two features of very different lengths it is not: a 33 long feature and a 1280 long feature both come back as roughly 656, so ids end up under the wrong feature. This is a correctness problem and not just wrong bookkeeping, because ZCH decides whether to insert an id from the name of the feature it arrived under: a feature whose name ends in the read-only suffix is looked up but never written. Ids moved to the wrong feature get written when they should not be. The fix walks each surviving id back to the first feature it appeared in, regroups the values so each feature is contiguous again, and recounts. Ids sent by both features still collapse into one row, so we keep the dedup saving. It only runs when a table actually has more than one feature, so the common path is unchanged. Also declares the `reverse_indices` field that `_dedup_indices` already appends to. `mc_modules` has its own local `EmbeddingCollectionContext` that was missing it, so sharding a `ManagedCollisionCollection` on its own with dedup enabled raised `AttributeError`. It only worked through `ManagedCollisionEmbeddingCollection`, whose context comes from `embedding.py`. Differential Revision: D115457342
Summary: When a managed collision table has more than one feature, index dedup can move ids from one feature to another. All features of a table share one hash range (see `_create_dedup_indices`), so `fbgemm.jagged_unique_indices` dedups them together. The op cannot tell which feature each surviving id came from, so it just splits the total evenly over the table's (feature, batch) slots. That is fine for a table with one feature. With two features of very different lengths it is not: a 33 long feature and a 1280 long feature both come back as roughly 656, so ids end up under the wrong feature. This is a correctness problem and not just wrong bookkeeping, because ZCH decides whether to insert an id from the name of the feature it arrived under: a feature whose name ends in the read-only suffix is looked up but never written. Ids moved to the wrong feature get written when they should not be. The fix walks each surviving id back to the first feature it appeared in, regroups the values so each feature is contiguous again, and recounts. Ids sent by both features still collapse into one row, so we keep the dedup saving. It only runs when a table actually has more than one feature, so the common path is unchanged. Also declares the `reverse_indices` field that `_dedup_indices` already appends to. `mc_modules` has its own local `EmbeddingCollectionContext` that was missing it, so sharding a `ManagedCollisionCollection` on its own with dedup enabled raised `AttributeError`. It only worked through `ManagedCollisionEmbeddingCollection`, whose context comes from `embedding.py`. Differential Revision: D115457342
yingufan
force-pushed
the
export-D115457342
branch
from
August 19, 2026 18:46
9c3f888 to
85a09e0
Compare
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.
Summary:
When a managed collision table has more than one feature, index dedup can move
ids from one feature to another.
All features of a table share one hash range (see
_create_dedup_indices), sofbgemm.jagged_unique_indicesdedups them together. The op cannot tell whichfeature each surviving id came from, so it just splits the total evenly over the
table's (feature, batch) slots. That is fine for a table with one feature. With
two features of very different lengths it is not: a 33 long feature and a 1280
long feature both come back as roughly 656, so ids end up under the wrong
feature.
This is a correctness problem and not just wrong bookkeeping, because ZCH decides
whether to insert an id from the name of the feature it arrived under: a feature
whose name ends in the read-only suffix is looked up but never written. Ids moved
to the wrong feature get written when they should not be.
The fix walks each surviving id back to the first feature it appeared in,
regroups the values so each feature is contiguous again, and recounts. Ids sent
by both features still collapse into one row, so we keep the dedup saving. It
only runs when a table actually has more than one feature, so the common path is
unchanged.
Also declares the
reverse_indicesfield that_dedup_indicesalready appendsto.
mc_moduleshas its own localEmbeddingCollectionContextthat was missingit, so sharding a
ManagedCollisionCollectionon its own with dedup enabledraised
AttributeError. It only worked throughManagedCollisionEmbeddingCollection, whose context comes fromembedding.py.Differential Revision: D115457342