[bugfix] track delta embeddings by owner-qualified FQN - #607
Merged
tiankongdeguiji merged 5 commits intoJul 27, 2026
Merged
Conversation
ModelDeltaTrackerTrec keyed raw table names globally, so same-named tables owned by different sharded modules overwrote mappings. Track callback IDs, weights, dynamic modules, and shard metadata by owner-qualified FQN.
tiankongdeguiji
marked this pull request as ready for review
July 26, 2026 04:41
tiankongdeguiji
marked this pull request as draft
July 26, 2026 04:43
tiankongdeguiji
marked this pull request as ready for review
July 26, 2026 04:45
| index_start = max(self.per_consumer_batch_idx.values()) | ||
| if index_start < index_end: | ||
| self.store.compact(index_start, index_end) | ||
| tracker_rows = self.store.get_unique( |
Contributor
There was a problem hiding this comment.
DeltaStoreTrec.delete(up_to_idx=...) retains FQN buckets with empty lookup lists. After one dump, if a previously seen table has no unread IDs in a later interval, this reaches torch.cat([]) inside get_unique() and aborts before the dumper's empty-output path. Please prune/omit empty unread buckets and add a two-interval regression.
Contributor
|
Static review complete. I left one inline comment: after a prior dump, |
DeltaStoreTrec retains empty FQN buckets after delete-on-read, causing its unique-row path to concatenate no tensors. Build rows only from unread lookups and omit empty buckets.
eric-gecheng
approved these changes
Jul 27, 2026
eric-gecheng
added a commit
to eric-gecheng/TorchEasyRec
that referenced
this pull request
Jul 27, 2026
Reconciles master alibaba#604 (online dense export), alibaba#607 (track delta by owner-qualified FQN), alibaba#608 (export sparse by table FQN), and alibaba#609 (drop INPUT_TILE mapping) with the branch's FeatureStore delta-dump work. Keeps master alibaba#607's FQN-keyed delta tracking (local ModelDeltaTracker subclass, source/table_fqn schema) and alibaba#608's FQN sparse-export naming as the canonical base, and re-grafts the branch's FeatureStore upload, minute-cadence, and synced-dataloader-exhaustion onto alibaba#607's dumper. The uploader reads table_fqn and publishes remap_input_tile_user_key(table_fqn) as the FeatureStore embedding_name, aligning delta upload with alibaba#608's export naming and serving. Synced-exhaustion is kept so alibaba#607's _sync_final_step boundary-skip does not drop a shorter-exhausted rank's trailing delta (SEANQ). alibaba#604's online dense export and alibaba#609's INPUT_TILE removal are adopted as-is; main.py keeps the synced-exhaustion gating and FeatureStore start/close hooks. Co-Authored-By: Claude <noreply@anthropic.com>
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
ShardedTensor, andDTensorinterfacesRoot cause
The previous tracking path built global mappings keyed by raw table name. When separate sharded modules owned tables with the same name, the later mapping could overwrite the earlier one, causing feature mappings, touched IDs, and embedding lookups to resolve to the wrong owner.
Additionally,
DeltaStoreTrec.delete(up_to_idx=...)retains FQN buckets with empty lookup lists. A later interval with no new IDs for a previously seen table could therefore reachtorch.cat([])inside TorchRec'sget_unique()implementation instead of reaching the dumper's empty-output path.Implementation
The delta dump now has an ID-only tracker built directly on TorchRec's abstract
ModelDeltaTracker. Each lookup callback uses its owning sharded module to resolve a module-local feature-to-FQN mapping, so IDs remain attached to the exact EC/EBC owner that produced them.DeltaEmbeddingDumpercarries that FQN directly through metadata and storage collection and intotable_fqnoutput. Dynamicemb extracts only the final table-name component where its table index API requires it; nofqn_to_tablemapping is introduced.The tracker builds unique rows only from unread lookups in each FQN bucket and omits buckets with no unread IDs. Consumer cursors, delete-on-read, compaction, and empty-shard behavior remain unchanged.
The implementation intentionally uses the supported TorchRec 1.7 interfaces instead of accepting arbitrary lookalike objects through defensive reflection. The sole remaining
getattrselects the active protobuf oneof field. The parquet schema, configuration, and dump scheduling are unchanged.Test Plan
Run in the
tzrec130environment:PYTHONPATH=. python -m tzrec.utils.delta_embedding_dump_test— 43 tests passed, including two consecutive intervals where the second has no unread IDs, duplicate EC/EBC owners, row-wise sharding, static and DTensor weights, pause guards, dynamicemb, and multi-process DMP regressionspre-commit run -a— passedgit diff --check— passedpython scripts/pyre_check.py— blocked by existing environment/repository constraints: the committed.pyre_configurationis invalid JSON, and the Pyre backend requires newer GLIBC/GLIBCXX symbols than the host provides