Add depth estimation pipeline #18618

nandwalritik · 2022-08-14T18:40:32Z

What does this PR do?

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?

Error While using the pipeline

pipe = pipeline("depth-estimation")
No model was supplied, defaulted to Intel/dpt-large and revision e93beec (https://huggingface.co/Intel/dpt-large).
Using a pipeline without specifying a model name and revision in production is not recommended.
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/pipelines/__init__.py", line 670, in pipeline
    framework, model = infer_framework_load_model(
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/pipelines/base.py", line 257, in infer_framework_load_model
    model = model_class.from_pretrained(model, **kwargs)
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 445, in from_pretrained
    model_class = _get_model_class(config, cls._model_mapping)
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 359, in _get_model_class
    supported_models = model_mapping[type(config)]
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 565, in __getitem__
    return self._load_attr_from_module(model_type, model_name)
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 579, in _load_attr_from_module
    return getattribute_from_module(self._modules[module_name], attr)
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 539, in getattribute_from_module
    return getattribute_from_module(transformers_module, attr)
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 539, in getattribute_from_module
    return getattribute_from_module(transformers_module, attr)
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 539, in getattribute_from_module
    return getattribute_from_module(transformers_module, attr)
  [Previous line repeated 982 more times]
  File "/home/nandwalritik/nandwalritik/transformers/src/transformers/models/auto/auto_factory.py", line 538, in getattribute_from_module
    transformers_module = importlib.import_module("transformers")
  File "/home/nandwalritik/anaconda3/envs/hftfSwinDev/lib/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1004, in _find_and_load
  File "<frozen importlib._bootstrap>", line 157, in __enter__
  File "<frozen importlib._bootstrap>", line 183, in _get_module_lock
  File "<frozen importlib._bootstrap>", line 59, in __init__
RecursionError: maximum recursion depth exceeded while calling a Python object

HuggingFaceDocBuilderDev · 2022-08-14T18:51:42Z

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

nandwalritik · 2022-08-15T08:22:49Z

Hi @NielsRogge @Narsil I tried debugging to resolve above issue, I found that in src/transformers/models/auto/auto_factory.py in line 462 model_class = _get_model_class(config, cls._model_mapping) when I logged cls._model_mapping I recieved

OrderedDict([(<class 'transformers...PTConfig'>, <?>), (<class 'transformers...PNConfig'>, <?>)])
<error>:
Traceback (most recent call last):

Can you guide me to resolve this error.

src/transformers/models/auto/modeling_auto.py

Narsil

Great PR, you also need to add

+    def _sanitize_parameters(self, **kwargs):
+        return {}, {}, {}
+

To the pipeline, it's not used but the base class expects this method to exist. (Here there's not parameters declared so it's quite easy.

This weird method exist to allow parameters to be defined both at definition time or call tiime

pipe = pipeline(model=model, myargs=1)
data = pipe(image)
# or
pipe = pipeline(model=model)
data = pipe(image, myargs=1)

Cheers !
Otherwise LGTM.

Why do you output 3 different images ? That sounds like a lot.

The image I can understand, the predicted depth is defined in what unit ? Is it noisy hence the interpolation ?
IMO that seems like something to be left to the user to decide what to do.

In general I think a pure image would be nice (to be a bit more general) but I can understand that the loss of precision might be harmful, do you mind sharing how you use those numbers ? Maybe we could output an other time of image that doesn't loose information (keeping f32 pixel)

Wdyt ?

nandwalritik · 2022-09-01T05:41:02Z

Great PR, you also need to add
+    def _sanitize_parameters(self, **kwargs):
+        return {}, {}, {}
+
To the pipeline, it's not used but the base class expects this method to exist. (Here there's not parameters declared so it's quite easy.

This weird method exist to allow parameters to be defined both at definition time or call tiime
pipe = pipeline(model=model, myargs=1)
data = pipe(image)
# or
pipe = pipeline(model=model)
data = pipe(image, myargs=1)
Cheers ! Otherwise LGTM.

Why do you output 3 different images ? That sounds like a lot.

The image I can understand, the predicted depth is defined in what unit ? Is it noisy hence the interpolation ? IMO that seems like something to be left to the user to decide what to do.

In general I think a pure image would be nice (to be a bit more general) but I can understand that the loss of precision might be harmful, do you mind sharing how you use those numbers ? Maybe we could output an other time of image that doesn't loose information (keeping f32 pixel)

Wdyt ?

I just saw DPT's depth estimation example and added these three outputs. I have removed the interpolation one and In the output I have kept only the predicted_depth (which is a tensor) and depth (which is the PIL Image object).
Let me know if I should remove the predicted_depth also.

Narsil

LGTM.

Aside from the missing generic test.
Would you like us to write it ? (It's not too hard to write I think).

My biggest question is why depth-estimation needs it's own pipeline.

I understand in the robotics context you really want something that's not an image, but really a matrix with depth (expressed iin meters I guess).
This reason only makes me ok with the idea of creating this pipeline as is.

But ideally it would be nice if the output was an image (so it could become image-generation) with either an output that would be trivial to transform into a depth map or a parameter.

@mishig25 maybe for advice ideas on the widget that could be different than an image/generation here ?

In any case, expect if anyone has better ideas counterarguments, we should just merge this right now (after having the generic test), and maybe later convert this to an alias if we decide to merge.

nandwalritik · 2022-09-05T06:24:31Z

Review required

At least 1 approving review is required by reviewers with write access. Learn more.
** 1 pending reviewer **

By generic test did you mean run_pipeline_test ? Let me know if it's other than this, I have added run_pipeline_test for now.
Also In CI test_pipelines_depth_estimation is failing can you help me with that ?

github-actions · 2022-10-09T15:01:56Z

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.

NielsRogge · 2022-10-10T10:25:42Z

Hi @nandwalritik, could you revive this PR by rebasing with the main branch?

nandwalritik · 2022-10-10T11:47:36Z

Hi @nandwalritik, could you revive this PR by rebasing with the main branch?

Done.

Narsil · 2022-10-10T14:15:11Z

@nandwalritik Do you want to help to get the PR green ?

nandwalritik · 2022-10-11T04:36:18Z

@nandwalritik Do you want to help to get the PR green ?

@Narsil Yeah please , I tried but I was not able to make the test cases pass.

Narsil

Added some comments on how I fixed the CI for you.

Narsil · 2022-10-11T10:05:39Z

docs/source/en/main_classes/pipelines.mdx

-<<<<<<< HEAD
-  - [`DocumentQuestionAnsweringPipeline`]
-=======
  - [`DepthEstimationPipeline`]
->>>>>>> Add initial files for depth estimation pipelines


This was a leftover from a bad rebase I think, I removed those lines and put back the documentation in alphabetical order.

Narsil · 2022-10-11T10:05:44Z

docs/source/en/main_classes/pipelines.mdx

 ### DocumentQuestionAnsweringPipeline

 [[autodoc]] DocumentQuestionAnsweringPipeline
    - __call__
    - all
-
-=======
-### DepthEstimationPipeline


Narsil · 2022-10-11T10:06:03Z

src/transformers/pipelines/depth_estimation.py

@@ -1,7 +1,6 @@
 from typing import List, Union

 import numpy as np
-import torch


torch is not a mandatory dependency, we need to import it only when it's OK to do so.

Narsil · 2022-10-11T10:06:11Z

tests/pipelines/test_pipelines_depth_estimation.py

@@ -32,6 +30,9 @@
 from .test_pipelines_common import ANY, PipelineTestCaseMeta


+if is_torch_available():
+    import torch


Same in tests.

Narsil · 2022-10-11T10:12:51Z

tests/pipelines/test_pipelines_depth_estimation.py

-    require_vision,
-    slow,
-)
+from transformers.testing_utils import nested_simplify, require_tf, require_timm, require_torch, require_vision, slow


Since you create your PR and today we removed is_pipeline_test as we're now passing on these tests based on directory not by ENV variable.

tests/pipelines/test_pipelines_depth_estimation.py

nandwalritik · 2022-10-11T11:22:43Z

Added some comments on how I fixed the CI for you.

Thanks I will look at them.

Narsil · 2022-10-11T12:22:22Z

@sgugger for final review.

sgugger

LGTM! Thanks a lot for your work on this!

docs/source/en/main_classes/pipelines.mdx

Co-authored-by: Sylvain Gugger <[email protected]>

Narsil · 2022-10-12T09:33:52Z

@sgugger the test failure seem unrelated to the PR, should we go ahead and merge ?

sgugger · 2022-10-12T12:54:17Z

Yes, those are flaky tests.

nandwalritik force-pushed the add_depth_estimation_pipeline branch from 143d331 to fed82a2 Compare August 15, 2022 04:57

LysandreJik requested review from Narsil and NielsRogge August 16, 2022 06:15

Narsil reviewed Aug 26, 2022

View reviewed changes

src/transformers/models/auto/modeling_auto.py Outdated Show resolved Hide resolved

Narsil reviewed Aug 26, 2022

View reviewed changes

nandwalritik force-pushed the add_depth_estimation_pipeline branch 2 times, most recently from 930d335 to e80aadd Compare September 1, 2022 03:54

Narsil reviewed Sep 2, 2022

View reviewed changes

nandwalritik force-pushed the add_depth_estimation_pipeline branch from 9d57e01 to df4ed06 Compare September 5, 2022 04:07

nandwalritik requested review from Narsil and removed request for NielsRogge and Narsil September 7, 2022 08:15

nandwalritik force-pushed the add_depth_estimation_pipeline branch from 5d97e03 to 7d5dd21 Compare September 15, 2022 10:20

nandwalritik force-pushed the add_depth_estimation_pipeline branch from c5e7df6 to 0a1fb70 Compare October 10, 2022 11:47

Narsil reviewed Oct 11, 2022

View reviewed changes

tests/pipelines/test_pipelines_depth_estimation.py Show resolved Hide resolved

nandwalritik added 2 commits October 11, 2022 14:21

Add initial files for depth estimation pipelines

e41bdd8

Add test file for depth estimation pipeline

12cf1c3

nandwalritik and others added 10 commits October 11, 2022 14:21

Update model mapping names

c48bbc8

Add updates for depth estimation output

88fa61e

Add generic test

766dbae

Hopefully fixing the tests.

7dd8fa2

Check if test passes

2d53427

Add make fixup and make fix-copies changes after rebase with main

b7d4caf

Rebase with main

b5d1338

Fixing up depth pipeline.

1883d71

This is not used anymore.

ce2027e

Fixing the test. Image is a module Image.Image is the type.

31a08bd

Narsil force-pushed the add_depth_estimation_pipeline branch from 4fd5e5f to 31a08bd Compare October 11, 2022 12:22

sgugger approved these changes Oct 11, 2022

View reviewed changes

docs/source/en/main_classes/pipelines.mdx Outdated Show resolved Hide resolved

Update docs/source/en/main_classes/pipelines.mdx

cd48ed5

Co-authored-by: Sylvain Gugger <[email protected]>

sgugger merged commit e94384e into huggingface:main Oct 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add depth estimation pipeline #18618

Add depth estimation pipeline #18618

nandwalritik commented Aug 14, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Aug 14, 2022 •

edited

Loading

nandwalritik commented Aug 15, 2022 •

edited

Loading

Narsil left a comment

nandwalritik commented Sep 1, 2022 •

edited

Loading

Narsil left a comment

nandwalritik commented Sep 5, 2022

Review required

github-actions bot commented Oct 9, 2022

NielsRogge commented Oct 10, 2022

nandwalritik commented Oct 10, 2022

Narsil commented Oct 10, 2022

nandwalritik commented Oct 11, 2022

Narsil left a comment

Narsil Oct 11, 2022

Narsil Oct 11, 2022

Narsil Oct 11, 2022

Narsil Oct 11, 2022

Narsil Oct 11, 2022

nandwalritik commented Oct 11, 2022

Narsil commented Oct 11, 2022

sgugger left a comment

Narsil commented Oct 12, 2022

sgugger commented Oct 12, 2022

Add depth estimation pipeline #18618

Add depth estimation pipeline #18618

Conversation

nandwalritik commented Aug 14, 2022 • edited Loading

What does this PR do?

Before submitting

Who can review?

Error While using the pipeline

HuggingFaceDocBuilderDev commented Aug 14, 2022 • edited Loading

nandwalritik commented Aug 15, 2022 • edited Loading

Narsil left a comment

Choose a reason for hiding this comment

nandwalritik commented Sep 1, 2022 • edited Loading

Narsil left a comment

Choose a reason for hiding this comment

nandwalritik commented Sep 5, 2022

Review required

github-actions bot commented Oct 9, 2022

NielsRogge commented Oct 10, 2022

nandwalritik commented Oct 10, 2022

Narsil commented Oct 10, 2022

nandwalritik commented Oct 11, 2022

Narsil left a comment

Choose a reason for hiding this comment

Narsil Oct 11, 2022

Choose a reason for hiding this comment

Narsil Oct 11, 2022

Choose a reason for hiding this comment

Narsil Oct 11, 2022

Choose a reason for hiding this comment

Narsil Oct 11, 2022

Choose a reason for hiding this comment

Narsil Oct 11, 2022

Choose a reason for hiding this comment

nandwalritik commented Oct 11, 2022

Narsil commented Oct 11, 2022

sgugger left a comment

Choose a reason for hiding this comment

Narsil commented Oct 12, 2022

sgugger commented Oct 12, 2022

nandwalritik commented Aug 14, 2022 •

edited

Loading

HuggingFaceDocBuilderDev commented Aug 14, 2022 •

edited

Loading

nandwalritik commented Aug 15, 2022 •

edited

Loading

nandwalritik commented Sep 1, 2022 •

edited

Loading