MPE preprint version - #215
Conversation
MXueguang
left a comment
There was a problem hiding this comment.
Thanks — multi-prefix embeddings / MaxSim over chunk boundaries is a feature we'd like to land, and moving the scripts under examples/mpe/ is already an improvement over #211. But there are several blockers, one of them urgent, so I'm requesting changes.
Urgent: decrypted BrowseComp-Plus queries committed in plaintext
examples/BrowseComp-Plus/topics-qrels/queries.tsv contains 830 decrypted BC+ queries. BrowseComp-Plus deliberately distributes queries encrypted to prevent web-crawl/training contamination; committing the plaintext to a public repo defeats that. Since the file is in the branch history, deleting it in a follow-up commit isn't enough — the branch needs to be rewritten (or recreated) without it ever having been committed.
Silently breaks SPLADE training (clean-merge semantic conflict)
The PR moves loss computation out of EncoderModel.forward (which now returns loss=None during training) into TevatronTrainer.compute_loss. But SpladeTrainer.compute_loss on current main (from 84433fb) reads output.loss from the model — after this merge that's None, and git merges the file cleanly so nothing flags it. It also bypasses the compute_similarity()/compute_loss() override contract that EncoderModel subclasses rely on. Please keep the forward-loss contract intact — e.g. route MaxSim through a compute_similarity override in a chunk-aware subclass — so SPLADE/GradCache/subclass paths are untouched.
Not gated even when the feature is off
With all chunk flags at defaults, training still goes through the rewritten forward + relocated trainer loss, the restructured TrainCollator.__call__ (whose no-chunk fallback hardcodes add_special_tokens=True, reverting main's add_special_tokens=self.data_args.add_special_tokens), and the unconditional tokenizer.eos_token_id = tokenizer.pad_token_id mutation in both drivers (in train.py it runs before the pad-None fallback, so Llama/Mistral-style tokenizers end up with pad = eos = None). "MPE off" must mean "behavior unchanged".
Other issues
trust_remote_code=Trueadded to all threeload_datasetcalls indataset.py— a security-relevant default change unrelated to the feature; please drop it.from rich import printat the top ofcollator.pyanddriver/encode.py(richis not a Tevatron dependency → ImportError for all retriever users), ~10 debugprint()s, a bareexcept:, a 40-line commented-out logging block, and module-leveltorch.set_printoptions.- The tests from #211 didn't make it into this PR, so this version is untested — please bring them over (hermetic, no Hub downloads in fixtures).
- The branch predates the July Megatron/SPLADE merge;
collator.pyhas two hard textual conflicts with main (lazy multimodal imports, tokenization block).
Suggested path forward: rebase/re-implement on current main with the feature strictly opt-in, restore the forward-loss contract, drop queries.tsv from history, port the tests, and close #211 as superseded so this PR is the single vehicle. Happy to review a reworked version — the underlying feature is a good fit for Tevatron.
The plaintext queries must never be committed; regenerate locally with examples/BrowseComp-Plus/decrypt_dataset.py --generate-tsv. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the thorough review. The urgent item is addressed: I've rewritten the branch history so One caveat: the pre-rewrite commits may remain reachable by SHA on GitHub until the dangling objects are purged — I'll contact GitHub support about purging them from my fork. I'm now working through the remaining items: restoring the forward-loss contract (routing MaxSim through a chunk-aware |
|
Superseded by #229, which was opened from a branch whose history never contained the decrypted queries.tsv. |
Summary
This PR adds Multi-Prefix Embedding (MPE) support for long-document dense retrieval in Tevatron, together with reproduction scripts for the preprint experiments.
The main idea is to represent a long passage with multiple prefix embeddings extracted at EOS-separated chunk boundaries, then score query-document relevance with MaxSim over the passage prefixes.
Main changes
Add chunking-related data arguments:
passage_chunk_sizepassage_chunk_size_rangepassage_chunk_size_variablepassage_chunk_independentencode_use_pre_chunkedExtend training and encoding collators to support:
Extend
DenseModelto support chunked passage representations:[num_passages, max_chunks, hidden_dim]pluschunk_mask,Update trainer logic to:
Update search logic to:
Add
examples/mpereproduction scripts for:Methods covered by the reproduction scripts
The scripts compare:
Notes for reviewers
This PR touches both core retriever code and paper reproduction scripts. The core change is the chunked passage representation path; the scripts under
examples/mpeare included to reproduce the preprint results.Areas worth reviewing carefully:
AI generated