Support per-test tolerances for ONNX tests #11775

garymm · 2022-06-07T22:43:17Z

Prior to this every test shared the same global tolerances. This meant
that if an ONNX test failed due to a small but acceptable difference in
output, the only alternative was to disable the test entirely.

In op set 17, the DFT operator is being added. Without this change, the
tests for that operator fail because the output is off by about 5e-5.
It's better to keep test coverage for this new op rather than disable
the test entirely.

Also prior to this change, the global tolerances were not shared between
JavaScript, Python and C++ tests. Now they are.

Also update the JSON submodule, which is needed to get it to build with
exceptions disabled.

Unblocks #11640.

Prior to this every test shared the same global tolerances. This meant that if an ONNX test failed due to a small but acceptable difference in output, the only alternative was to disable the test entirely. In op set 17, the DFT operator is being added. Without this change, the tests for that operator fail because the output is off by about 5e-5. It's better to keep test coverage for this new op rather than disable the test entirely. Also prior to this change, the global tolerances were not shared between JavaScript and Python tests. Now they are. Unblocks microsoft#11640.

garymm · 2022-06-09T00:11:58Z

onnxruntime/test/python/onnx_backend_test_series.py


-import onnxruntime.backend as c2
+from onnxruntime import backend


I have no idea why this is causing this check to fail.

jcwchen

Thanks for proposing per-test tolerances! The document improvement looks good to me. It would be better to get other experts' approvals about the Python/JavaScript part. Going forward with this PR, perhaps in the future we can go through the whole skip list for ONNX node tests and enable some of them with specific tolerances if reasonable.

js/node/test/test-utils.ts

fs-eire · 2022-06-09T20:50:39Z

please fix the build errors of code formatting and ESLint by running the following commands under <ORT_ROOT>/js/ :

npm ci
npm run format
npm run lint

lgtm-com · 2022-06-09T23:26:39Z

This pull request introduces 107 alerts when merging bec5a0d into 5ecfaef - view on LGTM.com

new alerts:

98 for Commented-out code
4 for Long switch case
2 for Use of goto
1 for For loop variable changed in body
1 for Block with too many statements
1 for No trivial switch statements

lgtm-com · 2022-06-10T22:46:09Z

This pull request introduces 129 alerts when merging 5d4b8f6 into 5562b47 - view on LGTM.com

new alerts:

119 for Commented-out code
4 for Long switch case
2 for Use of goto
1 for For loop variable changed in body
1 for Block with too many statements
1 for Sizeof with side effects
1 for No trivial switch statements

lgtm-com · 2022-06-11T02:10:10Z

This pull request introduces 129 alerts when merging ccafd94 into 0869f4f - view on LGTM.com

new alerts:

119 for Commented-out code
4 for Long switch case
2 for Use of goto
1 for For loop variable changed in body
1 for Block with too many statements
1 for Sizeof with side effects
1 for No trivial switch statements

lgtm-com · 2022-06-13T21:28:18Z

This pull request introduces 129 alerts when merging bc78ae7 into a93ebd2 - view on LGTM.com

new alerts:

119 for Commented-out code
4 for Long switch case
2 for Use of goto
1 for For loop variable changed in body
1 for Block with too many statements
1 for Sizeof with side effects
1 for No trivial switch statements

garymm · 2022-06-13T22:22:20Z

@fs-eire @jcwchen could you please review?

js/node/test/test-utils.ts

jcwchen

Overall LGTM. It would be great if this PR can get approvals from ORT Core team and ORT Web team respectively.

cmake/onnxruntime_unittests.cmake

yuslepukhin · 2022-06-14T17:32:28Z

onnxruntime/test/onnx/main.cc

    static const ORTCHAR_T* x86_disabled_tests[] = {ORT_TSTR("mlperf_ssd_resnet34_1200"), ORT_TSTR("mask_rcnn_keras"), ORT_TSTR("mask_rcnn"), ORT_TSTR("faster_rcnn"), ORT_TSTR("vgg19"), ORT_TSTR("coreml_VGG16_ImageNet")};
    all_disabled_tests.insert(std::begin(x86_disabled_tests), std::end(x86_disabled_tests));
 #endif

+    const TestTolerances tt = LoadTestTolerances(enable_cuda, enable_openvino);


tt

test_tolerances

Inlined it instead.

yuslepukhin · 2022-06-14T17:35:31Z

winml/test/model/model_tests.cpp

@@ -173,8 +173,6 @@ std::string GetTestDataPath() {
 static std::vector<ITestCase*> GetAllTestCases() {
  std::vector<ITestCase*> tests;
  std::vector<std::basic_string<PATH_CHAR_TYPE>> whitelistedTestCases;
-  double perSampleTolerance = 1e-3;


doubl

constexpr

Sorry I'm not sure what you're asking me to do. Could you please elaborate?

yuslepukhin · 2022-06-14T17:36:40Z

LGTM. As soon as checks pass I can sign off.

fs-eire

signed off for typescript files (/js/node/**)

lgtm-com · 2022-06-14T20:53:24Z

This pull request introduces 129 alerts when merging d498d52 into 52f6db1 - view on LGTM.com

new alerts:

119 for Commented-out code
4 for Long switch case
2 for Use of goto
1 for For loop variable changed in body
1 for Block with too many statements
1 for Sizeof with side effects
1 for No trivial switch statements

garymm · 2022-06-14T22:10:48Z

@yuslepukhin all checks passed so I'll merge now. If there are any changes you'd like to see, feel free to comment and I'll open another PR.

This is required on CentOS 7 if using distro-provided json-devel 3.6.1. Regression introduced in: - microsoft#11775 Related upstream commit: - nlohmann/json@74520d8 Fixed microsoft#12393

- microsoft/onnxruntime#12394 - microsoft/onnxruntime#12393 - microsoft/onnxruntime#11775 Upstream change: - nlohmann/json@74520d8

This is required on CentOS 7 if using distro-provided json-devel 3.6.1. Regression introduced in: - microsoft#11775 Related upstream commit: - nlohmann/json@74520d8 Fixed microsoft#12393

This is required on CentOS 7 if using distro-provided json-devel 3.6.1. Regression introduced in: - #11775 Related upstream commit: - nlohmann/json@74520d8 Fixed #12393

This is required on CentOS 7 if using distro-provided json-devel 3.6.1. Regression introduced in: - microsoft#11775 Related upstream commit: - nlohmann/json@74520d8 Fixed microsoft#12393

Prior to this every test shared the same tolerances. This meant that if an ONNX test failed due to a small but acceptable difference in output, the only alternative was to disable the test entirely. In op set 17, the DFT operator is being added. Without this change, the tests for that operator fail because the output is off by about 5e-5. It's better to keep test coverage for this new op rather than disable the test entirely. Also prior to this change, the global tolerances were not shared between C++, JavaScript, and Python tests. Now they are. Also fix various minor issues raised by linters. Unblocks microsoft#11640.

garymm mentioned this pull request Jun 7, 2022

ONNX release 1.12.0 #11640

Closed

garymm added 7 commits June 8, 2022 20:59

fix eslint

674f4c1

fix python lint and namedtuple support

8b63cee

more python lint

3059817

fix python

882790c

python lint

0378894

declare constants

36cbbfc

add docstring

7b9911c

garymm commented Jun 9, 2022

View reviewed changes

garymm marked this pull request as ready for review June 9, 2022 00:15

garymm requested review from fs-eire, jcwchen and snnn June 9, 2022 00:15

jcwchen reviewed Jun 9, 2022

View reviewed changes

js/node/test/test-utils.ts Show resolved Hide resolved

garymm mentioned this pull request Jun 9, 2022

Add title to "How to update ONNX" page #11692

Closed

Add C++ onnx_test_runner

bec5a0d

garymm added 4 commits June 10, 2022 19:16

typescript format

8ca783a

update json and disable exceptions in it

a2d0800

try to support more platforms

20165e2

lint

5d4b8f6

hopefully fix windows build

ccafd94

update winml GetAllTestCases

bc78ae7

fs-eire reviewed Jun 14, 2022

View reviewed changes

js/node/test/test-utils.ts Outdated Show resolved Hide resolved

jcwchen previously approved these changes Jun 14, 2022

View reviewed changes

cmake/onnxruntime_unittests.cmake Show resolved Hide resolved

use better type annotation

d3d69be

garymm dismissed jcwchen’s stale review via d3d69be June 14, 2022 17:16

garymm requested a review from yuslepukhin June 14, 2022 17:16

yuslepukhin reviewed Jun 14, 2022

View reviewed changes

garymm added 2 commits June 14, 2022 18:12

lint and review comments

ef8f4bf

supress pylint

d498d52

fs-eire approved these changes Jun 14, 2022

View reviewed changes

garymm merged commit e8b0d24 into microsoft:master Jun 14, 2022

garymm deleted the tol-overrides branch June 14, 2022 22:12

This was referenced Jul 30, 2022

Build failure with gcc 11 on CentOS 7. #12393

Closed

Compatibility patch for nlohmann/json < 3.9.0. #12394

Merged

xkszltl added a commit to xkszltl/Roaster that referenced this pull request Jul 30, 2022

Patch ort 1.12.0 for json.

aa2fc8f

- microsoft/onnxruntime#12394 - microsoft/onnxruntime#12393 - microsoft/onnxruntime#11775 Upstream change: - nlohmann/json@74520d8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support per-test tolerances for ONNX tests #11775

Support per-test tolerances for ONNX tests #11775

garymm commented Jun 7, 2022 •

edited

Loading

garymm Jun 9, 2022

jcwchen left a comment

fs-eire commented Jun 9, 2022

lgtm-com bot commented Jun 9, 2022

lgtm-com bot commented Jun 10, 2022

lgtm-com bot commented Jun 11, 2022

lgtm-com bot commented Jun 13, 2022

garymm commented Jun 13, 2022

jcwchen left a comment

yuslepukhin Jun 14, 2022

garymm Jun 14, 2022

yuslepukhin Jun 14, 2022

garymm Jun 14, 2022

yuslepukhin commented Jun 14, 2022

fs-eire left a comment

lgtm-com bot commented Jun 14, 2022

garymm commented Jun 14, 2022


		import onnxruntime.backend as c2
		from onnxruntime import backend

Support per-test tolerances for ONNX tests #11775

Support per-test tolerances for ONNX tests #11775

Conversation

garymm commented Jun 7, 2022 • edited Loading

garymm Jun 9, 2022

Choose a reason for hiding this comment

jcwchen left a comment

Choose a reason for hiding this comment

fs-eire commented Jun 9, 2022

lgtm-com bot commented Jun 9, 2022

lgtm-com bot commented Jun 10, 2022

lgtm-com bot commented Jun 11, 2022

lgtm-com bot commented Jun 13, 2022

garymm commented Jun 13, 2022

jcwchen left a comment

Choose a reason for hiding this comment

yuslepukhin Jun 14, 2022

Choose a reason for hiding this comment

garymm Jun 14, 2022

Choose a reason for hiding this comment

yuslepukhin Jun 14, 2022

Choose a reason for hiding this comment

garymm Jun 14, 2022

Choose a reason for hiding this comment

yuslepukhin commented Jun 14, 2022

fs-eire left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 14, 2022

garymm commented Jun 14, 2022

garymm commented Jun 7, 2022 •

edited

Loading