Factored out some code in the `image-segmentation` pipeline. #19727

Narsil · 2022-10-18T14:09:32Z

What does this PR do?

Modifies the pipeline's internal a little.

Hopefully less code, so easier to read.
Removed the "blank mask" when nothing gets detected. Sending [] instead
is more obvious IMO (and used to be the behavior, so making it non breaking change).
Changed the default of subtask to None instead of semantic. panoptic used to be the default, and I merely adopted the same behavior. If a user specifies subtask=x we definitely try to use it's task and error out when it's impossible. But when nothing gets passed (should be the most common case), then we try in order "panoptic", "instance", , and "semantic". Since roughly they are in decreasing order of expressiveness.
Added Detr as an exception to sending empty lists in the randomly generated
models (Like maskformer, it can return nothing because it has an empty slot
it seems)
Renamed task to subtask since pipeline(task="image-segmentation") already exists and would be impossible to use from pipeline directly. I feel subtask is appropriate but welcome any suggestions there.

TODO : figure out the real bug that makes panoptic on the tiny detr:

Figured it out. Not sure what the original code was doing but it was outputting two masks for LABEL_215.
What happens is that pred_masks is a tensor of shape (2, H, W) where 2 is the number of class queries.
Now we only have one mask because we have for all pixels pred_masks[0, :, :] == pred_masks[1, :, :]. So when we do
pred_masks.argmax(dim=0) to get the class they are associated with, everything gets attributed to class 0, and no pixel to class 1.
Since mask 1 is empty, we exclude it (which is good).

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-18T15:49:00Z

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

Narsil · 2022-10-18T16:03:59Z

Okay, I've pinpointed out the filter that filters OUT the prediction for panoptic:
(https://huggingface.slack.com/archives/D03VB3U5CVC/p1666108719919289)
36f52e9
(https://huggingface.slack.com/archives/D03VB3U5CVC/p1666108795507369)
It's this line specifically : https://github.com/huggingface/transformers/pull/19262/files#diff-826e0e0303aa4a12e527884e4dc548fe8debb137905260e42ec8b4331c9c2c63R199
(https://huggingface.slack.com/archives/D03VB3U5CVC/p1666108939394469)
For the random model, the predictions are rather low (score == 0.004) and so >0.5 filter them out, basically loosing the predictions right there.I'm not sure who is correct, but as mentionned earlier, if semantic is saying something, I feel like panoptic without any thresholds should ALWAYS output at least as much.

I think the >0.5 comes from the expectations that sigmoid would have put the range of probs into something normalized and make sure something was in the >0.5 range.I don't really have the time to figure out where the logic fails (if it does) ?

Without this line I recover a LABEL_215 label in the small_model_pt test (not two though) but with the same score, which is encouraging IMO.

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. Added a test showcasing that models not supporting some tasks get a clear error. Factored out code. Further factor out. Fixup. Bad rebase. Put `panoptic` before `instance` as it should be a superset.

Narsil · 2022-10-24T18:36:02Z

@alaradirik @amyeroberts This PR is ready for review.

Don't hesitate to voice any concern over the changes. This mostly code cleanup. Except for the empty mask being removed (which wasn't the behavior before, and is not tested against).

sgugger

LGTM in general, but please wait for @alaradirik review as well since she knows this code more than I do :-)

amyeroberts

LGTM! Thanks for the refactor :)

Two Qs (not for this PR but to understand what's covered) :

Do we have the default subtask selection behaviour, panoptic -> instance -> semantic, documented anywhere?
Do we have a test to make sure the expected segmentation task is selected?

src/transformers/pipelines/image_segmentation.py

tests/pipelines/test_pipelines_image_segmentation.py

+ Fixes `instance` segmentation which was broken due to default and non kwargs arguments.

Narsil · 2022-10-25T15:03:07Z

src/transformers/models/detr/feature_extraction_detr.py

+                pred_labels=pred_labels_item,
+                mask_threshold=mask_threshold,
+                overlap_mask_area_threshold=overlap_mask_area_threshold,
+                label_ids_to_fuse=[],


@alaradirik While testing instance subtask, I found that the returned masks did not respect target_size.
I pinpointed that this call was sending target_size to ids_to_fuse instead of target_size.

post_process_instance_segmentation doesn't take a label_ids_to_fuse as a parameter, is that expected ?

Future note:

We can write functions as :

def myfunction(*, arg1, arg2): pass

And this forbids the caller from calling myfunction(a, b) he is forced to use myfunction(arg1=a, arg2=b) helping prevent this sort of issue.
I don't think we have to do it, just saying that it's a handy tool.

Thanks for the target_size fix @Narsil! And yes, post_process_instance_segmentation doesn't and shouldn't take a label_ids_to_fuse as a parameter.

I'm more in favor of using named arguments but both are fine with me really.

alaradirik

All looks good to me! Thanks for the fix and code clean up.

alaradirik · 2022-10-25T18:12:02Z

src/transformers/models/detr/feature_extraction_detr.py

+                pred_labels=pred_labels_item,
+                mask_threshold=mask_threshold,
+                overlap_mask_area_threshold=overlap_mask_area_threshold,
+                label_ids_to_fuse=[],


Thanks for the target_size fix @Narsil! And yes, post_process_instance_segmentation doesn't and shouldn't take a label_ids_to_fuse as a parameter.

I'm more in favor of using named arguments but both are fine with me really.

…face#19727) * Factored out some code in the image-segmentation pipeline 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. Added a test showcasing that models not supporting some tasks get a clear error. Factored out code. Further factor out. Fixup. Bad rebase. Put `panoptic` before `instance` as it should be a superset. * Fixing tests. * Adding subtasks tests + Fixes `instance` segmentation which was broken due to default and non kwargs arguments. * Fix bad replace.

Narsil force-pushed the fixing_image_semantic branch 2 times, most recently from 16dfda9 to 83a9ba2 Compare October 18, 2022 15:09

Narsil requested review from alaradirik and amyeroberts October 18, 2022 16:02

Narsil force-pushed the fixing_image_semantic branch from b3f6541 to 0a116e2 Compare October 24, 2022 17:46

Fixing tests.

5ab549f

Narsil requested a review from sgugger October 24, 2022 18:31

sgugger approved these changes Oct 24, 2022

View reviewed changes

amyeroberts approved these changes Oct 25, 2022

View reviewed changes

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

tests/pipelines/test_pipelines_image_segmentation.py Outdated Show resolved Hide resolved

Narsil added 2 commits October 25, 2022 16:57

Adding subtasks tests

c86e741

+ Fixes `instance` segmentation which was broken due to default and non kwargs arguments.

Fix bad replace.

090ee37

Narsil commented Oct 25, 2022

View reviewed changes

alaradirik approved these changes Oct 25, 2022

View reviewed changes

Narsil merged commit 5fd5990 into huggingface:main Oct 26, 2022

Narsil deleted the fixing_image_semantic branch October 26, 2022 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factored out some code in the `image-segmentation` pipeline. #19727

Factored out some code in the `image-segmentation` pipeline. #19727

Narsil commented Oct 18, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Oct 18, 2022 •

edited

Loading

Narsil commented Oct 18, 2022 •

edited

Loading

Narsil commented Oct 24, 2022

sgugger left a comment

amyeroberts left a comment

Narsil Oct 25, 2022

alaradirik Oct 25, 2022

alaradirik left a comment

alaradirik Oct 25, 2022

Factored out some code in the image-segmentation pipeline. #19727

Factored out some code in the image-segmentation pipeline. #19727

Conversation

Narsil commented Oct 18, 2022 • edited Loading

What does this PR do?

Before submitting

Who can review?

HuggingFaceDocBuilderDev commented Oct 18, 2022 • edited Loading

Narsil commented Oct 18, 2022 • edited Loading

Narsil commented Oct 24, 2022

sgugger left a comment

Choose a reason for hiding this comment

amyeroberts left a comment

Choose a reason for hiding this comment

Narsil Oct 25, 2022

Choose a reason for hiding this comment

alaradirik Oct 25, 2022

Choose a reason for hiding this comment

alaradirik left a comment

Choose a reason for hiding this comment

alaradirik Oct 25, 2022

Choose a reason for hiding this comment

Factored out some code in the `image-segmentation` pipeline. #19727

Factored out some code in the `image-segmentation` pipeline. #19727

Narsil commented Oct 18, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Oct 18, 2022 •

edited

Loading

Narsil commented Oct 18, 2022 •

edited

Loading