[ONNX] Support Bernoulli op on ONNX front-end by vvchernov · Pull Request #13802 · apache/tvm

vvchernov · 2023-01-18T06:00:22Z

Support Bernoulli op on ONNX front-end

tvm-bot · 2023-01-18T06:00:25Z

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

cc @KJlaccHoeUM9l, @ehsanmok _{See #10317 for details}

_{Generated by tvm-bot}

KJlaccHoeUM9l

LGTM

KJlaccHoeUM9l · 2023-01-23T13:11:21Z

+        assert in_dtype in [
+            "float32",
+            "float64",
+        ], "Only float input tensor is currently supported."


ONNX has support for float16. However, this type is not supported here. Maybe it's worth pointing out the reason (TODO) why this data type is not currently supported at the ONNX front-end level?

vvchernov · 2023-01-23T13:22:48Z

Hello @AndrewZhaoLuo! Could you review this PR? One more question: do you know what means expanded part in test names ("test_bernoulli_expanded", "test_bernoulli_double_expanded", "test_bernoulli_seed_expanded")? I'm ready to support these tests, but do not know what should they do.

echuraev

LGTM. Thanks

AndrewZhaoLuo · 2023-01-24T19:36:25Z

IIRC the expanded tests simply expand the node into things that can be written in terms of existing ops.

E.g.
If we had an “FMA” node in ONNX which does output = in1 * in2 + in3 , the FMA_test would be just an FMA node but the FMA_expanded_test will be a multiplication node and an addition node.

So in FMA_test we would hit the FMA node converter, while for FMA_expanded_test we would hit the multiply and addition node converters

https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/test/onnx/main.cc#L699 If we look here, the bernoulli tests are listed as broken so 🤷

AndrewZhaoLuo

LGTM for most part, just some comments on testing

AndrewZhaoLuo

Actually going to block for now, due to concern about flakes. at least until @octoJon chimes in.

… and tvm sides

…mparison. clean code

vvchernov · 2023-01-25T18:16:02Z

Hello @AndrewZhaoLuo and @octoJon! I think CI will be green soon, Could you recheck updated test part?

AndrewZhaoLuo · 2023-01-26T18:12:07Z

LGTM, just a few final nits.

octoJon · 2023-01-26T18:49:54Z

+            tvm.testing.assert_allclose(inputs, tvm_out)
+        else:
+            # check that mean value is close to the theoretical one by binomial test
+            bnm_test_res = scipy.stats.binomtest(


Nit: Strictly speaking, this test is only appropriate when the input probabilities are all identical. I think it's guaranteed to be over-conservative in cases where the input probabilities are not identical, like when you call verify_bernoulli(shape=[1000]). (By "over-conservative", I mean that the true rate of flaky failures will be less than the nominal 1e-6 implied by the p-value threshold in the test.) Reference for that claim: https://stats.stackexchange.com/a/510798

I would recommend just documenting this with a comment in the code. Something like:

This test is strictly appropriate when input probabilities are all identical. In that case, it should lead to flaky failures in only one run in a million.
The test should be over-conservative when input probabilities are not identical. (i.e., It should have a rate of flaky failures lower than one run in a million.) If this test starts repeatedly throwing flaky failures, consult a statistician in addition to your regular debugging.

Oh, god. I didn't read that reference link closely enough. It contains a really blatantly misogynistic comment. Apologies to anyone I exposed to that. Please don't include that reference link in the code.

(I think the math is right, though.)

Hello @octoJon! I've modified the test due to it was already "over-conservative" with p-value threshold = 1e-6. I've increased threshold to 0.05 as more classical approach. If test condition failed there are two cases: something wrong in the operation or we have gotten "bad" output sequence on the tail of distribution. Due to the last is rare case and should be rechecked I repeat the test again (and third time if need) with new seed for internal distribution (input is the same).
P.S. As you know RandomUniform and RandomNormal already were implemented on TVM side. Possibly their CI tests should be also updated to testing stability and avoiding of flaky failures.

vvchernov · 2023-01-27T12:44:25Z

Hello @AndrewZhaoLuo! Please, recheck, discuss with Jon if need and merge if can

AndrewZhaoLuo

Thanks!

* add Bernoulli converter for onnx front-end * test for bernoulli was implemented * fix tuple split. update test for stability with different seed on ort and tvm sides * check that output values are 0 or 1 * remove std check as meaningless * calculate theoretical mean and compare with result, remove ort for comparison. clean code * add customized input as arg * add test with input sequence of 0 and 1 * pylint fix * fix inputs-shape issue * add binomial test * fix input type * small fix * update 0-1 check * init arrays in numpy style * check result determinism for fixed seed * fix inputs issue * modify binomial test * pylint fix --------- Co-authored-by: Valery Chernov <valery.chernov@deelvin.com>

vvchernov force-pushed the vc/bernoulli branch 2 times, most recently from 441626b to 02c0960 Compare January 18, 2023 12:47

vvchernov changed the title ~~WIP: [ONNX] Support Bernoulli op on ONNX front-end~~ [ONNX] Support Bernoulli op on ONNX front-end Jan 18, 2023

vvchernov force-pushed the vc/bernoulli branch from 63c2bc9 to 04de519 Compare January 23, 2023 09:14

KJlaccHoeUM9l approved these changes Jan 23, 2023

View reviewed changes

echuraev approved these changes Jan 23, 2023

View reviewed changes

AndrewZhaoLuo reviewed Jan 24, 2023

View reviewed changes

Comment thread tests/python/frontend/onnx/test_forward.py Outdated

Comment thread tests/python/frontend/onnx/test_forward.py Outdated

AndrewZhaoLuo requested changes Jan 24, 2023

View reviewed changes

Valery Chernov added 9 commits January 25, 2023 11:25

add Bernoulli converter for onnx front-end

317d750

test for bernoulli was implemented

4fd035a

fix tuple split. update test for stability with different seed on ort…

0b099b0

… and tvm sides

check that output values are 0 or 1

d326ce6

remove std check as meaningless

09b79cc

calculate theoretical mean and compare with result, remove ort for co…

f8e7b0d

…mparison. clean code

add customized input as arg

8962ab7

add test with input sequence of 0 and 1

dd99e6f

pylint fix

5a4724f

vvchernov force-pushed the vc/bernoulli branch from 7acdf35 to 5a4724f Compare January 25, 2023 08:25

Valery Chernov added 4 commits January 25, 2023 13:36

fix inputs-shape issue

fa105a6

add binomial test

e99d6c9

fix input type

863cecb

small fix

53d66ba

AndrewZhaoLuo reviewed Jan 26, 2023

View reviewed changes

Comment thread tests/python/frontend/onnx/test_forward.py Outdated

Comment thread tests/python/frontend/onnx/test_forward.py Outdated

Comment thread tests/python/frontend/onnx/test_forward.py Outdated

octoJon reviewed Jan 26, 2023

View reviewed changes

update 0-1 check

6314fa3

init arrays in numpy style

163e101

vvchernov force-pushed the vc/bernoulli branch from 0189c07 to 163e101 Compare January 27, 2023 05:48

Valery Chernov added 4 commits January 27, 2023 09:01

check result determinism for fixed seed

c19fc49

fix inputs issue

00295c9

modify binomial test

e91364b

pylint fix

3c85a2e

AndrewZhaoLuo approved these changes Jan 27, 2023

View reviewed changes

AndrewZhaoLuo merged commit 1bc8cf8 into apache:main Jan 27, 2023

vvchernov deleted the vc/bernoulli branch January 28, 2023 06:49

ysh329 mentioned this pull request Apr 17, 2023

[Release] v0.12.0 Release Candidate Notes #14645

Closed

Conversation

vvchernov commented Jan 18, 2023

Uh oh!

tvm-bot commented Jan 18, 2023

Uh oh!

KJlaccHoeUM9l left a comment

Choose a reason for hiding this comment

Uh oh!

KJlaccHoeUM9l Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

vvchernov commented Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

echuraev left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewZhaoLuo commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

Uh oh!

vvchernov commented Jan 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewZhaoLuo commented Jan 26, 2023

Uh oh!

octoJon Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

octoJon Jan 26, 2023

Choose a reason for hiding this comment

Uh oh!

vvchernov Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

vvchernov commented Jan 27, 2023

Uh oh!

AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vvchernov commented Jan 23, 2023 •

edited

Loading

AndrewZhaoLuo commented Jan 24, 2023 •

edited

Loading