[Op] Do not override specified layout in pooling (2nd PR) · Pull Request #9328 · apache/tvm

ghost · 2021-10-19T21:11:43Z

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

ghost · 2021-10-19T21:16:19Z

@vinx13, @comaniac, @masahi:
Sorry for the trouble, I have created a new PR for this issue so that all changes are shown in one commit.
Please abandon the previous PR-#9235.
This time, if needed, for any new changes, I will following advise to: either use github GUI to do rebase-and-merge or use git pull --rebase as suggested.
Thanks,

Joe

ghost · 2021-10-19T21:30:38Z

FYI - a new "out_layout" attribute is added for pooling so that this field can be used by user to specify a user-specific layout for pooling op (similar to what we have for conv op); then, in the pooling.cc file, we can based on the value of out_layout to determine whether to use & stick to the user-specified layout or to use relay caller's infer layout for pooling.
4 new test cases, which use user-specified layout, are added to the test_pass_convert_op_layout.py test suite.

comaniac

Otherwise LGTM.

ghost · 2021-10-19T22:14:02Z

Thanks for the prompt feedback. All 3 of your suggestions make sense and I will find time to make them into the PR. Hopefully, this time I am not going to screw up the git pull –rebase. * Joe From: Cody Yu ***@***.***> Sent: Tuesday, October 19, 2021 3:09 PM To: apache/tvm ***@***.***> Cc: Joe Chou ***@***.***>; Author ***@***.***> Subject: [EXT] Re: [apache/tvm] [Op] Do not override specified layout in pooling (2nd PR) (PR #9328) External Email

________________________________ @comaniac commented on this pull request. Otherwise LGTM.

________________________________ In src/relay/op/nn/pooling.cc<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_tvm_pull_9328-23discussion-5Fr732274127&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=84zI-iH2xz28Q_xujSGbjYaq38CgcRgl_vGzCtF6TwQ&m=deTteN0zq7kQqrwuzCJQ3p2axsN9PJ7v8xQjQGIve18&s=qOJGnkqHVHovYYnBYzVuOUzkvFQ3j6c4fPi9Iyc0ekQ&e=>:

+ if (params->out_layout != "") {

+ // when users specify the out_layout of pooling, transforms.ConvertLayout pass will + // follow user's preference + ICHECK_EQ(params->layout, params->out_layout); ⬇️ Suggested change - if (params->out_layout != "") { - // when users specify the out_layout of pooling, transforms.ConvertLayout pass will - // follow user's preference - ICHECK_EQ(params->layout, params->out_layout); + if (params->out_layout != "") { + // when users specify the out_layout of pooling, follow user's preference. + ICHECK_EQ(params->layout, params->out_layout) << "Pooling input/output layouts mismatch: " << params->layout << " vs. " << params->out_layout;

________________________________ In tests/python/relay/test_pass_convert_op_layout.py<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_tvm_pull_9328-23discussion-5Fr732275775&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=84zI-iH2xz28Q_xujSGbjYaq38CgcRgl_vGzCtF6TwQ&m=deTteN0zq7kQqrwuzCJQ3p2axsN9PJ7v8xQjQGIve18&s=p9Tev2QBahzEmthRelJ_5ZfZ04wmSV8UnNs9DEimbIs&e=>:

+ @tvm.ir.register_op_attr("nn.max_pool2d", "FTVMConvertOpLayout")

+ def convert_maxpool2d(attrs, inputs, tinfos, desired_layouts): + # stick by convertng layout and out_layout to use NHWC and NHWC, + # respectively, as specified in the transforms.ConvertLayout() function's arguments later + new_attrs = dict(attrs) + new_attrs["layout"] = str(desired_layouts[0]) + new_attrs["out_layout"] = str(desired_layouts[0]) + return relay.nn.max_pool2d(*inputs, **new_attrs) We should register this to relay/op/nn/_nn.py once along with other converters such as conv2d. Otherwise users will need to manually specify this function to use the feature.

________________________________ In tests/python/relay/test_pass_convert_op_layout.py<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_tvm_pull_9328-23discussion-5Fr732278336&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=84zI-iH2xz28Q_xujSGbjYaq38CgcRgl_vGzCtF6TwQ&m=deTteN0zq7kQqrwuzCJQ3p2axsN9PJ7v8xQjQGIve18&s=pI9YHqoLpOCITuyaT-5dO4Larr1Emt-G6YFenCPryXs&e=>:

@@ -2039,5 +2039,337 @@ def expected():

assert tvm.ir.structural_equal(a, b), "Actual = \n" + str(a) +def test_conv_max_pool_uses_specified_convert_layout(): Better to put this test along with other existing pooling tests. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_apache_tvm_pull_9328-23pullrequestreview-2D783824550&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=84zI-iH2xz28Q_xujSGbjYaq38CgcRgl_vGzCtF6TwQ&m=deTteN0zq7kQqrwuzCJQ3p2axsN9PJ7v8xQjQGIve18&s=LwUNrVv5yQEZGrLGk0j6SIUjicyyvj__N2uTn0SmrKc&e=>, or unsubscribe<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AM636PHJNAD42HPORL3O5FDUHXUBHANCNFSM5GKDB5YQ&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=84zI-iH2xz28Q_xujSGbjYaq38CgcRgl_vGzCtF6TwQ&m=deTteN0zq7kQqrwuzCJQ3p2axsN9PJ7v8xQjQGIve18&s=fourqVbNZv9CB2yb2moONKwf8TEH9hZg1CUFFX-knac&e=>. Triage notifications on the go with GitHub Mobile for iOS<https://urldefense.proofpoint.com/v2/url?u=https-3A__apps.apple.com_app_apple-2Dstore_id1477376905-3Fct-3Dnotification-2Demail-26mt-3D8-26pt-3D524675&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=84zI-iH2xz28Q_xujSGbjYaq38CgcRgl_vGzCtF6TwQ&m=deTteN0zq7kQqrwuzCJQ3p2axsN9PJ7v8xQjQGIve18&s=xkQavsjIkg1P3x9Ba7ffv8fXckYgq_BEJVgYAA74p7c&e=> or Android<https://urldefense.proofpoint.com/v2/url?u=https-3A__play.google.com_store_apps_details-3Fid-3Dcom.github.android-26referrer-3Dutm-5Fcampaign-253Dnotification-2Demail-2526utm-5Fmedium-253Demail-2526utm-5Fsource-253Dgithub&d=DwMFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=84zI-iH2xz28Q_xujSGbjYaq38CgcRgl_vGzCtF6TwQ&m=deTteN0zq7kQqrwuzCJQ3p2axsN9PJ7v8xQjQGIve18&s=dA-9WtyOYvlSuGQWwG1PecGytGZwkfiuyWxtWn2bN28&e=>.

comaniac

Last batch of few comments.

comaniac

LGTM.

masahi · 2021-10-21T04:40:31Z

@ccjoechou Looks like you've hit a flaky CI test, please run a new job again (rebase and push).
https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/PR-9328/6/pipeline/273

ghost · 2021-10-21T14:59:32Z

@masahi: Got it and I will rebase & push to trigger a new Jenkins build & test run.

ghost · 2021-10-21T15:45:10Z

@masahi, @vinx13:
BTW, we have also installed TVM Jenkinsfile with minor adjustments in order to meet requirements of our Jenkins servers. We can't get the Unit Test - python3: i386 to pass and were seeing similar ci-test error (similar to the 273 one, e.g., setup-pythest-env.sh reaching seg fault). Any tips for us to check our setup or installation here? This Jenkins server node is a ubuntu 20.04 one.
FYI - we were able to "manually" run docker/bash.sh ci steps on another machine, a ubuntu-18.04 one, and we can run Unit Test - python3: i386 w/o problems here.
Thanks.

masahi · 2021-10-21T18:50:39Z

Yes this is a known new CI issue since this week. I've sent a PR to disable the flaky i368 test for now #9344

masahi · 2021-10-21T21:53:56Z

Thanks @ccjoechou @comaniac

* [Op] Do not override specified layout in pooling (2nd PR) * [Op] Do not override specified layout in pooling (2nd PR) * [Op] Do not override specified layout in pooling (2nd PR) * [Op] Do not override specified layout in pooling (2nd PR)

[Op] Do not override specified layout in pooling (2nd PR)
b2e30b3

ghost requested review from MarisaKirisame, ZihengJiang, anijain2305, areusch, comaniac, jroesch, junrushao, jwfromm, mbrookhart, merrymercy, slyubomirsky, tqchen, vinx13, wweic, yzhliu and zhiics as code owners October 19, 2021 21:11

comaniac reviewed Oct 19, 2021
View reviewed changes

Comment threadsrc/relay/op/nn/pooling.cc Outdated
Comment threadtests/python/relay/test_pass_convert_op_layout.py Outdated
Comment threadtests/python/relay/test_pass_convert_op_layout.py

Joe Chouand others added 2 commits October 19, 2021 17:05

Merge branch 'apache:main' into op-pooling
787394f

[Op] Do not override specified layout in pooling (2nd PR)
8a75869

comaniac reviewed Oct 20, 2021
View reviewed changes

Comment threadpython/tvm/relay/op/nn/_nn.py Outdated
Comment threadpython/tvm/relay/op/nn/_nn.py Outdated
Comment threadpython/tvm/relay/op/nn/_nn.py Outdated

[Op] Do not override specified layout in pooling (2nd PR)
30bf278

comaniac approved these changes Oct 20, 2021
View reviewed changes

masahi approved these changes Oct 21, 2021
View reviewed changes

Comment threadpython/tvm/relay/op/nn/_nn.py Outdated

Joe Chouand others added 2 commits October 20, 2021 18:46

Merge branch 'apache:main' into op-pooling
fc9c53b

[Op] Do not override specified layout in pooling (2nd PR)
c099515

Merge branch 'apache:main' into op-pooling
c4a74e2

masahi merged commit d11bdcd into apache:mainOct 21, 2021

Uh oh!

Conversation

ghost commented Oct 19, 2021

Uh oh!

ghost commented Oct 19, 2021

Uh oh!

ghost commented Oct 19, 2021

Uh oh!

comaniac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Oct 19, 2021 via email

Uh oh!

comaniac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comaniac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

masahi commented Oct 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 21, 2021

Uh oh!

ghost commented Oct 21, 2021

Uh oh!

masahi commented Oct 21, 2021

Uh oh!

masahi commented Oct 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

masahi commented Oct 21, 2021•
edited
Loading