-
Notifications
You must be signed in to change notification settings - Fork 27.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tokenizers class mismatch detection between cls
and checkpoint
#12619
Changes from all commits
de3725f
4fc28ed
faa4fa3
05b43e8
b998ac5
b2db7c5
a2050ba
7d33c28
c440af2
634e31d
f36c44f
2ac9387
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1745,13 +1745,58 @@ def _from_pretrained( | |
if tokenizer_config_file is not None: | ||
with open(tokenizer_config_file, encoding="utf-8") as tokenizer_config_handle: | ||
init_kwargs = json.load(tokenizer_config_handle) | ||
# First attempt. We get tokenizer_class from tokenizer_config to check mismatch between tokenizers. | ||
config_tokenizer_class = init_kwargs.get("tokenizer_class") | ||
init_kwargs.pop("tokenizer_class", None) | ||
saved_init_inputs = init_kwargs.pop("init_inputs", ()) | ||
if not init_inputs: | ||
init_inputs = saved_init_inputs | ||
else: | ||
config_tokenizer_class = None | ||
init_kwargs = init_configuration | ||
|
||
SaulLu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if config_tokenizer_class is None: | ||
from .models.auto.configuration_auto import AutoConfig | ||
|
||
# Second attempt. If we have not yet found tokenizer_class, let's try to use the config. | ||
try: | ||
config = AutoConfig.from_pretrained(pretrained_model_name_or_path) | ||
config_tokenizer_class = config.tokenizer_class | ||
except (OSError, ValueError, KeyError): | ||
# skip if an error occured. | ||
config = None | ||
if config_tokenizer_class is None: | ||
# Third attempt. If we have not yet found the original type of the tokenizer, | ||
# we are loading we see if we can infer it from the type of the configuration file | ||
from .models.auto.configuration_auto import CONFIG_MAPPING | ||
from .models.auto.tokenization_auto import TOKENIZER_MAPPING | ||
|
||
if hasattr(config, "model_type"): | ||
config_class = CONFIG_MAPPING.get(config.model_type) | ||
else: | ||
# Fallback: use pattern matching on the string. | ||
config_class = None | ||
for pattern, config_class_tmp in CONFIG_MAPPING.items(): | ||
if pattern in str(pretrained_model_name_or_path): | ||
config_class = config_class_tmp | ||
break | ||
|
||
if config_class in TOKENIZER_MAPPING.keys(): | ||
config_tokenizer_class, config_tokenizer_class_fast = TOKENIZER_MAPPING[config_class] | ||
if config_tokenizer_class is not None: | ||
config_tokenizer_class = config_tokenizer_class.__name__ | ||
else: | ||
config_tokenizer_class = config_tokenizer_class_fast.__name__ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. -------- EDIT:-------- -------- Old comment:-------- It would have to be checked by running all the tests, but I have the impression that by doing the imports at this level we don't have a circular import problem. # If we have not yet found the original type of the tokenizer we are loading we see if we can infer it from the
# type of the configuration file
if config_dict is not None and config_tokenizer_class is None:
from .models.auto.configuration_auto import CONFIG_MAPPING
from .models.auto.tokenization_auto import TOKENIZER_MAPPING
if "model_type" in config_dict:
config_class = CONFIG_MAPPING[config_dict["model_type"]]
else:
# Fallback: use pattern matching on the string.
for pattern, config_class_tmp in CONFIG_MAPPING.items():
if pattern in str(pretrained_model_name_or_path):
config_class = config_class_tmp
break
if config_class in TOKENIZER_MAPPING.keys():
config_tokenizer_class, config_tokenizer_class_fast = TOKENIZER_MAPPING[config_class]
if config_tokenizer_class is not None:
config_tokenizer_class = config_tokenizer_class.__name__
else:
config_tokenizer_class = config_tokenizer_class_fast.__name__ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for a excellent suggestion! |
||
if config_tokenizer_class is not None: | ||
if cls.__name__.replace("Fast", "") != config_tokenizer_class.replace("Fast", ""): | ||
logger.warning( | ||
"The tokenizer class you load from this checkpoint is not the same type as the class this function is called from. " | ||
"It may result in unexpected tokenization. \n" | ||
f"The tokenizer class you load from this checkpoint is '{config_tokenizer_class}'. \n" | ||
f"The class this function is called from is '{cls.__name__}'." | ||
Comment on lines
+1794
to
+1797
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great 👍 ! This will really help future users |
||
) | ||
|
||
# Update with newly provided kwargs | ||
init_kwargs.update(kwargs) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,11 @@ def setUp(self): | |
tokenizer = PreTrainedTokenizerFast.from_pretrained(model_paths[0]) | ||
tokenizer.save_pretrained(self.tmpdirname) | ||
|
||
def test_tokenizer_mismatch_warning(self): | ||
# We disable this test for PreTrainedTokenizerFast because it is the only tokenizer that is not linked to any | ||
# model | ||
pass | ||
Comment on lines
+47
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
def test_pretrained_model_lists(self): | ||
# We disable this test for PreTrainedTokenizerFast because it is the only tokenizer that is not linked to any | ||
# model | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that the other tokenizers in the
transformers
library specify the specific class of tokenizer here instead of the genericAutoTokenizer
. Was there any particular reason to preferAutoTokenizer
toBertJapaneseTokenizer
? 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion about it too. I chose
AutoTokenizer
because I thought leading a user toAutoTokenizer
would avoid a problem like this issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to encourage users to use the
AutoTokenizer
class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your feedback @sgugger ! In that case, @europeanplaice your proposal is great - you can ignore my previous comment.
@sgugger, Should we take this opportunity to make the same change with other tokenizers that log the same type of message (cf PR #12745)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was a great idea!