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

[RT-DETR] Fix onnx inference bug for Optype (Where) #33877

Merged

Conversation

YHallouard
Copy link
Contributor

@YHallouard YHallouard commented Oct 1, 2024

What does this PR do?

Fixes

Before submitting

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.

@HuggingFaceDocBuilderDev

Hey! 🤗 Thanks for your contribution to the transformers library!

Before merging this pull request, slow tests CI should be triggered. To enable this:

  • Add the run-slow label to the PR
  • When your PR is ready for merge and all reviewers' comments have been addressed, push an empty commit with the command [run-slow] followed by a comma separated list of all the models to be tested, i.e. [run_slow] model_to_test_1, model_to_test_2
    • If the pull request affects a lot of models, put at most 10 models in the commit message
  • A transformers maintainer will then approve the workflow to start the tests

(For maintainers) The documentation for slow tests CI on PRs is here.

@YHallouard YHallouard force-pushed the feat/add_config_for_RT_DETR_onnx branch from 01e5d38 to b146090 Compare October 1, 2024 23:33
@YHallouard YHallouard force-pushed the feat/add_config_for_RT_DETR_onnx branch from b146090 to 70e48fa Compare October 1, 2024 23:36
@YHallouard
Copy link
Contributor Author

Hi @SangbumChoi, how are you ? I saw that you were the main maintainer of RT-DETR. Thank you very much for your work !

I propose a fix for the anchor generation to avoid a bug in onnx inference : Error: Type parameter (T) of Optype (Where) bound to different types (tensor(float) and tensor(double) in node (/model/decoder/Where). It has already been discussed here: lyuwenyu/RT-DETR#307

What do you feel about it ?

Copy link
Contributor

@SangbumChoi SangbumChoi 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 contribution! For me it looks great,

  1. I think it would be helpful to add some ONNX conversion description script in rt_detr.md?

Requesting @qubvel for the final approvement!

@@ -1736,7 +1736,7 @@ def generate_anchors(self, spatial_shapes=None, grid_size=0.05, device="cpu", dt
anchors = torch.concat(anchors, 1)
valid_mask = ((anchors > eps) * (anchors < 1 - eps)).all(-1, keepdim=True)
anchors = torch.log(anchors / (1 - anchors))
anchors = torch.where(valid_mask, anchors, torch.inf)
anchors = torch.where(valid_mask, anchors, torch.tensor(float("inf"), dtype=torch.float32, device=device))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
anchors = torch.where(valid_mask, anchors, torch.tensor(float("inf"), dtype=torch.float32, device=device))
anchors = torch.where(valid_mask, anchors, torch.tensor(float("inf"), dtype=dtype, device=device))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some doc f157406 :) Tell me what do you think

@YHallouard
Copy link
Contributor Author

YHallouard commented Oct 2, 2024

Thanks for the contribution! For me it looks great,

  1. I think it would be helpful to add some ONNX conversion description script in rt_detr.md?

Requesting @qubvel for the final approvement!

Thanks for your reply ! Yes it would be interesting, what do you think about adding a section :


ONNX Tips

See RTDetrConfig in this section. (put the link to that section)

from transformers.models.rt_detr.import RTDetrConfig, RTDetrOnnxConfig  # type: ignore[import-untyped]
from transformers.onnx.convert import export_pytorch  # type: ignore[import-untyped]

rtdetr_onnx_config = RTDetrOnnxConfig(config=RTDetrConfig(), task="object-detection")

export_pytorch(
    preprocessor=preprocessor,
    model=model,
    config=rtdetr_onnx_config,
    opset=17,
    output=output_path,
    tokenizer=None,
    device="cuda",
)

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.

Hi @YHallouard, thanks a lot for your contribution! Overall looks good to me!

The only concern I have is that onnx export in transformers is no longer maintained and at some moment we might end up removing it.
It will be better to add this config to Optimum, leaving here just a snippet of code on how to export the model using Optimum.

cc @ArthurZucker regarding onnx

@@ -1736,7 +1736,7 @@ def generate_anchors(self, spatial_shapes=None, grid_size=0.05, device="cpu", dt
anchors = torch.concat(anchors, 1)
valid_mask = ((anchors > eps) * (anchors < 1 - eps)).all(-1, keepdim=True)
anchors = torch.log(anchors / (1 - anchors))
anchors = torch.where(valid_mask, anchors, torch.inf)
anchors = torch.where(valid_mask, anchors, torch.tensor(float("inf"), dtype=dtype, device=device))
Copy link
Member

Choose a reason for hiding this comment

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

cc @yonigozlan re dtype/device for torch.compile

@YHallouard
Copy link
Contributor Author

YHallouard commented Oct 3, 2024

t will be better to add this config to Optimum,

Hi @qubvel, honestly I wasn't aware of this repository ! Very interesting discovery !
I see how to put it in optimum, but since there is no ORTModelForObjectDetection yet in Optimum (thing i can work on maybe in the future). What do you think about putting this config (RTDetrOnnxConfig) in transformers and in optimum ?

But I can rework the rt_detr.mdOnnx section, specifying in transformer section that is deprecated and also put a optimum section

@ArthurZucker
Copy link
Collaborator

Hey @YHallouard sorry for the confusion, I think we need better doc on this 😢 let's not add anything that we know is already deprecated! If you need guidance on ONNX and Optimum contribution for this, I am sure @michaelbenayoun will be happy to help!

@YHallouard YHallouard force-pushed the feat/add_config_for_RT_DETR_onnx branch from 2b90511 to e38ba18 Compare October 6, 2024 11:48
@YHallouard YHallouard changed the title [RT-DETR] Add onnx config and fix onnx inference bug for Optype (Where) [RT-DETR] Fix onnx inference bug for Optype (Where) Oct 6, 2024
@YHallouard
Copy link
Contributor Author

Hi @ArthurZucker, @michaelbenayoun,

No problem, I removed th Onnx config and openned a pull request in optimum. huggingface/optimum#2040

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks! 🤗

@@ -1736,7 +1736,7 @@ def generate_anchors(self, spatial_shapes=None, grid_size=0.05, device="cpu", dt
anchors = torch.concat(anchors, 1)
valid_mask = ((anchors > eps) * (anchors < 1 - eps)).all(-1, keepdim=True)
anchors = torch.log(anchors / (1 - anchors))
anchors = torch.where(valid_mask, anchors, torch.inf)
anchors = torch.where(valid_mask, anchors, torch.tensor(float("inf"), dtype=dtype, device=device))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
anchors = torch.where(valid_mask, anchors, torch.tensor(float("inf"), dtype=dtype, device=device))
anchors = torch.where(valid_mask, anchors, torch.finfo(dtype).min, dtype=dtype, device=device))

if the dtype is the dtype of valid_mask then this would make more sense ! Otherwise the min (float32 -inf) is not gonna be the same!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dtype is the dtype of anchors, valid_mask is a condition, dtypes are the same but just by construction.

But you're right, torch.finfo(dtype).max is good :)

Copy link
Contributor Author

@YHallouard YHallouard Oct 7, 2024

Choose a reason for hiding this comment

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

@YHallouard YHallouard force-pushed the feat/add_config_for_RT_DETR_onnx branch from 6b0f18a to 1aa73fe Compare October 7, 2024 13:41
@YHallouard
Copy link
Contributor Author

YHallouard commented Oct 10, 2024

Hi @ArthurZucker, should I run a run-slow ?

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

LGTM now thanks for updating!

@ArthurZucker ArthurZucker merged commit eb6a734 into huggingface:main Oct 22, 2024
16 checks passed
@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.

BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* feat: [RT-DETR] Add onnx runtime config and fix onnx inference bug Optype (Where)

* fix lint

* use dtype istead of torch.float32

* add doc

* remove onnx config

* use dtype info

* use tensor to fix lint
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
* feat: [RT-DETR] Add onnx runtime config and fix onnx inference bug Optype (Where)

* fix lint

* use dtype istead of torch.float32

* add doc

* remove onnx config

* use dtype info

* use tensor to fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants