Skip to content

Put all models in the constants#9170

Merged
sgugger merged 2 commits intomasterfrom
fix_tapas_tokenizer
Dec 17, 2020
Merged

Put all models in the constants#9170
sgugger merged 2 commits intomasterfrom
fix_tapas_tokenizer

Conversation

@sgugger
Copy link
Copy Markdown
Collaborator

@sgugger sgugger commented Dec 17, 2020

What does this PR do?

It was impossible to use all pretrained checkpoints in the tapas tokenzier file because they were not in the constants of the file. This PR fixes that.

@sgugger sgugger requested a review from LysandreJik December 17, 2020 16:04
@sgugger
Copy link
Copy Markdown
Collaborator Author

sgugger commented Dec 17, 2020

An alternative is to remove all content, but the previous state where one variable as some keys but the others not leads to failures.

Copy link
Copy Markdown
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, thanks @sgugger!

@julien-c
Copy link
Copy Markdown
Member

Yes, we really need to remove all of this @LysandreJik @thomwolf

@LysandreJik
Copy link
Copy Markdown
Member

We do, but right now we still use them in the tests

@NielsRogge
Copy link
Copy Markdown
Collaborator

It was a bit confusing to me which to add in the constants and which not.. 😅

@NielsRogge
Copy link
Copy Markdown
Collaborator

NielsRogge commented Dec 17, 2020

@sgugger one more small fix you can add is in tapas.rst under "Usage: inference", there should be one more space:

docs

(Sorry, if I see more things I make a new PR myself :p)

@sgugger sgugger merged commit 6d2e864 into master Dec 17, 2020
@sgugger sgugger deleted the fix_tapas_tokenizer branch December 17, 2020 16:23
@LysandreJik
Copy link
Copy Markdown
Member

Pushing your fix on master directly Niels

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