Add post_process_depth_estimation to image processors and support ZoeDepth's inference intricacies

[alex-bene]

What does this PR do?

This PR adds a post_process_depth_estimation method for the image processors of depth estimation models, similar to the post_process_semantic_segmentation methods for the segmentation models. Also, it updates the depth estimation pipeline to use the new post_process_depth_estimation method. Lastly, it adds full support for the ZoeDepth special inference (dynamically padded input + inference of the flipped of the input). A small update to the documentation is pending.

Fixes #30917 #32381

Before submitting

• 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?

@NielsRogge @amyeroberts @Narsil

[alex-bene]

I'm not sure who to tag about this, but I have added to the image_transforms.py file the function colorize_depth, which, given a depth prediction, generates a colored PIL image or numpy image. If this is not the correct place for this function let me know. @NielsRogge

[alex-bene]

Hey everyone, @NielsRogge @amyeroberts @Narsil, is there anything missing from this PR so that it can be merged?

[amyeroberts]

Thanks for all the investigation work and adding this logic!

Most comments are about simplifying the added logic and bringing it more in-line with transformers. Overall looks great!

[amyeroberts]

The post-processing methods for image processors should follow a similar pattern to the tokenizers i.e. accept and return the same array type (numpy arrays or torch tensors) e.g. like here for batch_decode. No need to bother with tensorflow tensors at the moment as we have so few TF vision models. We shouldn't be converting to PIL.Image.Image

[alex-bene]

Hey @ArthurZucker , regarding your question, initially the post-processing method returned a PIL image too, however, my understanding is that this functionality did not follow the "usual" transformers conventions. We can easily add it back though.

[alex-bene]

@amyeroberts let me know if there's anything else that needs changing.

[amyeroberts]

Thanks for the continued work on this!

Mostly nits on formatting and docs

[alex-bene]

@amyeroberts kind reminder

[amyeroberts]

Looks great!

The only outstanding question is about the handling of different input image sizes.

[amyeroberts]

Linking to the outstanding comment here, in case it's been lost in review, as it's always hard to find again in github: #32550 (comment)

[alex-bene]

Hey @amyeroberts , I finished up with the last pending comments; let me know if it seems ok.

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[alex-bene]

Hey @amyeroberts , in the docs, I have added a code block and a link inside a "tip" block that did not render markdown assets as usual, as seen in the image. Do you have a good idea of how to fix this?

[amyeroberts]

@alex-bene Tbh, I'm not sure. I'd suggest just removing the tip tags altogether -- the section follows on directly from the part above, so I don't think this will impact the message in the docs much

[amyeroberts]

Looks great - thanks for iterating on this!

Just two nits on the docs and fixing the rendering for the top section. Once done we're good to merge!

[amyeroberts]

This isn't right - it's not returning a list of dictionaries

[amyeroberts]

Same here

[alex-bene]

@alex-bene Tbh, I'm not sure. I'd suggest just removing the tip tags altogether -- the section follows on directly from the part above, so I don't think this will impact the message in the docs much

Hey @amyeroberts , I was thinking of doing something like this if it makes sense:

<Tip>
<p>In the <a href="https://github.com/isl-org/ZoeDepth/blob/edb6daf45458569e24f50250ef1ed08c015f17a7/zoedepth/models/depth_model.py#L131">original implementation</a> ZoeDepth model performs inference on both the original and flipped images and averages out the results. The <code>post_process_depth_estimation</code> function can handle this for us by passing the flipped outputs to the optional <code>outputs_flipped</code> argument:</p>
<pre><code class="language-Python">&gt;&gt;&gt; with torch.no_grad():   
...     outputs = model(pixel_values)
...     outputs_flipped = model(pixel_values=torch.flip(inputs.pixel_values, dims=[3]))
&gt;&gt;&gt; post_processed_output = image_processor.post_process_depth_estimation(
...     outputs,
...     source_sizes=[image.size[::-1]],
...     outputs_flipped=outputs_flipped,
... )
</code></pre>
</Tip>

[amyeroberts]

@alex-bene Tbh, I'm not sure. I'd suggest just removing the tip tags altogether -- the section follows on directly from the part above, so I don't think this will impact the message in the docs much

Hey @amyeroberts , I was thinking of doing something like this if it makes sense:

<Tip>
<p>In the <a href="https://github.com/isl-org/ZoeDepth/blob/edb6daf45458569e24f50250ef1ed08c015f17a7/zoedepth/models/depth_model.py#L131">original implementation</a> ZoeDepth model performs inference on both the original and flipped images and averages out the results. The <code>post_process_depth_estimation</code> function can handle this for us by passing the flipped outputs to the optional <code>outputs_flipped</code> argument:</p>
<pre><code class="language-Python">&gt;&gt;&gt; with torch.no_grad():   
...     outputs = model(pixel_values)
...     outputs_flipped = model(pixel_values=torch.flip(inputs.pixel_values, dims=[3]))
&gt;&gt;&gt; post_processed_output = image_processor.post_process_depth_estimation(
...     outputs,
...     source_sizes=[image.size[::-1]],
...     outputs_flipped=outputs_flipped,
... )
</code></pre>
</Tip>

You can always try and see how it renders :)

[alex-bene]

Yeah, it seems to have done the trick @amyeroberts -- I think we're good!

[amyeroberts]

@alex-bene Great! Could you resolve the conflicts? Once done I think we're good to merge!

cc @qubvel

[qubvel]

I'd like to add a comment regarding the target size. I believe the following format improves readability.

[alex-bene]

@amyeroberts @qubvel I think we're done, but let me know if there's anything else pending.

[qubvel]

Thanks for update!

[alex-bene]

@qubvel is it now ready to merge?

[alex-bene]

@qubvel @amyeroberts kind reminder

[qubvel]

@alex-bene thanks for ping!

cc @ArthurZucker for final review

[ArthurZucker]

Thanks! Left a small question but good otherwise!

[ArthurZucker]

while we are at it , is there a reason not to do this in the image processing? (*225, divide by max etc?

[alex-bene]

Hey @ArthurZucker , check here please.

[ArthurZucker]

same Q here!

[ArthurZucker]

Ok thanks, merging as you answered 🤗

[ydshieh]

Hi @alex-bene Thank you for the contribution!

There are 2 failing tests after this PR is merged to main:

(GLPN model)

FAILED tests/models/glpn/test_modeling_glpn.py::GLPNModelTest::test_pipeline_depth_estimation - AttributeError: 'GLPNImageProcessor' object has no attribute 'post_process_depth_estimation'

FAILED tests/models/glpn/test_modeling_glpn.py::GLPNModelTest::test_pipeline_depth_estimation_fp16 - AttributeError: 'GLPNImageProcessor' object has no attribute 'post_process_depth_estimation'

Could you take a look? Thanks in advance.

job run page

[alex-bene]

Hey @ydshieh , that's because this is a new model not available when we initially added the post processing interface for depth models. I'll try to make the necessary changes until ~tomorrow.

[alex-bene]

Hey @ydshieh , the fix for this is on #34413