`image-segmentation` pipeline: re-enable `small_model_pt` test. #19716

Narsil · 2022-10-18T12:32:39Z

Fixes # (issue)

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?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

HuggingFaceDocBuilderDev · 2022-10-18T12:45:20Z

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

Re-enable `small_model_pt`. Enabling the current test with the current values. Debugging the values on the CI. More logs ? Printing doesn't work ? Using the CI values instead. Seems to be a Pillow sensitivity.

Narsil · 2022-10-18T15:14:10Z

I re-enable this tests, as it should work.

The error @alaradirik was seeing, seems to not happen on main, and the results are actually deterministic.

However
semantic returns 3 different masks
panoptic returns 0 masks

The old tests used to be panoptic and would return 2 masks (probably because of thresholding) of Label_215.

I kept the old values since I think something is wrong currently, but I wanted to re-enable the test before doing any modification to the actual pipeline code.
https://github.com/huggingface/transformers/pull/19727/files

@ydshieh I pinged you to see if you had better ideas to make tests less sensitive to the Pillow version. (there was a difference between CI and my computer), a few different pixels, and totally different hash.

We could use white_pixel_proportion instead and let the precision handle the slight differences in Pillow version, but I fear we're going to miss some really bad changes (when the actual shape of the mask is totally modified), that's why we added the hash in the first place. (But it seems to be quite sensitive, not flaky, but still seemed it could be painful to keep track of this)

ydshieh · 2022-10-18T15:47:58Z

I will check this pipeline to get a better idea of it, and see if I have any idea 👀 👀
👀 👀 !

alaradirik

Thank you, I'm looking into the post_process methods of DETR to pinpoint the issue.

src/transformers/pipelines/image_segmentation.py

Co-authored-by: Alara Dirik <8944735+alaradirik@users.noreply.github.com>

amyeroberts · 2022-10-19T11:13:46Z

tests/pipelines/test_pipelines_image_segmentation.py

    def test_small_model_pt(self):
        model_id = "hf-internal-testing/tiny-detr-mobilenetsv3-panoptic"

        model = AutoModelForImageSegmentation.from_pretrained(model_id)
        feature_extractor = AutoFeatureExtractor.from_pretrained(model_id)
-        image_segmenter = ImageSegmentationPipeline(model=model, feature_extractor=feature_extractor)


Just to check my understanding: we're testing two calls to the image_segmenter pipeline, one with one input image, the other with 2, and previously the "panoptic" task was tested and then "semantic" - whereas now both calls are for "semantic". Is the reason for this to make sure we're only testing one thing i.e. the pipeline can take batched and non-batched inputs?

Batching is actually taken care somewhere else too for random models.

A list is not batched necessarily, but it should return a list of what could have been inferred individually.

pipe([a, b]) == [pipe(a), pipe(b)] in some sense.
Not all pipelines respect that unfortunately (the old ones mostly we never modified their behavior to keep things compatible).

The second test with the list was probably from somewhere where it was tested against and just updated I think.
The main property of small_model_pt is that we are checking real values that should be fixed for time eternal, meaning we should never deviate from the values, because that would be a breaking change.

It can happen that updates in the dependencies like torch or Pillow trigger changes in the output values, and that is fine (since we are not responsible for the change).
Any other kind of change is breaking backward compatiblity and should be done with caution and be documented as to why we're doing it. I think the current way of thinking is that the older something in the library is, the less likely it's ok to break anything with regards to it. If it was just added, then it's ok.

previously the "panoptic" task was tested and then "semantic"

No previously only the panoptic was tested (it was defaulted to) for both.
But I'm switching here because panoptic appears to be broken, as it does not output anything (instead of outputting either 2 masks like before, or something resembling what semantic outputs).

Since I want to test against values that make sense I'm using semantic which seem OK.
When we find what's wrong with panoptic (or why the assumption that panoptic should return at least as much information as semantic is wrong) we can add them as tests again.
And then this test would start checking both values, and that panoptic > semantic in expressiveness (which afaik should always hold)

Thank you for taking the time to write out such detailed explanations ❤️

sgugger

LGTM! Can you make sure the title is more explicit (in particular contains image segmentation pipeline ;-) ) before merging?

amyeroberts

LGTM! Thanks for adding the test back in and for the detailed explanation of the pipeline tests.

…ingface#19716) * Re-enable `small_model_pt`. Re-enable `small_model_pt`. Enabling the current test with the current values. Debugging the values on the CI. More logs ? Printing doesn't work ? Using the CI values instead. Seems to be a Pillow sensitivity. * Update src/transformers/pipelines/image_segmentation.py Co-authored-by: Alara Dirik <8944735+alaradirik@users.noreply.github.com> Co-authored-by: Alara Dirik <8944735+alaradirik@users.noreply.github.com>

Narsil changed the title ~~# What does this PR do?~~ Re-enable small_model_pt. Oct 18, 2022

Re-enable small_model_pt.

686cc73

Re-enable `small_model_pt`. Enabling the current test with the current values. Debugging the values on the CI. More logs ? Printing doesn't work ? Using the CI values instead. Seems to be a Pillow sensitivity.

Narsil force-pushed the reenable_test branch from 41b342a to 686cc73 Compare October 18, 2022 15:07

Narsil requested review from amyeroberts, alaradirik and ydshieh October 18, 2022 15:09

alaradirik approved these changes Oct 18, 2022

View reviewed changes

src/transformers/pipelines/image_segmentation.py Outdated Show resolved Hide resolved

Update src/transformers/pipelines/image_segmentation.py

4282aa3

Co-authored-by: Alara Dirik <8944735+alaradirik@users.noreply.github.com>

Narsil requested a review from sgugger October 19, 2022 07:38

amyeroberts reviewed Oct 19, 2022

View reviewed changes

sgugger approved these changes Oct 19, 2022

View reviewed changes

amyeroberts approved these changes Oct 19, 2022

View reviewed changes

Narsil changed the title ~~Re-enable small_model_pt.~~ image-segmentation pipeline: re-enable small_model_pt test. Oct 19, 2022

Narsil merged commit a403866 into huggingface:main Oct 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`image-segmentation` pipeline: re-enable `small_model_pt` test. #19716

`image-segmentation` pipeline: re-enable `small_model_pt` test. #19716

Narsil commented Oct 18, 2022

HuggingFaceDocBuilderDev commented Oct 18, 2022 •

edited

Loading

Narsil commented Oct 18, 2022

ydshieh commented Oct 18, 2022

alaradirik left a comment

amyeroberts Oct 19, 2022

Narsil Oct 19, 2022

Narsil Oct 19, 2022

amyeroberts Oct 19, 2022

sgugger left a comment

amyeroberts left a comment

image-segmentation pipeline: re-enable small_model_pt test. #19716

image-segmentation pipeline: re-enable small_model_pt test. #19716

Conversation

Narsil commented Oct 18, 2022

Before submitting

Who can review?

HuggingFaceDocBuilderDev commented Oct 18, 2022 • edited Loading

Narsil commented Oct 18, 2022

ydshieh commented Oct 18, 2022

alaradirik left a comment

Choose a reason for hiding this comment

amyeroberts Oct 19, 2022

Choose a reason for hiding this comment

Narsil Oct 19, 2022

Choose a reason for hiding this comment

Narsil Oct 19, 2022

Choose a reason for hiding this comment

amyeroberts Oct 19, 2022

Choose a reason for hiding this comment

sgugger left a comment

Choose a reason for hiding this comment

amyeroberts left a comment

Choose a reason for hiding this comment

`image-segmentation` pipeline: re-enable `small_model_pt` test. #19716

`image-segmentation` pipeline: re-enable `small_model_pt` test. #19716

HuggingFaceDocBuilderDev commented Oct 18, 2022 •

edited

Loading