0121 maxsim support - #211
Conversation
…nd random chunking
MXueguang
left a comment
There was a problem hiding this comment.
Thanks for the contribution — chunked passage encoding + MaxSim aggregation is a feature we'd genuinely like to have in Tevatron. That said, the branch in its current form is a research-experiment snapshot rather than a mergeable change, so I'm requesting changes. Main points:
Files that shouldn't be in the PR
req.txtis a personalpip freeze, including-e git+ssh://references to private forks — please remove.finetune.sh,finetune_with_chunk.sh,run_retrieval*.share personal experiment scripts (hardcoded output dirs etc.). If you want to keep runnable recipes, move a cleaned-up version underexamples/.
Changes that break the default (non-chunked) path for all users
driver/train.py/driver/encode.py: the unconditionaltokenizer.eos_token_id = tokenizer.pad_token_idmutates EOS for every run. Intrain.pyit's placed before thepad_token_id is Nonefallback, so models like Llama/Mistral end up withpad = eos = None, breaking--append_eos_token. This only happens to work for Qwen (pad == eos). Please drop both lines and derive the chunk separator from the tokenizer instead.- Debug leftovers that fire on every step for everyone:
print(target...)inEncoderModel.forward,print(eos_positions...)inencode_passage, theselected_negatives: Randomly selected!!!print indataset.py,print(encoded)dumping full embedding arrays inencode.py, module-leveltorch.set_printoptions(threshold=inf)andfrom rich import printincollator.py(richisn't a declared dependency), and theif True:Python double-loop incompute_maxsim_similaritythat logs every Q×P pair per forward. EOS_TOKEN_ID = 151643is hardcoded (Qwen vocab) and asserted inencode_passage— chunked mode crashes on any non-Qwen model.
Correctness within the feature
- Under DDP,
q_reps/p_repsare dist-gathered butchunk_maskis not, socompute_maxsim_similarityseesp_repsof sizeP × world_sizevs achunk_maskof sizeP— multi-GPU chunked training is broken. - Passing
eos_positionsvia attribute mutation (model.eos_positions = ...in the trainer) is fragile; please thread it throughforward()kwargs.
Staleness
- The branch is based on an October base and conflicts with the Megatron/SPLADE merge (84433fb):
collator.pyhas two real conflicts, and a naive resolution would revert main's lazy multimodal imports and theadd_special_tokens=self.data_args.add_special_tokensbehavior.
Given the drift, I'd suggest re-implementing the (reasonably contained) core logic on top of current main rather than resolving conflicts in place, keeping the feature strictly additive: with chunking flags off, behavior must be byte-identical to main. The test suite here is a good start — please keep it, but make it hermetic (no Hub downloads in fixtures) and trim the duplicated golden cases.
Also note #215 appears to be an evolved superset of this PR — if that's the intended vehicle, let's close this one and consolidate the rework (including these tests) there.
|
Closing as superseded by #215, which is now the single vehicle for the MPE / chunked-MaxSim feature. |
For training, train
--passage_chunk_size_variable # fully random chunks selected btw 32-256
For evaluation encoding, encode
--passage_chunk_size 0
--passage_chunk_size_range "32,64" \
--dataset_name json
--dataset_path "${prechunked_corpus_jsonl}"
--dataset_split train
--encode_use_pre_chunked
prechunked.jsonl: {"docid": "q-en-0-pos-0", "chunks": ["a", "b", "c"]}