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 support for GridSample operator #2909

Merged
merged 55 commits into from
Jun 19, 2024
Merged

Add support for GridSample operator #2909

merged 55 commits into from
Jun 19, 2024

Conversation

gyulaz-htec
Copy link
Collaborator

@gyulaz-htec gyulaz-htec commented Mar 20, 2024

Add support for GridSample onnx operator.
Currently the following feature set is supported:

@gyulaz-htec gyulaz-htec marked this pull request as draft March 20, 2024 09:35
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.90%. Comparing base (bceef13) to head (a1cf519).

Current head a1cf519 differs from pull request most recent head 8bb3d24

Please upload reports for the commit 8bb3d24 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2909      +/-   ##
===========================================
+ Coverage    91.82%   91.90%   +0.08%     
===========================================
  Files          486      487       +1     
  Lines        18991    19192     +201     
===========================================
+ Hits         17438    17639     +201     
  Misses        1553     1553              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Mar 20, 2024

Test Batch Rate new
b6172d
Rate old
5fcf86
Diff Compare
torchvision-resnet50 64 1,751.55 1,751.74 -0.01%
torchvision-resnet50_fp16 64 4,084.47 4,084.34 0.00%
torchvision-densenet121 32 1,467.78 1,467.39 0.03%
torchvision-densenet121_fp16 32 2,529.08 2,525.45 0.14%
torchvision-inceptionv3 32 889.72 889.64 0.01%
torchvision-inceptionv3_fp16 32 1,483.73 1,483.57 0.01%
cadene-inceptionv4 16 412.38 412.40 -0.01%
cadene-resnext64x4 16 419.71 419.50 0.05%
slim-mobilenet 64 4,007.09 4,006.71 0.01%
slim-nasnetalarge 64 101.02 101.01 0.01%
slim-resnet50v2 64 1,680.79 1,680.60 0.01%
bert-mrpc-onnx 8 615.83 618.22 -0.39%
bert-mrpc-tf 1 277.98 279.81 -0.65%
pytorch-examples-wlang-gru 1 320.54 319.57 0.30%
pytorch-examples-wlang-lstm 1 288.76 289.36 -0.21%
torchvision-resnet50_1 1 471.26 471.89 -0.13%
cadene-dpn92_1 1 247.18 247.09 0.04%
cadene-resnext101_1 1 203.92 204.23 -0.15%
onnx-taau-downsample 1 206.48 206.24 0.11%
dlrm-criteoterabyte 1 22.91 22.90 0.04%
dlrm-criteoterabyte_fp16 1 42.72 42.73 -0.02%
agentmodel 1 6,369.83 6,323.67 0.73%
unet_fp16 2 34.18 34.21 -0.07%
resnet50v1_fp16 1 574.81 589.36 -2.47%
resnet50v1_int8 1 578.26 573.50 0.83%
bert_base_cased_fp16 64 646.28 646.33 -0.01%
bert_large_uncased_fp16 32 199.02 198.99 0.02%
bert_large_fp16 1 117.21 117.54 -0.27%
distilgpt2_fp16 16 1,212.43 1,211.40 0.09%
yolov5s 1 301.12 301.25 -0.04%
tinyllama 1 23.33 23.34 -0.03%
vicuna-fastchat 1 133.85 133.65 0.15%
whisper-tiny-encoder 1 244.42 244.39 0.01%
whisper-tiny-decoder 1 256.29 256.66 -0.14%

This build is OK for merge ✅

@migraphx-bot
Copy link
Collaborator

migraphx-bot commented Mar 20, 2024


     ✅ bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

     ✅ bert-mrpc-tf: PASSED: MIGraphX meets tolerance

     ✅ pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

     ✅ pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

     ✅ torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

     ✅ cadene-dpn92_1: PASSED: MIGraphX meets tolerance

     ✅ cadene-resnext101_1: PASSED: MIGraphX meets tolerance

     ✅ dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

     ✅ agentmodel: PASSED: MIGraphX meets tolerance

     ✅ unet: PASSED: MIGraphX meets tolerance

     ✅ resnet50v1: PASSED: MIGraphX meets tolerance

     ✅ bert_base_cased_fp16: PASSED: MIGraphX meets tolerance

🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output


     ✅ bert_large: PASSED: MIGraphX meets tolerance

     ✅ yolov5s: PASSED: MIGraphX meets tolerance

     ✅ tinyllama: PASSED: MIGraphX meets tolerance

     ✅ vicuna-fastchat: PASSED: MIGraphX meets tolerance

     ✅ whisper-tiny-encoder: PASSED: MIGraphX meets tolerance

     ✅ whisper-tiny-decoder: PASSED: MIGraphX meets tolerance

     ✅ distilgpt2_fp16: PASSED: MIGraphX meets tolerance

@TedThemistokleous TedThemistokleous added the Onnx Operators Adding or modifying an Onnx Operator in the MIGraphX codebase label Mar 20, 2024
@gyulaz-htec gyulaz-htec force-pushed the grid_sample branch 2 times, most recently from 6066406 to 4287616 Compare March 25, 2024 10:02
@gyulaz-htec gyulaz-htec marked this pull request as ready for review March 25, 2024 10:12
@gyulaz-htec
Copy link
Collaborator Author

gyulaz-htec commented Mar 25, 2024

@causten The PR is ready for review
This sould not be marged yet, because I've found an accuracy issue with fuse_pointwise pass with the GPU target when computing GridSample results (with linear inetrploation).
Collected my findings on that issue in #2923

@gyulaz-htec gyulaz-htec force-pushed the grid_sample branch 2 times, most recently from e9fbe18 to dda7a4a Compare March 27, 2024 09:06
@gyulaz-htec
Copy link
Collaborator Author

I had to exclude logical_and and where operators from pointwise fusion, so the GPU tests are passing now.
#2923 is fixed by that.

@gyulaz-htec gyulaz-htec changed the title Grid sample Add support for GridSample operator Mar 28, 2024
Copy link
Collaborator

@TedThemistokleous TedThemistokleous left a comment

Choose a reason for hiding this comment

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

test/py/onnx_backend_test.py Show resolved Hide resolved
src/onnx/parse_gridsample.cpp Show resolved Hide resolved
src/fuse_pointwise.cpp Outdated Show resolved Hide resolved
src/onnx/parse_gridsample.cpp Outdated Show resolved Hide resolved
src/onnx/parse_gridsample.cpp Show resolved Hide resolved
src/onnx/parse_gridsample.cpp Outdated Show resolved Hide resolved
@gyulaz-htec gyulaz-htec force-pushed the grid_sample branch 2 times, most recently from ad8ac0d to 17205b0 Compare April 11, 2024 14:00
@gyulaz-htec
Copy link
Collaborator Author

@TedThemistokleous @pfultz2 I've adressed the majority of you commets except the Look into alternative to avoid O(n^4) updates and ops. part.

@gyulaz-htec
Copy link
Collaborator Author

@pfultz2 I've removed the fuse pointwise related changes from the PR in favor of #3054

@gyulaz-htec gyulaz-htec requested a review from pfultz2 May 8, 2024 09:01
gyulaz-htec and others added 3 commits May 21, 2024 11:42
Windows build requires '#include<array>' otherwise we get a compile error when using std:array
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.05%. Comparing base (5fcf86e) to head (b6172d3).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2909      +/-   ##
===========================================
+ Coverage    91.97%   92.05%   +0.08%     
===========================================
  Files          489      490       +1     
  Lines        19398    19599     +201     
===========================================
+ Hits         17841    18042     +201     
  Misses        1557     1557              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@causten
Copy link
Collaborator

causten commented Jun 11, 2024

@gyulaz-htec you don't need hit the "update branch" button. Once the review are complete I'll handle it

@kahmed10
Copy link
Collaborator

I would like to see additional documentation and possible refactoring since the parse_gridsample.cpp is quite large. Not blocking for merge.

@causten causten merged commit 2b4d328 into develop Jun 19, 2024
45 checks passed
@causten causten deleted the grid_sample branch June 19, 2024 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Onnx Operators Adding or modifying an Onnx Operator in the MIGraphX codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accuracy issue with fuse_pointwise and GridSample
7 participants