Conversation
hmellor
left a comment
There was a problem hiding this comment.
Should we also remove this from MT5 and UMT5 then?
|
[For maintainers] Suggested jobs to run (before merge) run-slow: mt5, umt5 |
There was a problem hiding this comment.
Pull request overview
This PR removes the tokenizer_class attribute from the base PreTrainedConfig and from the MT5/UMT5 config classes, aligning tokenizer selection away from config-level defaults and toward tokenizer config/mappings (as referenced by the vLLM CI failure in the PR description).
Changes:
- Remove
tokenizer_classfromMT5ConfigandUMT5Config(docstring + class attribute). - Remove the
tokenizer_classfield (and its import dependency) fromPreTrainedConfig.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/transformers/models/umt5/configuration_umt5.py |
Drops tokenizer_class from UMT5 config surface and docs. |
src/transformers/models/mt5/configuration_mt5.py |
Drops tokenizer_class from MT5 config surface and docs. |
src/transformers/configuration_utils.py |
Removes the base PreTrainedConfig.tokenizer_class field and the now-unused tokenizer import. |
| # Fine-tuning task arguments | ||
| id2label: dict[int, str] | dict[str, str] | None = None | ||
| label2id: dict[str, int] | dict[str, str] | None = None | ||
| problem_type: Literal["regression", "single_label_classification", "multi_label_classification"] | None = None | ||
|
|
There was a problem hiding this comment.
Removing tokenizer_class from PreTrainedConfig will break existing tests/utilities that treat it as a common config kwarg/field (e.g. tests/utils/test_configuration_utils.py::test_config_common_kwargs_is_complete expects tokenizer_class to be present in PreTrainedConfig().__dict__). Please update the corresponding test expectations (and any shared config_common_kwargs/common-config logic) to reflect the new base config surface, or keep a backward-compatible tokenizer_class field if it’s still considered part of the common config contract.
|
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. |
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=44971&sha=ece5f2 |
What does this PR do?
Removed the tokenizer_class attr was never there to begin with, and kwargs are now supported.
This was failing some test on vllm ci. Fixes https://buildkite.com/vllm/ci/builds/57601/steps/canvas?sid=019d1aec-aa5a-41db-bac6-4f42397279d5
checked and this works expectedly.