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

[PluggableDevice] Add TF_OpKernelConstruction_GetNodeDef #52157

Merged
merged 4 commits into from Aug 8, 2022

Conversation

PatriceVignola
Copy link
Contributor

Although the API has functions like TF_OpKernelConstruction_GetAttrInt32, it lacks an abstract to iterate through all the attributes and get their names as well as their value. Since some device backends have relatively slow kernel creation times, being able to access the NodeDef allows them to uniquely identify kernels for caching purposes.

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Sep 28, 2021
@google-cla google-cla bot added the cla: yes label Sep 28, 2021
@gbaned gbaned self-assigned this Sep 28, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 28, 2021
@gbaned gbaned requested a review from sjamesr September 28, 2021 12:54
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Sep 28, 2021
@gbaned gbaned requested a review from jpienaar October 28, 2021 14:24
@gbaned
Copy link
Contributor

gbaned commented Dec 3, 2021

@jpienaar Can you please review this PR ? Thanks!

@jpienaar
Copy link
Member

jpienaar commented Dec 3, 2021

We already have taken some steps to get rid of most iteration over attributes towards enabling the backing to not be NodeDef.

Additionally exposing the NodeDef results in exposing unregistered attributes which are in the UB part of the world. But I don't know what the policy is PluggableDevice side.

@PatriceVignola
Copy link
Contributor Author

@jpienaar I can certainly change this PR provide ways to iterate through attributes instead of exposing the NodeDef itself. The ultimate goal here is to be able to cache kernels that have identical attributes to avoid creating kernels that have a long initialization cost multiple times. Can you point me towards the steps taken so far to decouple attributes from the NodeDef?

@gbaned
Copy link
Contributor

gbaned commented Dec 31, 2021

@jpienaar Can you please assist on above comments from @PatriceVignola. Thanks!

@gbaned
Copy link
Contributor

gbaned commented Feb 10, 2022

@jpienaar Can you please assist on above comments from @PatriceVignola. Thanks!

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Feb 20, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Feb 20, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 20, 2022
@mihaimaruseac mihaimaruseac changed the title [PluggableDevice] Add TF_OpKernelConstruction_GetNodeDef [PluggableDevice] Add TF_OpKernelConstruction_GetNodeDef Feb 21, 2022
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Feb 23, 2022
@gbaned gbaned removed the ready to pull PR ready for merge process label Mar 4, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 4, 2022
@gbaned gbaned added ready to pull PR ready for merge process and removed kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 4, 2022
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jun 17, 2022
@gbaned gbaned added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jul 1, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 1, 2022
@gbaned gbaned added ready to pull PR ready for merge process kokoro:force-run Tests on submitted change and removed ready to pull PR ready for merge process labels Jul 13, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 14, 2022
@gbaned
Copy link
Contributor

gbaned commented Jul 20, 2022

@PatriceVignola Can you please resolve conflicts? Thank you!

@gbaned gbaned removed the ready to pull PR ready for merge process label Jul 20, 2022
@PatriceVignola
Copy link
Contributor Author

@gbaned Done!

@gbaned gbaned requested review from sjamesr and removed request for sjamesr July 21, 2022 14:29
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Jul 21, 2022
@gbaned
Copy link
Contributor

gbaned commented Aug 3, 2022

@PatriceVignola Can you please resolve conflicts? Thank you!

@gbaned gbaned removed the awaiting review Pull request awaiting review label Aug 3, 2022
@PatriceVignola
Copy link
Contributor Author

@gbaned Done!

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 4, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 4, 2022
@copybara-service copybara-service bot merged commit 7fd2a3f into tensorflow:master Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process 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