Skip to content

Conversation

@amyeroberts
Copy link
Contributor

@amyeroberts amyeroberts commented Dec 15, 2022

What does this PR do?

Creates an equivalent test_feature_extraction_common.py for image processors: test_image_processing_common.py and moves any vision and image processing specific logic to this file.

This is necessary for creating ImageProcessingSavingTestMixin in order to rename any feature extractor references in the image processor tests. This is left for a future PR.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@amyeroberts amyeroberts force-pushed the add-test-image-processing-common branch from 9760785 to 56fe551 Compare January 19, 2023 16:03
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jan 19, 2023

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

@amyeroberts amyeroberts force-pushed the add-test-image-processing-common branch from 5ee0594 to 7b936c9 Compare January 20, 2023 14:34
@amyeroberts amyeroberts marked this pull request as ready for review January 20, 2023 17:01
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.

Shouldn't all the tests use ImageProcessingSavingTestMixin instead of FeatureExtractionSavingTestMixin?

LGTM otherwise, thanks!

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, with same question as Sylvain mentioned. Thank you for working on 🖼️ !

You mentioned

This is necessary for creating ImageProcessingSavingTestMixin in order to rename any feature extractor references in the image processor tests. This is left for a #20768.

But the new added image_processing_common.py contains ImageProcessingSavingTestMixin. So it looks a bit confusing, but I believe you have some reasons.

@amyeroberts
Copy link
Contributor Author

@ydshieh @sgugger Yes, sorry, I could have been clearer above.

The reason for not replacing FeatureExtractionSavingTestMixin with ImageProcessingSavingTestMixin in the test_image_processing_xxx.py files in this PR is that the tests in the original mixin use class attributes like self.feature_extraction_class. When updating the mixin, then attributes in the testing class XxxImageProcessingTest for each test file test_image_processing_xxx.py have to be updated. This results in either 1) updating just the FE references to have the tests running or 2) updating all of the FE references. In the case of 1) it results in the code being mixed between feature extractors and image processors which I found confusing to read (subjective opinion) and for 2) it introduced hundreds of lines of additional diff. I decided to leave the switch to a follow up PR.

@ydshieh
Copy link
Collaborator

ydshieh commented Jan 23, 2023

@ydshieh @sgugger Yes, sorry, I could have been clearer above.

The reason for not replacing FeatureExtractionSavingTestMixin with ImageProcessingSavingTestMixin in the test_image_processing_xxx.py files in this PR is that the tests in the original mixin use class attributes like self.feature_extraction_class. When updating the mixin, then attributes in the testing class XxxImageProcessingTest for each test file test_image_processing_xxx.py have to be updated. This results in either 1) updating just the FE references to have the tests running or 2) updating all of the FE references. In the case of 1) it results in the code being mixed between feature extractors and image processors which I found confusing to read (subjective opinion) and for 2) it introduced hundreds of lines of additional diff. I decided to leave the switch to a follow up PR.

Understand! Thank you for explaining!
BTW, it would be super nice to give a link like

feature_extraction_class = BeitFeatureExtractor if is_vision_available() else None

(I agree it is a bit more time consuming :-) )

@amyeroberts amyeroberts merged commit 66459ce into huggingface:main Jan 23, 2023
This was referenced Jan 25, 2023
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