[ONNX] Fix `argmin` and `argmax` test cases #79503

qqaatw · 2022-06-14T04:47:38Z

The keepdim argument is theoretically ignored when dim is not specified (See docs).

Unfortunately the PyTorch implementation seems to still take it into account, resulting in a non-fully-reduced tensor, which is an undefined behavior. Thus, I add dim argument to the tests to make the outputs between PyTorch and ONNX runtime consistent.

facebook-github-bot · 2022-06-14T04:47:44Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/79503
❔ Python docs are pending
❔ C++ docs are pending
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (26 Pending)

As of commit 263ae6b (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

justinchuby

LGTM

titaiwangms · 2022-06-21T18:17:47Z

raise an issue for the doc in #79953.

titaiwangms

💯

BowenBao · 2022-06-21T18:27:32Z

@pytorchbot merge -g

pytorchmergebot · 2022-06-21T18:28:48Z

@pytorchbot successfully started a merge job. Check the current status here

pytorchmergebot · 2022-06-21T18:28:49Z

Merge failed due to This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again.
Raised by https://github.com/pytorch/pytorch/actions/runs/2537466773

lazycal · 2022-06-21T20:20:43Z

raise an issue for the doc in #79953.

@AllenTiTaiWang FYI the doc issue has been explained in #78791

justinchuby

Thanks for the link to the doc. So the behavior is defined and expected. @qqaatw could you retain the lines and add your changes as additional tests instead?

qqaatw · 2022-06-22T05:48:13Z

Thanks for the link to the doc. So the behavior is defined and expected. @qqaatw could you retain the lines and add your changes as additional tests instead?

The ArgMin and ArgMax don't allow axis=None AFAIK, so in this case we have to flatten the input beforehand then feed it, which underlyingly loses dimension info. After all, retaining the lines would definitely make the proposed check #79263 fail (unless we intentionally resize the output to make it consistent with PyTorch).

justinchuby · 2022-06-23T04:17:14Z

@BowenBao thoughts?

titaiwangms · 2022-06-23T04:55:00Z

Personally, I feel like depending on what is the optimization goal of onnx opset, if the goal is to align the functionality as much as we can with torch, then maybe Argmin/Argmax should be modified to support this behavior instead and then add the test case, as I don't think changing the code of input/output format in test case would be wise (it definitely affects a lot of other cases.)

qqaatw · 2022-06-23T05:49:47Z

I actually meant by resizing the output in the symbolic functions, not in the test cases. I agree that modifying the functionality of ArgMin and ArgMax on ONNX side would be the best choice.

qqaatw · 2022-06-23T05:51:14Z

@pytorchbot rebase

pytorchmergebot · 2022-06-23T05:52:39Z

@pytorchbot successfully started a rebase job. Check the current status here

pytorchmergebot · 2022-06-23T05:52:44Z

Successfully rebased fix_argmin_argmax_test_onnx onto refs/remotes/origin/master, please pull locally before adding more changes (for example, via git checkout fix_argmin_argmax_test_onnx && git pull --rebase)

justinchuby · 2022-06-28T22:00:17Z

ONNX definitions are set for any particular opsets. IMO If resizing is needed to match the behavior with PyTorch then we should do that

qqaatw · 2022-07-01T10:40:36Z

All should be good now. (If there is any coding style issue, feel free to push on this branch)

Reset based on new changes

justinchuby

Thanks for the change! Some minor comments

torch/onnx/symbolic_helper.py

…_test_onnx

justinchuby

LGTM🚀

justinchuby · 2022-08-05T18:08:28Z

@pytorchbot merge

pytorchmergebot · 2022-08-05T18:09:43Z

@pytorchbot successfully started a merge job. Check the current status here

github-actions · 2022-08-05T18:34:24Z

Hey @qqaatw.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Part of #79263 The `keepdim` argument is theoretically ignored when `dim` is not specified (See [docs](https://pytorch.org/docs/stable/generated/torch.argmin.html)). Unfortunately the PyTorch implementation seems to still take it into account, resulting in a non-fully-reduced tensor, which is an undefined behavior. Thus, I add `dim` argument to the tests to make the outputs between PyTorch and ONNX runtime consistent. Pull Request resolved: #79503 Approved by: https://github.com/justinchuby, https://github.com/AllenTiTaiWang, https://github.com/BowenBao Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/6bdf89b0c71a827be2a0969afa33f151e87a32f3 Reviewed By: kit1980 Differential Revision: D38478785 fbshipit-source-id: cd9493d6d088c4e32eb11bd19924efc0b6e6335c

Currently we don't have a dtype check in verifying the consistency between PyTorch and ONNX outputs. As a result, some of dtype inconsistencies were found and reported: #77842 #77845 This is a POC. Failed workflows: - [linux-xenial-py3.7-clang7-onnx / test (default, 2, 2, linux.2xlarge)] - inconsistent shape - TestONNXRuntime_opset10.test_all (#79371) - TestONNXRuntime_opset10.test_any (#79371) - TestONNXRuntime_opset10.test_argmin_argmax (#79503) - TestONNXRuntime_opset10.test_hardshrink (#79695) - TestONNXRuntime_opset10.test_linalg_norm (#79506) - TestONNXRuntime_opset10.test_linalg_vector_norm (#79506) - TestONNXRuntime_opset10.test_prelu_scalar (#79846) - TestONNXRuntime_opset10.test_softshrink (#79695) - TestONNXRuntime_opset10.test_sum_empty_tensor (skipped) - TestONNXRuntime_opset10.test_tolist (skipped) - inconsistent dtype - test_arithmetic_prim_bool (skipped) - test_arithmeticOps_with_low_precision (skipped) - test_arithmetic_prim_float (skipped) - test_logical_and (#79339) - test_logical_or (#79339) - test_logical_xor (#79339) - test_pow (skipped) - test_primitive_input_floating (skipped) - test_quantize_per_tensor (#79690) - test_quantized_adaptive_avg_pool2d (#79690) - test_quantized_arithmetic (#79690) - test_quantized_arithmetic_qfunctional (#79690) - test_quantized_conv2d (#79690) - test_quantized_conv2d_relu (#79690) - test_quantized_flatten (#79690) - test_quantized_hardsigmoid (#79690) - test_quantized_hardswish (#79690) - test_quantized_linear (#79690) - test_quantized_sigmoid (#79690) - test_item (skipped) - test_full_like_value (skipped) - TestONNXRuntime_opset7.test_div_rounding_mode (skipped) - TestONNXRuntime_opset8.test_div_rounding_mode (skipped) - TestONNXRuntime_opset9.test_div_rounding_mode (skipped) - TestONNXRuntime_opset9_IRv4.test_div_rounding_mode (skipped) - test_outer (skipped) - test_symbolic_shape_inference_arange_2 (skipped) Pull Request resolved: #79263 Approved by: https://github.com/justinchuby, https://github.com/BowenBao

Summary: Currently we don't have a dtype check in verifying the consistency between PyTorch and ONNX outputs. As a result, some of dtype inconsistencies were found and reported: #77842 #77845 This is a POC. Failed workflows: - [linux-xenial-py3.7-clang7-onnx / test (default, 2, 2, linux.2xlarge)] - inconsistent shape - TestONNXRuntime_opset10.test_all (#79371) - TestONNXRuntime_opset10.test_any (#79371) - TestONNXRuntime_opset10.test_argmin_argmax (#79503) - TestONNXRuntime_opset10.test_hardshrink (#79695) - TestONNXRuntime_opset10.test_linalg_norm (#79506) - TestONNXRuntime_opset10.test_linalg_vector_norm (#79506) - TestONNXRuntime_opset10.test_prelu_scalar (#79846) - TestONNXRuntime_opset10.test_softshrink (#79695) - TestONNXRuntime_opset10.test_sum_empty_tensor (skipped) - TestONNXRuntime_opset10.test_tolist (skipped) - inconsistent dtype - test_arithmetic_prim_bool (skipped) - test_arithmeticOps_with_low_precision (skipped) - test_arithmetic_prim_float (skipped) - test_logical_and (#79339) - test_logical_or (#79339) - test_logical_xor (#79339) - test_pow (skipped) - test_primitive_input_floating (skipped) - test_quantize_per_tensor (#79690) - test_quantized_adaptive_avg_pool2d (#79690) - test_quantized_arithmetic (#79690) - test_quantized_arithmetic_qfunctional (#79690) - test_quantized_conv2d (#79690) - test_quantized_conv2d_relu (#79690) - test_quantized_flatten (#79690) - test_quantized_hardsigmoid (#79690) - test_quantized_hardswish (#79690) - test_quantized_linear (#79690) - test_quantized_sigmoid (#79690) - test_item (skipped) - test_full_like_value (skipped) - TestONNXRuntime_opset7.test_div_rounding_mode (skipped) - TestONNXRuntime_opset8.test_div_rounding_mode (skipped) - TestONNXRuntime_opset9.test_div_rounding_mode (skipped) - TestONNXRuntime_opset9_IRv4.test_div_rounding_mode (skipped) - test_outer (skipped) - test_symbolic_shape_inference_arange_2 (skipped) Pull Request resolved: #79263 Approved by: https://github.com/justinchuby, https://github.com/BowenBao Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/d9a7e93aaf3166e639ea413123bd6c38b9144adc Reviewed By: seemethere Differential Revision: D38585848 fbshipit-source-id: 9da98581ceec51142ae31d3f8a06f9f296a16b23

qqaatw requested a review from BowenBao as a code owner June 14, 2022 04:47

facebook-github-bot added the cla signed label Jun 14, 2022

qqaatw mentioned this pull request Jun 14, 2022

[ONNX] Add dtype check in onnx verification #79263

Closed

pytorchbot added the open source label Jun 14, 2022

mikaylagawarecki added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jun 14, 2022

justinchuby added the module: onnx Related to torch.onnx label Jun 15, 2022

justinchuby approved these changes Jun 16, 2022

View reviewed changes

justinchuby assigned justinchuby and BowenBao Jun 16, 2022

titaiwangms self-assigned this Jun 21, 2022

titaiwangms approved these changes Jun 21, 2022

View reviewed changes

BowenBao previously approved these changes Jun 21, 2022

View reviewed changes

justinchuby requested changes Jun 21, 2022

View reviewed changes

justinchuby requested a review from BowenBao June 21, 2022 21:33

Fix

ccfbf58

pytorchmergebot force-pushed the fix_argmin_argmax_test_onnx branch from 6ed015d to ccfbf58 Compare June 23, 2022 05:52

resize output

b55217b

qqaatw requested a review from shubhambhokare1 as a code owner July 1, 2022 10:38

fix

d45e668

justinchuby reviewed Jul 5, 2022

View reviewed changes

torch/onnx/symbolic_helper.py Show resolved Hide resolved

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

torch/onnx/symbolic_helper.py Outdated Show resolved Hide resolved

qqaatw added 3 commits August 5, 2022 12:28

apply

9acae46

Merge remote-tracking branch 'upstream/master' into fix_argmin_argmax…

0e3278a

…_test_onnx

exclude opset version < 9

263ae6b

justinchuby approved these changes Aug 5, 2022

View reviewed changes

justinchuby requested review from BowenBao and removed request for BowenBao August 5, 2022 15:02

BowenBao approved these changes Aug 5, 2022

View reviewed changes

pytorchmergebot added the Merged label Aug 5, 2022

pytorchmergebot closed this in 6bdf89b Aug 5, 2022

titaiwangms added release notes: onnx torch.onnx related changes that should show up in the release notes topic: bug fixes topic category labels Aug 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ONNX] Fix `argmin` and `argmax` test cases #79503

[ONNX] Fix `argmin` and `argmax` test cases #79503

qqaatw commented Jun 14, 2022

facebook-github-bot commented Jun 14, 2022 •

edited

justinchuby left a comment

titaiwangms commented Jun 21, 2022

titaiwangms left a comment

BowenBao commented Jun 21, 2022

pytorchmergebot commented Jun 21, 2022

pytorchmergebot commented Jun 21, 2022

lazycal commented Jun 21, 2022

justinchuby left a comment

qqaatw commented Jun 22, 2022 •

edited

justinchuby commented Jun 23, 2022

titaiwangms commented Jun 23, 2022 •

edited by justinchuby

qqaatw commented Jun 23, 2022

qqaatw commented Jun 23, 2022

pytorchmergebot commented Jun 23, 2022

pytorchmergebot commented Jun 23, 2022

justinchuby commented Jun 28, 2022

qqaatw commented Jul 1, 2022 •

edited

justinchuby left a comment •

edited

justinchuby left a comment

justinchuby commented Aug 5, 2022

pytorchmergebot commented Aug 5, 2022

github-actions bot commented Aug 5, 2022

[ONNX] Fix argmin and argmax test cases #79503

[ONNX] Fix argmin and argmax test cases #79503

Conversation

qqaatw commented Jun 14, 2022

facebook-github-bot commented Jun 14, 2022 • edited

🔗 Helpful links

✅ No Failures (26 Pending)

justinchuby left a comment

Choose a reason for hiding this comment

titaiwangms commented Jun 21, 2022

titaiwangms left a comment

Choose a reason for hiding this comment

BowenBao commented Jun 21, 2022

pytorchmergebot commented Jun 21, 2022

pytorchmergebot commented Jun 21, 2022

lazycal commented Jun 21, 2022

justinchuby left a comment

Choose a reason for hiding this comment

qqaatw commented Jun 22, 2022 • edited

justinchuby commented Jun 23, 2022

titaiwangms commented Jun 23, 2022 • edited by justinchuby

qqaatw commented Jun 23, 2022

qqaatw commented Jun 23, 2022

pytorchmergebot commented Jun 23, 2022

pytorchmergebot commented Jun 23, 2022

justinchuby commented Jun 28, 2022

qqaatw commented Jul 1, 2022 • edited

justinchuby left a comment • edited

Choose a reason for hiding this comment

justinchuby left a comment

Choose a reason for hiding this comment

justinchuby commented Aug 5, 2022

pytorchmergebot commented Aug 5, 2022

github-actions bot commented Aug 5, 2022

[ONNX] Fix `argmin` and `argmax` test cases #79503

[ONNX] Fix `argmin` and `argmax` test cases #79503

facebook-github-bot commented Jun 14, 2022 •

edited

qqaatw commented Jun 22, 2022 •

edited

titaiwangms commented Jun 23, 2022 •

edited by justinchuby

qqaatw commented Jul 1, 2022 •

edited

justinchuby left a comment •

edited