Repository navigation
Fix stale _added_tokens_encoder entries in cpmant and wav2vec2 - #47440
Conversation
|
cc @itazap |
12fdfb5 to
c2c0177
Compare
|
For whoever picks this up: the red shard is unrelated to this diff. I cannot re-run the job from my side. Happy to rebase to force a fresh run if that is easier than a re-run. |
|
I'm not sure if the sync function is what we're looking for here, there should be a direct way to fix the used _added_tokens_decoder / _added_tokens_encoder in the models that touch them |
c2c0177 to
23132c9
Compare
ishan-1010
changed the title
|
@itazap pushed the direct version, base class untouched now. cpmant was popping the property instead of the real dict (one underscore in the end), and wav2vec2's set_target_lang pops both dicts now. dropping the cache costs nothing btw, the hot loops already read these once since #46323/#47425. still think a single mutation point would stop this from recurring, but that can be a follow-up, this PR doesn't depend on it. |
|
@itazap just checking in on this, it's been about 6 weeks since I pushed the direct fix addressing your feedback. CI is green and it's still mergeable, happy to make any further changes if needed. |
|
hey @ishan-1010 , I apologize for the big delay on this! I just left a few comments. happy to merge this asap afterwards! |
23132c9 to
91f508f
Compare
|
@itazap done, pushed. rebased on main too. the new tests fail without the fix. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: cpmant, wav2vec2 |
91f508f to
af8618c
Compare
- wav2vec2: pop the encoder entry straight from the decoder pop - tests: drop set(), assertNotIn on _added_tokens_encoder and the contents of _added_tokens_decoder, compare the dicts directly
af8618c to
32c8cdb
Compare
CI recapDashboard: View test results in Grafana |
|
awesome, thank you ! 🚀 |
|
run_slow: wav2vec2, cpmant |
AMD CIThis comment contains models: ["models/cpmant", "models/wav2vec2"] |
Nvidia CIThis comment contains models: ["models/cpmant", "models/wav2vec2"] |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Fixes #47439.
Two tokenizers leave
_added_tokens_encoderout of sync with_added_tokens_decoderafter mutating it, soconvert_tokens_to_idsreturns ids that no longer exist.self.added_tokens_encoder, which is the property. The property builds a fresh dict on every access, so the pop did nothing and the markers kept their stale ids in the real cache. Popping_added_tokens_encoderinstead fixes it.set_target_langdeleted conflicting entries from the decoder only, so a removed added token still encoded to its old id, which now belongs to an unrelated vocabulary token in the new language. The loop now pops both dicts.One regression test per model; both fail on main and pass here.
Earlier revisions of this PR did this through a base-class sync helper plus a cached sorted view. Reworked per the review to fix the two models directly, no base-class changes. The per-access property rebuild that the cache removed is not a problem in practice: the hot paths that used to read these properties in loops were already fixed to read them once (#46323, #46535, #47425), so nothing user-visible regresses.
Tests run:
pytest tests/models/cpmant/test_tokenization_cpmant.py tests/models/wav2vec2/test_tokenization_wav2vec2.py tests/tokenization/test_tokenization_utils.py- 132 passed (withjinja2,tiktoken,sentencepieceinstalled), 0 failed.AI-assisted; every line reviewed and run locally.