Skip to content

Fix for TypeError in train_new_from_iterator() in tokenization_utils_fast.py - #34661

Closed
sindhuvahinis wants to merge 2 commits into
huggingface:mainfrom
sindhuvahinis:fix
Closed

sindhuvahinis wants to merge 2 commits into
huggingface:mainfrom
sindhuvahinis:fix

Conversation

@sindhuvahinis

Copy link
Copy Markdown

What does this PR do?

Fixes # (issue)

When calling train_new_from_iterator() on a tokenizer initialized using AutoTokenizer.from_pretrained(), the following error is raised:

tokenizer = AutoTokenizer.from_pretrained("TinyLlama/TinyLlama-1.1B-Chat-v1.0")
new_tokenizer = tokenizer.train_new_from_iterator(
    training_corpus, vocab_size=vocab_keep_items
)

Error traceback:

    new_tokenizer = tokenizer.train_new_from_iterator(
  File "/usr/local/lib/python3.10/dist-packages/transformers/tokenization_utils_fast.py", line 818, in train_new_from_iterator
    or tokenizer_json["pre_tokenizer"]["type"] == "Sequence"
    TypeError: 'NoneType' object is not subscriptable

Root Cause

The issue is due to improper handling of the conditional check for pre_tokenizer in tokenization_utils_fast.py. The current implementation does not correctly account for the case where tokenizer_json does not contain the "pre_tokenizer" key. This results in attempting to access "type" from a NoneType object, causing a TypeError. This bug seems to be existing from transformers version 4.45.2 onwards.

Manually tested with the above code.

@ArthurZucker

@LysandreJik LysandreJik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable, yes

cc @ArthurZucker for final look but feel free to merge if you're lacking bandwidth

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good but the failing test seems related: test_training_new_tokenizer_with_bytelevel vould you check it?

@ArthurZucker

Copy link
Copy Markdown
Collaborator

Thanks @sindhuvahinis for the fix! The pre_tokenizer is None crash in train_new_from_iterator() was fixed in #35396, and tokenization_utils_fast.py itself was later replaced by tokenization_utils_tokenizers.py in #40936. Closing as this is covered on main.

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.

3 participants