Improve Transformers v4/v5 compatibility in tokenizers and processors#34768
Improve Transformers v4/v5 compatibility in tokenizers and processors#34768cccat6 wants to merge 2 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements to enhance compatibility with different versions of transformers and huggingface-hub.
- The
enable_hf_transferfunction is now compatible with both old and new APIs for transfer acceleration. - A robust compatibility layer has been added to handle older custom processors and tokenizers with newer versions of the
transformerslibrary, using temporary patches instead of global monkey-patching. - Special token handling for Ovis2.5 has been improved to be more robust.
- Tests have been updated to be more comprehensive and to properly clean up global state modifications.
The changes are well-implemented and address important compatibility issues. The code quality is high, and the solutions are pragmatic and well-designed. I have no major concerns and approve of these changes.
|
Hi @cccat6, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
Signed-off-by: Codex Bot <codex-bot@users.noreply.github.com>
b602bd0 to
54cc897
Compare
|
Addressed CI feedback in force-pushed commit |
|
@cccat6 Can you also test transformers 5.2 https://github.com/huggingface/transformers/releases/tag/v5.2.0 |
Will do. Thanks. |
|
Follow-up on compatibility testing request: I created a fresh local venv ( Validated with
No additional code changes were required from this round. |
|
Update: addressed the version-cap request and validated on Transformers 5.2.0.
This keeps v4/v5 transition compatibility while explicitly covering 5.2.0 as requested. |
Signed-off-by: Codex Bot <codex-bot@users.noreply.github.com>
06ba906 to
320ac1a
Compare
|
Thank you for this PR @cccat6, I'm going to ask @zucchini-nlp to help review this so that the processor changes are compatible with the vision for processors in Transformers. Then, when these models are upstreamed into Transformers, we can seamlessly transition to requiring no custom code on the HF Hub or in vLLM |
| missing = [ | ||
| token_name | ||
| for token_name in REQUIRED_SPECIAL_TOKENS.values() | ||
| if _get_token_id(tokenizer, token_name) is None | ||
| ] | ||
| if not missing: | ||
| return set() | ||
|
|
||
| add_special_tokens = getattr(tokenizer, "add_special_tokens", None) | ||
| if callable(add_special_tokens): | ||
| # Keep order stable so token IDs remain deterministic across runs. | ||
| add_special_tokens({"additional_special_tokens": missing}) | ||
| missing = [ | ||
| token_name | ||
| for token_name in missing | ||
| if _get_token_id(tokenizer, token_name) is None | ||
| ] | ||
|
|
There was a problem hiding this comment.
this is quite a long of manipulations to get a token-id. TBH I'd expect that if the token id exists, then convert_tokens_to_ids will return an int otherwise we can stop searching
Better to ask for a confirmation from @itazap
| def _ensure_processing_utils_compat() -> None: | ||
| # Some remote processors still import this alias, which is missing in | ||
| # newer Transformers releases. | ||
| if not hasattr(hf_processing_utils, "ChatTemplateLoadKwargs"): | ||
| fallback = getattr(hf_processing_utils, "ProcessorChatTemplateKwargs", None) | ||
| if fallback is not None: | ||
| hf_processing_utils.ChatTemplateLoadKwargs = fallback |
There was a problem hiding this comment.
indeed, I think it was only MiniCPM. Would be great to nudge authors to update their remote code if possible
hmellor
left a comment
There was a problem hiding this comment.
Blocking for now as this PR does a few too many things
| blake3 | ||
| py-cpuinfo | ||
| transformers >= 4.56.0, < 5 | ||
| transformers >= 4.56.0, <= 5.2.0 |
There was a problem hiding this comment.
We're not ready to do this yet
| transformers >= 4.56.0, <= 5.2.0 | |
| transformers >= 4.56.0, < 5 |
There was a problem hiding this comment.
Not related to compatibility, but is a QoL feature. I've extracted this to #35098
There was a problem hiding this comment.
Surely turning all the missing methods into noops will break whatever model had them?
|
Closing as the fixes that are usable have been superseded by other PRs and the ones that remain are not correct |
Summary
optional_attributes.Validation
transformers==4.56.2pytest -q -rs tests/model_executor/test_weight_utils.pypytest -q -rs tests/models/multimodal/processing/test_common.py::test_processing_correctness -k 'AIDC-AI/Ovis2.5-2B or allenai/Molmo2-8B or allenai/Molmo2-O-7B or zai-org/GLM-ASR-Nano-2512'transformers==4.57.4transformers==5.1.0Transformers 5.2.0 validation
transformers >= 4.56.0, <= 5.2.0.transformers==5.2.0:pytest -q -rs tests/model_executor/test_weight_utils.py(2 passed)pytest -q -rs tests/models/multimodal/processing/test_common.py::test_processing_correctness -k 'AIDC-AI/Ovis2.5-2B or allenai/Molmo2-8B or allenai/Molmo2-O-7B or zai-org/GLM-ASR-Nano-2512'(12 passed, 351 deselected)