Skip to content
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 missing entries in mappings #16857

Merged
merged 2 commits into from
Apr 22, 2022

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Apr 20, 2022

What does this PR do?

Add missing entries in mappings: TOKENIZER_MAPPING, FEATURE_EXTRACTOR_MAPPING, PROCESSOR_MAPPING

The remaining models that don't have any tokenizer/feature_extractor/processor are:

  • EncoderDecoder
  • VisionEncoderDecoder
  • SpeechEncoderDecoder
  • DecisionTransformer

which are expected to have no processor.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 20, 2022

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

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

All for adding missing entries, thanks a lot for this PR!
BUT:

  • We should actually push more and more users to use the Auto classes API and not a specific model/tokenizer/feature extractor class. So big no on changing docstring that use AutoTokenizer
  • The ImageGPT changes should probably go in their own PR

src/transformers/models/yoso/modeling_yoso.py Outdated Show resolved Hide resolved
@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 20, 2022

  • The ImageGPT changes should probably go in their own PR

Without the changes, the test_pipeline will fail for ImageGPT. Previously, ImageGPT was not used in pipeline testing because it had no found tokenizer/feature_extractor.

I can add ("imagegpt", "ImageGPTFeatureExtractor") in another PR instead of this PR, and make the necessary changes along with it. Let me know if this is necessary 😃

@ydshieh ydshieh marked this pull request as ready for review April 20, 2022 21:01
@sgugger
Copy link
Collaborator

sgugger commented Apr 20, 2022

Yes, I think ImageGPT should be added in a separate PR since it requires additional changes.

@ydshieh ydshieh force-pushed the add_missing_items_in_mappings branch from 3860253 to 9da0c0d Compare April 21, 2022 07:54
@ydshieh ydshieh removed the request for review from NielsRogge April 21, 2022 07:58
@ydshieh
Copy link
Collaborator Author

ydshieh commented Apr 21, 2022

The changes on ImageGPT are now on a separate PR: #16869

@ydshieh ydshieh force-pushed the add_missing_items_in_mappings branch from 9da0c0d to 7ba782a Compare April 21, 2022 08:22
@ydshieh ydshieh force-pushed the add_missing_items_in_mappings branch from 7ba782a to 21ecef4 Compare April 22, 2022 08:12
@ydshieh ydshieh merged commit 3b1bbef into huggingface:main Apr 22, 2022
@ydshieh ydshieh deleted the add_missing_items_in_mappings branch April 22, 2022 08:53
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
* add missing entries in some mappings

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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