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

GridSample ND #5010

Merged
merged 23 commits into from
May 11, 2023
Merged

GridSample ND #5010

merged 23 commits into from
May 11, 2023

Conversation

leimao
Copy link
Contributor

@leimao leimao commented Mar 17, 2023

Description

Extending the existing GridSample (2D) operator to any arbitrary dimensions.

This PR is ready for review.

Motivation and Context

PyTorch GridSample supports both 2D and 3D interpolation whereas ONNX does not.

#4779

TODOs

  1. The PyTorch GridSample PR has been merged. Update the ONNX unit test reference values using the PyTorch TOT reference outputs.
  2. Move the GridSample from Opset 19 to Opset 20.
  3. Create Adapters for GridSample version upgrades.

@gramalingam gramalingam added the operator Issues related to ONNX operators label Apr 5, 2023
@leimao leimao force-pushed the grid_sample_nd branch 2 times, most recently from dc5b20a to 482bf16 Compare April 9, 2023 17:07
leimao added 9 commits May 3, 2023 02:50
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
@leimao
Copy link
Contributor Author

leimao commented May 3, 2023

@xadupre I realized that the TOT ONNX CI has added ONNX Runtime unit tests. I think this is inappropriate. Why ONNX which is something that ONNX Runtime depends on needs to be tested against ONNX Runtime?
In addition, because I extended the GridSample operator spec, ONNX Runtime does not support this operator anymore. How could I disable the ONNX Runtime tests for GridSample? Thank you.

@xadupre
Copy link
Contributor

xadupre commented May 3, 2023

You can add a section here https://github.com/onnx/onnx/blob/main/onnx/test/test_backend_onnxruntime.py#L231 for opset 20 and disable the tests you need.

leimao added 2 commits May 3, 2023 14:54
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
leimao added 3 commits May 4, 2023 15:40
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
@leimao
Copy link
Contributor Author

leimao commented May 5, 2023

@gramalingam @xadupre Some of the CI tests are taking forever. But if you check the details, they were actually successful. Do you know why?

@gramalingam
Copy link
Contributor

@gramalingam @xadupre Some of the CI tests are taking forever. But if you check the details, they were actually successful. Do you know why?

Hmmm ... I don't know. @jcwchen , do you know? Thanks!

@jcwchen
Copy link
Member

jcwchen commented May 5, 2023

Hmmm ... I don't know. @jcwchen , do you know? Thanks!

I haven't bumped into such kind of situation before. I guess AzurePipelines was downgraded at that moment. Rerun should help. Please let me know if this problem persists. Thanks!

@leimao
Copy link
Contributor Author

leimao commented May 5, 2023

Hmmm ... I don't know. @jcwchen , do you know? Thanks!

I haven't bumped into such kind of situation before. I guess AzurePipelines was downgraded at that moment. Rerun should help. Please let me know if this problem persists. Thanks!

Thanks. I will push another commit after I address the reviewers' comments.

Signed-off-by: Lei Mao <dukeleimao@gmail.com>
@yang-Michael
Copy link

@leimao hello, I really admire the countless contributions you've made to the pytorch and onnx code, including the gridsample operator. So, I'm dying to know when will the 5D input supported version may be released?

@leimao
Copy link
Contributor Author

leimao commented May 6, 2023

@leimao hello, I really admire the countless contributions you've made to the pytorch and onnx code, including the gridsample operator. So, I'm dying to know when will the 5D input supported version may be released?

This PR is being reviewed now. Hopefully it can be merged to main soon. But the official Opset 20 release, which includes the GridSample ND, will be in roughly 3 months from today I think. Once this PR is merged to main, the backend runtimes, such as ONNX Runtime and TensorRT, can start to work on supporting GridSample ND.

onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
@gramalingam gramalingam enabled auto-merge (squash) May 11, 2023 16:40
@gramalingam gramalingam merged commit 906884a into onnx:main May 11, 2023
37 checks passed
q-ycong-p pushed a commit to q-ycong-p/onnx that referenced this pull request May 18, 2023
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Co-authored-by: G. Ramalingam <grama@microsoft.com>
Signed-off-by: Yu Cong <congyc@amazon.com>
adityagoel4512 pushed a commit to adityagoel4512/onnx that referenced this pull request Jul 28, 2023
Signed-off-by: Lei Mao <dukeleimao@gmail.com>
Co-authored-by: G. Ramalingam <grama@microsoft.com>
Signed-off-by: Aditya Goel <agoel4512@gmail.com>
@liqunfu
Copy link
Contributor

liqunfu commented Sep 18, 2023

@leimao , thank you for contributing to ONNX. Now I am preparing ONNX 1.15.0 release and integrating the release with ORT, I wonder if you have a plan to implement the runtime kernel for this updated op in ORT? If so, I will let you know once ORT has ONNX 1.15.0 integrated. Thank you in advance.

@leimao
Copy link
Contributor Author

leimao commented Sep 21, 2023

@leimao , thank you for contributing to ONNX. Now I am preparing ONNX 1.15.0 release and integrating the release with ORT, I wonder if you have a plan to implement the runtime kernel for this updated op in ORT? If so, I will let you know once ORT has ONNX 1.15.0 integrated. Thank you in advance.

@liqunfu Thanks for bringing this up. For the moment, I probably don't have the bandwidth to commit a runtime kernel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator Issues related to ONNX operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants