Skip to content

Conversation

sgugger
Copy link
Collaborator

@sgugger sgugger commented Sep 15, 2022

What does this PR do?

The custom tokenizers tests were never run because the test fetcher was not run before we look at its output to decide whether or not to run the tests. This PR fixes that and also adds missing tests to the nightly suite.

elif is_tf_available():
returned_tensor = "tf"
else:
elif is_flax_available():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test was failing when no framework (PyTorch, TF, Flax) is installed. Returning instead of failing now.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 15, 2022

The documentation is not available anymore as the PR was closed or merged.

@sgugger sgugger changed the title Fix custom tok test Fix custom tokenizers test Sep 15, 2022
Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

But I don't understand well this test: why we test tokenization for these 3 models only?

Also with a quick search, I can see @custom_tokenizers decorator is applied to cpm and BertJapanese. But here we run tests for openai and clip?

I must miss context.

@sgugger
Copy link
Collaborator Author

sgugger commented Sep 15, 2022

@ydshieh Those three files contain tests that require some specific dependencies to be installed (ftfy for openai and clip). So in the other test jobs, those tests are never run.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM

@sgugger sgugger merged commit f7ce4f1 into main Sep 15, 2022
@sgugger sgugger deleted the fix_custom_tok_test branch September 15, 2022 15:31
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.

4 participants