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 DAB-DETR Object detection/segmentation model #30803

Open
wants to merge 106 commits into
base: main
Choose a base branch
from

Conversation

conditionedstimulus
Copy link
Contributor

What does this PR do?

Add DAB-DETR Object detection model. Paper: https://arxiv.org/abs/2201.12329
Original code repo: https://github.com/IDEA-Research/DAB-DETR

Fixes # (issue)
[WIP] This model is part of how DETR models have evolved, alongside DN DETR (not part of this PR), to pave the way for newer and better models like Dino and Stable Dino in object detection

Who can review?

@amyeroberts

@amyeroberts
Copy link
Collaborator

Hi @conditionedstimulus, thanks for opening a PR!

Just skimming over the modeling files, it looks like all of the modules are copied from, or can be copied from conditional DETR. Are there any architectural changes this model brings? If not, then all we need to do is convert the checkpoints and upload those to the hub such that they can be loaded in ConditionalDETR directly

@conditionedstimulus
Copy link
Contributor Author

Hi @conditionedstimulus, thanks for opening a PR!

Just skimming over the modeling files, it looks like all of the modules are copied from, or can be copied from conditional DETR. Are there any architectural changes this model brings? If not, then all we need to do is convert the checkpoints and upload those to the hub such that they can be loaded in ConditionalDETR directly

Hi Amy,

I attached a photo comparing the cross-attention of the decoder in DETR, Conditional DETR, and DAB DETR, as this is the main architectural difference. I copied the code from Conditional DETR because this model is an extension/evolved version of Conditional DETR. I believe it would be cool and useful to include this model in the HF object detection collection.
Screenshot 2024-05-15 at 22 25 15

@amyeroberts
Copy link
Collaborator

@conditionedstimulus Thanks for sharing! OK, seems useful to have this available as an option as part of the DETR family in the library. Feel free to ping me when the PR is ready for review.

cc @qubvel for reference

@qubvel
Copy link
Member

qubvel commented Jan 21, 2025

btw, I see there is a "DAB-Deformable-DETR-R50-v2" checkpoint on github which has the best metrics, is it possible to convert it as well?

Copy link
Member

@qubvel qubvel left a comment

Choose a reason for hiding this comment

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

Tested inference and training as well, all goes smoothly:

Final steps to merge the model:

  • Merge/rebase on main
  • Run slow tests (I can trigger it, as soon as main merged)
  • Convert the last checkpoint, mentioned above
  • Add model cards for all checkpoints on the Hub
  • Transfer checkpoints to IDEA-Research org (also on my side, only if it's ok with you) + correct checkpoint's path in code
  • Merge!

Thank you, @conditionedstimulus, for the iterations, and apologize for the delays on our end. 🤗

@conditionedstimulus
Copy link
Contributor Author

Hi @qubvel,

Thank you for your feedback and review! Here's an update on my progress:

  • I merged the main branch into the feature branch.
  • I ran the slow tests.
  • I updated the model cards.
  • Yes, please copy the model cards to IDEA-Research as it is their work, I already updated the 'how to use' part with their source in the model cards.
  • Unfortunately, I’m unable to convert the DAB-Deformable-DETR checkpoint, as it uses a different architecture. You can see the differences here. (c, e ,f)

Model cards:
davidhajdu/dab-detr-resnet-50
davidhajdu/dab-detr-resnet-50-dc5
davidhajdu/dab-detr-resnet-50-dc5-fixxy
davidhajdu/dab-detr-resnet-50-dc5-pat3
davidhajdu/dab-detr-resnet-50-pat3

If you would like any modifications to the model cards, please let me know!

@qubvel
Copy link
Member

qubvel commented Jan 22, 2025

run-slow: dab_detr

Copy link

This comment contains run-slow, running the specified jobs: ['models/dab_detr'] ...

@qubvel
Copy link
Member

qubvel commented Jan 22, 2025

I see these 3 tests are failing on CI:

FAILED tests/models/dab_detr/test_modeling_dab_detr.py::DabDetrModelTest::test_cpu_offload - RuntimeError: Attempted to call `variable.set_data(tensor)`, but `variable` and `tensor` have incompatible tensor type.
FAILED tests/models/dab_detr/test_modeling_dab_detr.py::DabDetrModelTest::test_disk_offload_safetensors - RuntimeError: Attempted to call `variable.set_data(tensor)`, but `variable` and `tensor` have incompatible tensor type.
FAILED tests/models/dab_detr/test_modeling_dab_detr.py::DabDetrModelTest::test_save_load_low_cpu_mem_usage - RuntimeError: Attempted to call `variable.set_data(tensor)`, but `variable` and `tensor` have incompatible tensor type.

Did they pass locally?

@qubvel
Copy link
Member

qubvel commented Jan 22, 2025

Also it seems like images are not loading in model cards (maybe my issue only). Feel free to open a upload them into dataset with "Add file" button and ping me to merge.

@conditionedstimulus
Copy link
Contributor Author

test_cpu_offload

Hey, yes I don't have these kind of errors locally. I have one issue but that related to the model weight sources. I already left the new 'IDEA-Research' one inside, but that throws a different kind of error.

src_fail


But if I fix the source then it runs without errors:
src_succ

@conditionedstimulus
Copy link
Contributor Author

Hi @qubvel,

I opened the PR on huggingface datasets.
When it will be ready I'll modify the image sources!

Thank you!

@qubvel
Copy link
Member

qubvel commented Jan 23, 2025

Thanks, merged!

Yeah, integration tests will be fixed as soon as we transfer checkpoints. test_cpu_offload is more interesting, are you running with GPU available?

@conditionedstimulus
Copy link
Contributor Author

Thanks, merged!

Yeah, integration tests will be fixed as soon as we transfer checkpoints. test_cpu_offload is more interesting, are you running with GPU available?

Thanks! No i run actually on a really weak cpu only with 8gb ram (i5 dual core)

@qubvel
Copy link
Member

qubvel commented Jan 23, 2025

I think that's the reason, the test is just skipped for cpu-only env. Try to run with pytest -v to see.

@conditionedstimulus
Copy link
Contributor Author

Hi @qubvel ,

This test fails: FAILED tests/models/qwen2_5_vl/test_modeling_qwen2_5_vl.py::Qwen2_5_VLModelTest::test_generate_from_inputs_embeds_1_beam_search
but it's not related to DAB-DETR.
I found the error and fixed it and I also pulled the main branch into the feature branch.
I also updated the model cards.

Thanks!

Comment on lines 895 to 902
module.class_embed.bias.data = (
torch.ones(
self.config.num_labels,
device=module.class_embed.bias.data.device,
dtype=module.class_embed.bias.data.dtype,
)
* bias_value
)
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we can use module.class_embed.bias.data.fill_(bias_value) or smth similar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, better solution. I've just updated the code.

@qubvel
Copy link
Member

qubvel commented Jan 23, 2025

run-slow: dab_detr

Copy link

This comment contains run-slow, running the specified jobs: ['models/dab_detr'] ...

@conditionedstimulus
Copy link
Contributor Author

run-slow: dab_detr

@conditionedstimulus
Copy link
Contributor Author

conditionedstimulus commented Jan 23, 2025

@qubvel I can see my comment doesn't have the superpower to trigger the bot to run the tests from a comment. :)

@qubvel
Copy link
Member

qubvel commented Jan 23, 2025

Yeah, it's available for maintainers only 😄

@qubvel
Copy link
Member

qubvel commented Jan 23, 2025

run-slow: dab_detr

Copy link

This comment contains run-slow, running the specified jobs: ['models/dab_detr'] ...

Copy link
Member

@qubvel qubvel 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 fixing tests! While I asking the team to move checkpoints to the org, can you please update the last thing (I hope 😄)

Comment on lines 832 to 835
self.assertEqual(len(results["scores"]), 5)
self.assertTrue(torch.allclose(results["scores"], expected_scores, atol=1e-4))
self.assertSequenceEqual(results["labels"].tolist(), expected_labels)
self.assertTrue(torch.allclose(results["boxes"][0, :], expected_boxes, atol=1e-4))
Copy link
Member

Choose a reason for hiding this comment

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

I hope the last thing! Can you please update to use torch.testing.assert_close instead of self.assertTrue(torch.allclose(...

Copy link
Member

Choose a reason for hiding this comment

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

Here and in other places in tests, for example:

https://github.com/huggingface/transformers/pull/35903/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, no problem I changed and ran the test w my model source. Is it enough to change only in that part of the tests or it should be in the whole file? Also apprx. how much time it's gonna take to move the model cards?:)
thanks

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.

5 participants