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] Fixing eager-related nightly test failures #52204

Merged

Conversation

mahmoud-abuzaina
Copy link
Contributor

This PR fixes : //tensorflow/python/kernel_tests:matmul_op_test and run_eager_op_as_function_test,
These tests which are run with eager op as function enabled, fail with error , "Could not find device for node:"

With a recent commit on public tensorflow : 1224326
Eager ops are wrapped, inside function WrapInCallOp, after the device has been inferred for op.

For MKL : we expect WrapInCallOp to be called before we infer device, which marks the '_kernel' attribute as NameChangeOp label for nodeDef of funcDef.
Since with new commit, WrapInCallOp is called AFTER the device inference, no device is found for the MKL Op as no kernel match is found without namechange label.

In this PR: we mark nodeDef with '_kernel' attribute as NameChange label at both places : a) before inferring the device and b) inside WrapInCallOp (already present in current code base)

@google-ml-butler google-ml-butler bot added size:S CL Change Size: Small awaiting review Pull request awaiting review labels Sep 30, 2021
@google-cla google-cla bot added the cla: yes label Sep 30, 2021
@kkimdev kkimdev requested review from saxenasaurabh and removed request for qqfish October 1, 2021 05:57
@kkimdev
Copy link
Contributor

kkimdev commented Oct 1, 2021

@saxenasaurabh It sounds like related to the eager-op-as-function project, Could you take a look at this?

@gbaned gbaned self-assigned this Oct 1, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Oct 1, 2021
@gbaned gbaned added the comp:core issues related to core part of tensorflow label Oct 1, 2021
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!

@@ -727,6 +727,16 @@ Status PopulateRetMap(FunctionDef* fdef, const AbstractOpAttrs* op_attrs,
return Status::OK();
}

#ifdef INTEL_MKL
void GetMKLNodeDef(NodeDef* ndef) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this an inline function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Changes done.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Oct 6, 2021
@penpornk penpornk removed the awaiting review Pull request awaiting review label Oct 6, 2021
@google-cla
Copy link

google-cla bot commented Oct 6, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Oct 6, 2021
@google-cla
Copy link

google-cla bot commented Oct 6, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@sachinmuradi
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 6, 2021
@gbaned gbaned requested a review from penpornk October 7, 2021 10:13
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Oct 7, 2021
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 change!

@penpornk penpornk removed the awaiting review Pull request awaiting review label Oct 7, 2021
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Oct 7, 2021
@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Oct 7, 2021
@google-ml-butler google-ml-butler bot added the ready to pull PR ready for merge process label Oct 7, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 7, 2021
@penpornk penpornk added the kokoro:force-run Tests on submitted change label Oct 11, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 11, 2021
@copybara-service copybara-service bot merged commit 3649013 into tensorflow:master Oct 11, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:core issues related to core part of tensorflow size:S CL Change Size: Small
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

6 participants