fix(DPT,Depth-Anything) `torch.export` #34103

philkuz · 2024-10-12T00:35:02Z

What does this PR do?

Small modification of the DPT modeling code to remove a new object creation in a forward() method of a Module. This object creation makes the model incompatible with torch.export, which is a key part of preparing a model to run on a variety of hardware backends through projects such as ExecuTorch (related issue: #32253)

Motivation

torch.export allows you to export PyTorch models into standardized model representations, intended to be optimized and run efficiently using frameworks such as TensorRT or ExecuTorch.

The Bug

They key issue was the slice on self.layers:

transformers/src/transformers/models/dpt/modeling_dpt.py

Line 696 in 617b212

for hidden_state, layer in zip(hidden_states[1:], self.layers[1:]):

self.layers[1:] creates a new ModuleList() each time this line is executed.

https://github.com/pytorch/pytorch/blob/69bcf1035e7f06f2eefd8986d000cc980e9ebd37/torch/nn/modules/container.py#L330

The model tracer in torch.export monkey-patches nn.Module constructors during evaluation of the forward() pass, so the original DPT modeling code raises the following error:

  File "/home/philkuz/.pyenv/versions/gml311/lib/python3.11/site-packages/torch/nn/modules/container.py", line 293, in __getitem__
      return self.__class__(list(self._modules.values())[idx])
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                 TypeError: _ModuleStackTracer.__init__.<locals>.AttrProxy.__init__() missing 1 required positional argument: 'path'

The Solution

Pytorch recommends users update the modeling code. My team and I figured this could be helpful to the broader community, especially a future where Export to Executorch becomes more widely available: #32253

This also removes an unnecessary creation of a new module list as a bonus.

Tests

I ensured that tests/models/dpt/test_modeling_dpt.py passes, which appears to test a portion of the outputs. I also verified that the entire output of the model
before and after my changes matched with the following script:

import os
import sys

import numpy as np
import requests
import torch
from PIL import Image
from transformers import pipeline

url = "http://images.cocodataset.org/val2017/000000039769.jpg"
image = Image.open(requests.get(url, stream=True).raw)


model = pipeline("depth-estimation", "facebook/dpt-dinov2-base-kitti")
result = model(image)


output_file = "depth_estimation_output.npy"

if not os.path.exists(output_file):
    # Save the current output
    np.save(output_file, result["predicted_depth"])
    print(f"Depth estimation output saved to {output_file}")
    print("Rerun the script to compare the output")
    sys.exit(0)
# Load existing output and compare
expected_output = np.load(output_file)
np.testing.assert_allclose(
    result["predicted_depth"],
    expected_output,
    rtol=1e-5,
    atol=1e-5,
    err_msg="Depth estimation output has changed",
)
print("Depth estimation output matches the saved version.")

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.

@amyeroberts, @qubvel

qubvel

Very nice, thanks for unlocking more models for torch export, this is very valuable!

The same comment as for Mask2Former PR, would be great to have this PR tested, and please push run-slow commit to trigger all tests at the end!

src/transformers/models/dpt/modeling_dpt.py

philkuz · 2024-10-29T23:11:50Z

Very nice, thanks for unlocking more models for torch export, this is very valuable!

The same comment as for Mask2Former PR, would be great to have this PR tested, and please push run-slow commit to trigger all tests at the end!

I've added Depth-anything to this PR, I'm not entirely sure if I've triggered the run_slow test for it and DPT correctly. Happy to split it off into a separate PR.

Also ran into an issue with zoedepth not working because of the beit backend. I suspect that will take more time to properly addres, I added a skipped test to zoedepth, but I can also remove that test entirely and add it in a WIP PR.

qubvel

Thanks for the update! Regarding ZoeDepth I suggest either fixing the model export or excluding this model from the PR. A skipped test is not the best solution, cause it might stuck in this state for a very long time 😄

To trigger multiple models' slow tests you can list them as follows [run_slow] depth_anything, dpt, zoedepth

philkuz · 2024-10-30T16:28:24Z

Thanks for the update! Regarding ZoeDepth I suggest either fixing the model export or excluding this model from the PR. A skipped test is not the best solution, cause it might stuck in this state for a very long time 😄

To trigger multiple models' slow tests you can list them as follows [run_slow] depth_anything, dpt, zoedepth

I have to add some of the model changes because of the copy-consistency check, but I'll remove the Relu change and the torch.export test!

Thanks for the heads up on slow tests.

philkuz · 2024-10-30T17:11:14Z

@qubvel could you approve the slow workflow?

qubvel · 2024-10-30T17:38:04Z

I have to add some of the model changes because of the copy-consistency check
Can you provide a bit more details on this? Can I somehow help to enable torch export for ZoeDepth?

philkuz · 2024-10-30T17:46:43Z

I have to add some of the model changes because of the copy-consistency check
Can you provide a bit more details on this? Can I somehow help to enable torch export for ZoeDepth?

I'm not 100% sure that this is part of the CI, but the contributing guide asks you to run repo-consistency

transformers/CONTRIBUTING.md

Line 217 in bc0633a

make repo-consistency

which throws an error in python utils/check_copies.py if you don't update ZoeDepth to match DPT. (ZoeDepth copied many layers from DPT
https://github.com/philkuz/transformers/blob/bc0633a82cbfe8d828fa2d3b432dfde4fbd2f0e5/src/transformers/models/zoedepth/modeling_zoedepth.py#L175
)

So basically I have to include those shared changes: https://github.com/huggingface/transformers/pull/34103/files#diff-02337c86e3fba49173cf2cb6fa1595ed168db19726938aec925b8b010a3b6a8c

The current crux of ZoeDepth is that the BEIT model, the backbone of all the HF hub models for ZoeDepth, isn't compatible. So you have to address that issue, which I have not had time to address yet.

HuggingFaceDocBuilderDev · 2024-10-30T18:03:01Z

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.

philkuz · 2024-10-30T18:11:06Z

The slow_tests are failing, but I think they're broken on main as well. Here's a repro:

git checkout main
# DPT Failures
CUDA_VISIBLE_DEVICES="" RUN_SLOW=true pytest tests/models/dpt/test_modeling_dpt_auto_backbone.py -v  -k=test_inference_depth_estimation_dinov2
# Depth-anything failures
CUDA_VISIBLE_DEVICES="" RUN_SLOW=true pytest tests/models/depth_anything/test_modeling_depth_anything.py -v  -k test_inference

The following also has checks on the output of slices and those checks seem to work.

CUDA_VISIBLE_DEVICES="" RUN_SLOW=true pytest tests/models/dpt/test_modeling_dpt.py -v  -k=test_inference

I went ahead and made a PR to try and address this issue: #34518

qubvel · 2024-10-30T19:08:34Z

I'm not 100% sure that this is part of the CI, but the contributing guide asks you to run repo-consistency

Ahh, ok, it's because model modules contain "Copied from" statements and these parts are synced across models. No worries then!

Signed-off-by: Phillip Kuznetsov <[email protected]>

qubvel

Thanks for updating it!

cc @guangy10

qubvel · 2024-10-31T16:59:28Z

src/transformers/models/zoedepth/modeling_zoedepth.py

+        fused_hidden_state = None
+        for hidden_state, layer in zip(hidden_states, self.layers):
+            if fused_hidden_state is None:
+                # first layer only uses the last hidden_state
+                fused_hidden_state = layer(hidden_state)
+            else:
+                fused_hidden_state = layer(fused_hidden_state, hidden_state)


Comment for final review:

This change included in the ZoeDepth model because of the "Copied from" statement, it doesn't unlock torch export for the model, however will be useful if we decide to enable it

guangy10

LGTM! Thanks for the contribution.

guangy10 · 2024-11-01T00:18:44Z

Extend the script in pytorch/executorch#6509 to support lowering (with the simplest recipe) the DepthEstimation and SemanticSegmentation models enabled in this PR.

The dpt model works as expected.
However, the depth-anything model fails due to an unsupported dim order. ExecuTorch supports dim_order: (0, 1, 2, 3) but got dim_order: (0, 2, 3, 1) for a placeholder node aten_clone_default. There seems to be a way to insert a compiler pass to fix it w/o requiring changing the source code. I will give a try.

philkuz · 2024-11-04T16:54:09Z

Extend the script in pytorch/executorch#6509 to support lowering (with the simplest recipe) the DepthEstimation and SemanticSegmentation models enabled in this PR.

The dpt model works as expected. However, the depth-anything model fails due to an unsupported dim order. ExecuTorch supports dim_order: (0, 1, 2, 3) but got dim_order: (0, 2, 3, 1) for a placeholder node aten_clone_default. There seems to be a way to insert a compiler pass to fix it w/o requiring changing the source code. I will give a try.

Any luck on the compiler pass?

Also do you think that this gates the support for torch.export? Seems like this is Executorch specific. Maybe we can scope this PR down to be more for torch.export generally and focus on adding support for Executorch in another PR? Happy to help with that

guangy10 · 2024-11-04T18:22:57Z

Extend the script in pytorch/executorch#6509 to support lowering (with the simplest recipe) the DepthEstimation and SemanticSegmentation models enabled in this PR.
The dpt model works as expected. However, the depth-anything model fails due to an unsupported dim order. ExecuTorch supports dim_order: (0, 1, 2, 3) but got dim_order: (0, 2, 3, 1) for a placeholder node aten_clone_default. There seems to be a way to insert a compiler pass to fix it w/o requiring changing the source code. I will give a try.

Any luck on the compiler pass?

@philkuz Sorry, haven't got a chance to write the pass yet.

Also do you think that this gates the support for torch.export? Seems like this is Executorch specific. Maybe we can scope this PR down to be more for torch.export generally and focus on adding support for Executorch in another PR? Happy to help with that

Right, it's ExecuTorch specific, i.e. all tensors need to be contiguous. BTW, do you happen to know where the channel_last tensor may come from the eager, we can fix it here, otherwise, having a separate PR for ExecuTorch is fine. Please note that unlike compiled artifact the exported program is just an intermediate representation, typically should only being used as the entry for further optimizations, i.e. ExecuTorch.

philkuz · 2024-11-06T16:56:11Z

Extend the script in pytorch/executorch#6509 to support lowering (with the simplest recipe) the DepthEstimation and SemanticSegmentation models enabled in this PR.
The dpt model works as expected. However, the depth-anything model fails due to an unsupported dim order. ExecuTorch supports dim_order: (0, 1, 2, 3) but got dim_order: (0, 2, 3, 1) for a placeholder node aten_clone_default. There seems to be a way to insert a compiler pass to fix it w/o requiring changing the source code. I will give a try.

Any luck on the compiler pass?

@philkuz Sorry, haven't got a chance to write the pass yet.

Also do you think that this gates the support for torch.export? Seems like this is Executorch specific. Maybe we can scope this PR down to be more for torch.export generally and focus on adding support for Executorch in another PR? Happy to help with that

Right, it's ExecuTorch specific, i.e. all tensors need to be contiguous. BTW, do you happen to know where the channel_last tensor may come from the eager, we can fix it here, otherwise, having a separate PR for ExecuTorch is fine. Please note that unlike compiled artifact the exported program is just an intermediate representation, typically should only being used as the entry for further optimizations, i.e. ExecuTorch.

I did a very quick scan for the channel_last tensor, and I believe it's in DINOv2 (the backbone) which is not part of this particular modeling code. I think we should move it to another PR IMO.

guangy10 · 2024-11-19T20:16:35Z

Any other block for merging this PR?

qubvel · 2024-11-19T21:10:41Z

@guangy10 no blocks IMO, waiting for @ArthurZucker's review, he has quite a few in line

ArthurZucker

Great contribution! Thanks all for iterating 🤗
Super good in general as slicing does not go well with compile either!

ArthurZucker · 2024-11-20T10:31:39Z

Sorry for the delay @guangy10 we were on a company wide offsite! 🌴

philkuz force-pushed the torch_export_dpt_based_models branch from aa7d562 to 2e15b57 Compare October 12, 2024 00:36

philkuz changed the title ~~Fix torch.export issue in dpt based models~~ Fix torch.export issue in DPT based models Oct 14, 2024

philkuz changed the title ~~Fix torch.export issue in DPT based models~~ Support torch.export/ExecuTorch for DPT-based models Oct 14, 2024

philkuz changed the title ~~Support torch.export/ExecuTorch for DPT-based models~~ Make DPT-based models compatible with torch.export Oct 14, 2024

qubvel added ExecuTorch run-slow labels Oct 29, 2024

qubvel reviewed Oct 29, 2024

View reviewed changes

src/transformers/models/dpt/modeling_dpt.py Outdated Show resolved Hide resolved

philkuz changed the title ~~Make DPT-based models compatible with torch.export~~ fix(DPT,Depth-Anything) torch.export Oct 29, 2024

philkuz force-pushed the torch_export_dpt_based_models branch from 2e15b57 to 4c65b67 Compare October 29, 2024 23:09

qubvel self-requested a review October 30, 2024 07:50

qubvel reviewed Oct 30, 2024

View reviewed changes

philkuz force-pushed the torch_export_dpt_based_models branch from 4c65b67 to bc0633a Compare October 30, 2024 16:33

philkuz mentioned this pull request Oct 30, 2024

fix(DPT,Depth-Anything) Address expected_slice errors inside inference tests #34518

Merged

5 tasks

philkuz added 7 commits October 31, 2024 09:48

Fix torch.export issue in dpt based models

9193fbd

Signed-off-by: Phillip Kuznetsov <[email protected]>

Simplify the if statements

c7f9c47

Signed-off-by: Phillip Kuznetsov <[email protected]>

Move activation definitions of zoe_depth to init()

3e28a48

Signed-off-by: Phillip Kuznetsov <[email protected]>

Add test_export for dpt and zoedepth

661d5b7

Signed-off-by: Phillip Kuznetsov <[email protected]>

add depth anything

812e3d5

Signed-off-by: Phillip Kuznetsov <[email protected]>

Remove zoedepth non-automated zoedepth changes and zoedepth test

cdc9cad

Signed-off-by: Phillip Kuznetsov <[email protected]>

[run_slow] dpt, depth_anything, zoedepth

c7ddd0f

Signed-off-by: Phillip Kuznetsov <[email protected]>

philkuz force-pushed the torch_export_dpt_based_models branch from fd1b352 to c7ddd0f Compare October 31, 2024 16:49

qubvel added the Vision label Oct 31, 2024

qubvel approved these changes Oct 31, 2024

View reviewed changes

qubvel requested a review from ArthurZucker October 31, 2024 17:04

guangy10 approved these changes Oct 31, 2024

View reviewed changes

philkuz closed this Nov 4, 2024

philkuz reopened this Nov 4, 2024

ArthurZucker approved these changes Nov 20, 2024

View reviewed changes

ArthurZucker merged commit 8cadf76 into huggingface:main Nov 20, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(DPT,Depth-Anything) `torch.export` #34103

fix(DPT,Depth-Anything) `torch.export` #34103

philkuz commented Oct 12, 2024 •

edited

Loading

qubvel left a comment

philkuz commented Oct 29, 2024

qubvel left a comment

philkuz commented Oct 30, 2024 •

edited

Loading

philkuz commented Oct 30, 2024

qubvel commented Oct 30, 2024

philkuz commented Oct 30, 2024 •

edited

Loading

HuggingFaceDocBuilderDev commented Oct 30, 2024

philkuz commented Oct 30, 2024 •

edited

Loading

qubvel commented Oct 30, 2024 •

edited

Loading

qubvel left a comment

qubvel Oct 31, 2024

guangy10 left a comment

guangy10 commented Nov 1, 2024 •

edited

Loading

philkuz commented Nov 4, 2024

guangy10 commented Nov 4, 2024

philkuz commented Nov 6, 2024

guangy10 commented Nov 19, 2024

qubvel commented Nov 19, 2024

ArthurZucker left a comment

ArthurZucker commented Nov 20, 2024

fix(DPT,Depth-Anything) torch.export #34103

fix(DPT,Depth-Anything) torch.export #34103

Conversation

philkuz commented Oct 12, 2024 • edited Loading

What does this PR do?

Motivation

The Bug

The Solution

Tests

Before submitting

Who can review?

qubvel left a comment

Choose a reason for hiding this comment

philkuz commented Oct 29, 2024

qubvel left a comment

Choose a reason for hiding this comment

philkuz commented Oct 30, 2024 • edited Loading

philkuz commented Oct 30, 2024

qubvel commented Oct 30, 2024

philkuz commented Oct 30, 2024 • edited Loading

HuggingFaceDocBuilderDev commented Oct 30, 2024

philkuz commented Oct 30, 2024 • edited Loading

qubvel commented Oct 30, 2024 • edited Loading

qubvel left a comment

Choose a reason for hiding this comment

qubvel Oct 31, 2024

Choose a reason for hiding this comment

guangy10 left a comment

Choose a reason for hiding this comment

guangy10 commented Nov 1, 2024 • edited Loading

philkuz commented Nov 4, 2024

guangy10 commented Nov 4, 2024

philkuz commented Nov 6, 2024

guangy10 commented Nov 19, 2024

qubvel commented Nov 19, 2024

ArthurZucker left a comment

Choose a reason for hiding this comment

ArthurZucker commented Nov 20, 2024

fix(DPT,Depth-Anything) `torch.export` #34103

fix(DPT,Depth-Anything) `torch.export` #34103

philkuz commented Oct 12, 2024 •

edited

Loading

philkuz commented Oct 30, 2024 •

edited

Loading

philkuz commented Oct 30, 2024 •

edited

Loading

philkuz commented Oct 30, 2024 •

edited

Loading

qubvel commented Oct 30, 2024 •

edited

Loading

guangy10 commented Nov 1, 2024 •

edited

Loading