Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ZoeDepth #30136

Merged
merged 108 commits into from
Jul 8, 2024
Merged

Add ZoeDepth #30136

merged 108 commits into from
Jul 8, 2024

Conversation

NielsRogge
Copy link
Contributor

@NielsRogge NielsRogge commented Apr 9, 2024

What does this PR do?

This PR adds ZoeDepth as introduced in ZoeDepth: Zero-shot Transfer by Combining Relative and Metric Depth.

To do:

  • double check image processor (not sure we can support the same resize). Update image processor accordingly
  • remove testing scripts
  • verify relative position bias table/index when loading a beit model from the hub
  • add slow integration test
  • add image processor tests
  • should we add backbone_hidden_size?
  • make doc tests pass

@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.

@NielsRogge
Copy link
Contributor Author

@amyeroberts feel free to approve the PR as all comments have been addressed

@amyeroberts
Copy link
Collaborator

@NielsRogge Have you tested for the issues related to weight loading / DPT?

@NielsRogge
Copy link
Contributor Author

Yes, can confirm:

>>> from transformers import ZoeDepthForDepthEstimation
>>> model = ZoeDepthForDepthEstimation.from_pretrained("Intel/zoedepth-nyu-kitti")
config.json: 100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 2.23k/2.23k [00:00<00:00, 1.41MB/s]
model.safetensors: 100%|███████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 1.38G/1.38G [00:29<00:00, 47.2MB/s]
>>> model = ZoeDepthForDepthEstimation.from_pretrained("Intel/zoedepth-nyu")
config.json: 100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 2.13k/2.13k [00:00<00:00, 3.27MB/s]
model.safetensors: 100%|███████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 1.38G/1.38G [00:29<00:00, 46.4MB/s]
>>> model = ZoeDepthForDepthEstimation.from_pretrained("Intel/zoedepth-kitti")
config.json: 100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 2.12k/2.12k [00:00<00:00, 1.52MB/s]
model.safetensors: 100%|███████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 1.38G/1.38G [00:29<00:00, 46.0MB/s]
>>> 

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for running all the tests to confirm the model behaviours!

There's still some comments which weren't resolved. In particular, the behaviour of the image processor's arguments wrt keep_aspect_ratio and ensure_multiple_of need to be throughly tested


def test_keep_aspect_ratio(self):
size = {"height": 512, "width": 512}
image_processor = ZoeDepthImageProcessor(size=size, keep_aspect_ratio=True, ensure_multiple_of=32)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enusre_multiple_of should also be tested with keep_aspect_ratio=False

@NielsRogge
Copy link
Contributor Author

@amyeroberts addressed your comment, failing CI is unrelated.

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the continued work on this!

There's still a bit of work do to on the testing and documentation of keep_aspect_ratio and ensure_multiple_of, otherwise the PR looks good!

I realise it might seem like I'm being picky here, but there's two reasons why this is important:

  • Users should be able to read the docstring and know how to use the objects. At the moment, the descriptions mean that someone still has to go look at the code.
  • There have been quite a few fixes to resizing logic for models like e.g. yolos. This wasn't caught because the behaviour wasn't properly tested. It's important we test thoroughly and correctly now, as it's harder to fix post-merge.

In particular, when testing, it's fine if some of the logic seems repetitive. Tests should be DAMP rather than DRY. They serve not just as a safety net, but also documentation. As such, it's important we isolate so we're testing a single idea at a time, and the behaviour being tested is obvious (e.g. having the same output with keep_aspect_ratio as True and False doesn't tell us about what keep_aspect_ratio does).

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work adding this model!

@NielsRogge
Copy link
Contributor Author

Slow tests passing locally, CI failing tests are unrelated, merging.

@NielsRogge NielsRogge merged commit 06fd797 into huggingface:main Jul 8, 2024
25 checks passed
@amyeroberts
Copy link
Collaborator

@NielsRogge Slow tests have to pass on the CI runs, not locally before merging. Please do not forcibly merge like this again. There are numerical differences which are introduced from the running environment and hardware which mean integration values may not match. You'll need to follow this up and make sure the slow tests pass on our CI images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants