-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
There was a problem hiding this 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!
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 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 |
This is merged now. Thanks @altanh, @tqchen, @junrushao1994, @jcf94, @mbrookhart and @jroesch! |
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 |
* 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
* 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
see #8579
Lifted the interpreter executor evaluation out of a hot loop in
check_grad
. Additionally, modified the interpreter executor to only create oneInterpreter
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