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

[oneDNN] Fix conv3d_backprop_filter_v2_grad_test_cpu test failure when oneDNN … #55940

Conversation

yimeisun123
Copy link
Contributor

…is enabled
The commit https://github.com/intel-innersource/frameworks.ai.tensorflow.private-tensorflow/commit/174c5096f303d5be7ed2ca2662b08371bff4ab88 introduced a new test case ( testBadFilterShape) in the conv3d_backprop_filter_v2_grad_test.py. This test case has a bad filter shape and expects the corresponding kernel to check for this condition. Update the mkl kernel to add the check for invalid filter shape.

@yimeisun123 yimeisun123 requested a review from penpornk as a code owner May 5, 2022 19:35
@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label May 5, 2022
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label May 5, 2022
@github-actions github-actions bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 5, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 5, 2022
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@@ -370,6 +370,14 @@ class MklConvCustomBackpropFilterOp
// a correct way to get filter shape. These operator-specific calls
// allow this class to handle this case.
TensorShape src_tf_shape = MakeInputTfShape(context, src_tensor);
const string& op_type = this->type_string();
if ((op_type.find("3D") != std::string::npos) &&
Copy link
Member

Choose a reason for hiding this comment

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

If the fix is only specific to Conv3DBackpropFilterV2, why don't we just check for the full string?

Suggested change
if ((op_type.find("3D") != std::string::npos) &&
if (op_type == "Conv3DBackpropFilterV2")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@penpornk - thanks for the quick response. In the oneDNN enabled build, the op is replaced by mkl op, there are currently two mkl ops (_MklConv3DBackpropFilterV2, line 745, _MklNativeConv3DBackpropFilterV2, line 763) need to be checked. I used this checking instead of the exact op name similar as what is done in commit 174c509 for the eigen kernel.
Please let me know if you want me to make the change or stay with current code. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation! I forgot about the native and non-native versions. Let's keep the current code. :)

@penpornk penpornk removed the awaiting review Pull request awaiting review label May 5, 2022
@@ -370,6 +370,14 @@ class MklConvCustomBackpropFilterOp
// a correct way to get filter shape. These operator-specific calls
// allow this class to handle this case.
TensorShape src_tf_shape = MakeInputTfShape(context, src_tensor);
const string& op_type = this->type_string();
if ((op_type.find("3D") != std::string::npos) &&
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation! I forgot about the native and non-native versions. Let's keep the current code. :)

@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label May 5, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 5, 2022
@yimeisun123
Copy link
Contributor Author

Two test are failed, I am trying to look at the details, but not sure if the failure is related to this PR change. For the Code Check-Changed Files, I couldn't see what exactly failed.
For the Py+CPP Test Suite, I see //tensorflow/c/eager:c_api_test_gpu failed, not sure if it is from this PR change.
Please let me know if I miss anything. Thanks.

@copybara-service copybara-service bot merged commit 49de5ed into tensorflow:master May 6, 2022
@yimeisun123 yimeisun123 deleted the yimei/fix_conv3d_backprop_filter_v2_grad_test branch May 9, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants