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

[FIX][CI] hotfix check_grad perf regression #8581

Merged
merged 4 commits into from
Jul 30, 2021

Conversation

altanh
Copy link
Contributor

@altanh altanh commented Jul 28, 2021

see #8579

Lifted the interpreter executor evaluation out of a hot loop in check_grad. Additionally, modified the interpreter executor to only create one Interpreter per expression evaluation (rather than each time the evaluated expression closure is invoked). Also fixed a bug that was in some code for grouped_conv2d on ARM CPU.

cc @tqchen

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@junrushao junrushao linked an issue Jul 29, 2021 that may be closed by this pull request
@altanh
Copy link
Contributor Author

altanh commented Jul 29, 2021

it turns out the problem goes a bit deeper with the Interpreter executor, please wait to merge until we settle on a complete solution

@altanh
Copy link
Contributor Author

altanh commented Jul 29, 2021

cc @Wheest author of #6137 regarding the fix I added for grouped_conv2d default config on ARM cpu, please check that it's correct

@tqchen
Copy link
Member

tqchen commented Jul 29, 2021

@altanh your change might have triggered other errors. Let us skip the interepreter in the gradient checkers and focus on running VM instead

@altanh
Copy link
Contributor Author

altanh commented Jul 29, 2021

@altanh your change might have triggered other errors. Let us skip the interepreter in the gradient checkers and focus on running VM instead

I saw the errors, I'm trying one more quick fix and if it doesn't work I will open an alternative PR which disables the interpreter in check_grad

@altanh altanh changed the title [FIX][CI] hotfix check_grad perf regression: lift compile out of hot loop [FIX][CI] hotfix check_grad perf regression Jul 29, 2021
@leandron leandron merged commit 8148028 into apache:main Jul 30, 2021
@leandron
Copy link
Contributor

leandron commented Jul 30, 2021

This is merged now. Thanks @altanh, @tqchen, @junrushao1994, @jcf94, @mbrookhart and @jroesch!

@Wheest
Copy link
Contributor

Wheest commented Aug 1, 2021

Thanks @altanh, the ARM grouped conv change LGTM. How did you discover it, if it wasn't caught by the tests in CI?

@altanh
Copy link
Contributor Author

altanh commented Aug 3, 2021

Thanks @altanh, the ARM grouped conv change LGTM. How did you discover it, if it wasn't caught by the tests in CI?

Good question... I was debugging the test locally and encountered the error with a basic pytest invocation. Perhaps the ARM test wasn't being run, or the default config wasn't being used on CI? Definitely weird

ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* hotfix check_grad perf regression: lift compile out of hot loop

* hoist interpreter creation out of python closure, fix weird conv2d bug on arm cpu

* lint

* try one more fix
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* hotfix check_grad perf regression: lift compile out of hot loop

* hoist interpreter creation out of python closure, fix weird conv2d bug on arm cpu

* lint

* try one more fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TEST] Conv2dGrad Takes long time to finish
8 participants