Skip to content

fix(train): pad training batches with the model's pad id - #358

Merged
stephantul merged 2 commits into
MinishLab:mainfrom
serhiizghama:fix/train-pad-with-model-pad-id
Sep 1, 2026
Merged

fix(train): pad training batches with the model's pad id#358
stephantul merged 2 commits into
MinishLab:mainfrom
serhiizghama:fix/train-pad-with-model-pad-id

Conversation

@serhiizghama

Copy link
Copy Markdown
Contributor

TextDataset.collate_fn pads training batches with a hardcoded padding_value=0, but _encode masks padding by comparing against self.pad_id. When the tokenizer's pad token isn't id 0, every padded position holds a real vocabulary token that never gets masked — it gets a real embedding, a real sigmoid(w[...]) weight, and it counts in the mean-pooling denominator. So a text's representation during training depends on which other texts landed in the same batch, and it doesn't match what predict() or the exported StaticModel produce for that same text.

The inference path in the same class already does it right (tokenize uses padding_value=self.pad_id), as does distill/inference.py — the training collate was the only place left on 0.

It stays latent on the official potion-* models because their pad resolves to 0, but anything distilled from a RoBERTa/XLM-R base hits it: paraphrase-multilingual-MiniLM-L12-v2 and multilingual-e5-small both resolve to pad_id=1 via get_probable_pad_token_id.

TextDataset now takes the pad id (defaulting to 0, so the existing constructor calls still work) and _prepare_dataset passes the model's. collate_fn had to stop being a staticmethod; it's only ever reached through to_dataloader, which already binds it off the instance.

The test builds a model with pad_id=1 and checks the training batch matches what tokenize produces, plus that a text encodes the same whether or not it got padded. Without the fix it sees [[2, 0], [2, 3]] against [[2, 1], [2, 3]]. Suite is 286 passed, ruff and mypy clean.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
model2vec/train/base.py 97.97% <100.00%> (ø)
model2vec/train/dataset.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stephantul

stephantul commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Can you demonstrate that this is true?

but anything distilled from a RoBERTa/XLM-R base hits it: paraphrase-multilingual-MiniLM-L12-v2 and multilingual-e5-small both resolve to pad_id=1 via get_probable_pad_token_id.

I think this is not true, these models set <pad> to 0. I don't mind the fix, but I don't see where this assertion is coming from.

@stephantul

Copy link
Copy Markdown
Contributor

Alright, I looked it over and looks good, thanks for the PR and for using Model2Vec!

For future reference: I'd prefer it if your accompanying PR description was toned down or accompanied by evidence. Several assertions made in the PR are false, and I think on the whole the impact of this is overstated: most, if not all, model2vec models get [PAD] as their 0 token. If the 0 index is not [PAD], it will be [UNK]. Both of these tokens are not shown during regular operation.

@stephantul
stephantul merged commit f16a2ce into MinishLab:main Sep 1, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants