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.

Sign up for GitHub

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

Jump to bottom

[Pipeline] skip feature extraction test if in IMAGE_PROCESSOR_MAPPING #20790

Merged
merged 1 commit into from
Dec 16, 2022
Merged

[Pipeline] skip feature extraction test if in IMAGE_PROCESSOR_MAPPING #20790

merged 1 commit into from
Dec 16, 2022

Conversation

younesbelkada
Copy link
Contributor

What does this PR do?

Fixes the following failing test

Context:

Currently FeatureExtractionPipelineTests are skipped for multi-modal models by checking if the model config is in FEATURE_EXTRACTOR_MAPPING. The check is done on this line
Recent vision and multimodal models will deprecate the usage of xxxFeatureExtractor in favor of xxxImageProcessors. For Blip, the test fails because BlipFeatureExtractor is not implemented at all in favor of BlipImageProcessor.

Why this fix is relevant?

Blip seems to be the first multimodal model that relies on xxxImageProcessor only.

cc @Narsil @amyeroberts @NielsRogge

@younesbelkada younesbelkada mentioned this pull request Dec 15, 2022
Merged
3 tasks
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Dec 15, 2022 •
edited
Loading

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

Narsil
Narsil approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

LGTM.

Are all image FeatureExtractor going to be renamed ImageProcessor ?

@younesbelkada
Copy link
Contributor Author

younesbelkada commented Dec 16, 2022 •
edited
Loading

Thanks
I am not sure about how do we want to approach that exactly, but I think that's the plan at some point cc @amyeroberts :D

@Narsil
Copy link
Contributor

Narsil commented Dec 16, 2022

Thanks I am not sure about how do we want to approach that exactly, but I think that's the plan at some point cc @amyeroberts :D

I think it's good to disambiguate Audio from Vision (both are currently named FeatureExtractor I think).

In that regard I'd like to stress to include no-code (or almost none) into Processor the general class that encapsulates Tokenizer, FeatureExtractor and ImageProcessor . It's great for demos and quick hacks, but it's much more cumbersome to reason about within a lib, as it's impossible to know what it should be able to do since it's by definition not standard. (It doesn't have any invariant).

For instance Tokenizer.encode(text) is always going to be a valid call and will return ids (that's an invariant).

sgugger
sgugger approved these changes Dec 16, 2022
View reviewed changes
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.

Thanks for the fix!

@younesbelkada younesbelkada merged commit 4341f4e into huggingface:main Dec 16, 2022
gsarti added a commit to gsarti/transformers that referenced this pull request Dec 16, 2022
@gsarti
Merge branch 'main' of ssh://github.com/huggingface/transformers into…
468faa7 
… add_get_encoder_decoder_fsmt

* 'main' of ssh://github.com/huggingface/transformers: (1433 commits)
  Add Universal Segmentation class + mapping (huggingface#20766)
  Stop calling expand_1d on newer TF versions (huggingface#20786)
  Fix object detection2 (huggingface#20798)
  [Pipeline] skip feature extraction test if in `IMAGE_PROCESSOR_MAPPING` (huggingface#20790)
  Recompile `apex` in `DeepSpeed` CI image (huggingface#20788)
  Move convert_to_rgb to image_transforms module (huggingface#20784)
  Generate: use `GenerationConfig` as the basis for `.generate()` parametrization (huggingface#20388)
  Install video dependency for pipeline CI (huggingface#20777)
  Fixing object detection with `layoutlm` (huggingface#20776)
  [Pipeline] fix failing bloom `pipeline` test (huggingface#20778)
  Patch for FlanT5-XXL 8bit support (huggingface#20760)
  Install vision for TF pipeline tests (huggingface#20771)
  Even more validation. (huggingface#20762)
  Add Swin backbone (huggingface#20769)
  Install `torch-tensorrt 1.3.0` for DeepSpeed CI (huggingface#20764)
  Replaces xxx_required with requires_backends (huggingface#20715)
  [CI-Test] Fixes but also skips the mT5 tests (huggingface#20755)
  Fix attribute error problem  (huggingface#20765)
  [Tests] Improve test_attention_outputs (huggingface#20701)
  Fix missing `()` in some usage of `is_flaky` (huggingface#20749)
  ...
amyeroberts pushed a commit to amyeroberts/transformers that referenced this pull request Jan 4, 2023
@younesbelkada @amyeroberts
[Pipeline] skip feature extraction test if in `IMAGE_PROCESSOR_MAPPIN…
4b85b45 
…G` (huggingface#20790)

skip feature extraction test if in `IMAGE_PROCESSOR_MAPPING`
venkat-natchi pushed a commit to venkat-natchi/transformers that referenced this pull request Jan 22, 2023
@younesbelkada @venkat-natchi
[Pipeline] skip feature extraction test if in `IMAGE_PROCESSOR_MAPPIN…
75e2198 
…G` (huggingface#20790)

skip feature extraction test if in `IMAGE_PROCESSOR_MAPPING`
miyu386 pushed a commit to miyu386/transformers that referenced this pull request Feb 9, 2023
@younesbelkada @miyu386
[Pipeline] skip feature extraction test if in `IMAGE_PROCESSOR_MAPPIN…
a22952b 
…G` (huggingface#20790)

skip feature extraction test if in `IMAGE_PROCESSOR_MAPPING`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgugger sgugger sgugger approved these changes

@Narsil Narsil Narsil approved these changes

@amyeroberts amyeroberts Awaiting requested review from amyeroberts

@NielsRogge NielsRogge Awaiting requested review from NielsRogge

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@younesbelkada @HuggingFaceDocBuilderDev @Narsil @sgugger