Skip to content

[PluggableDevice] Add pluggable device load mechanism #43610

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.

Sign up for GitHub

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

Jump to bottom
Merged
copybara-service merged 16 commits into from
Dec 9, 2020
Merged

[PluggableDevice] Add pluggable device load mechanism #43610

copybara-service merged 16 commits into from
Dec 9, 2020

Conversation

@jzhoulon
Copy link
Contributor

@jzhoulon jzhoulon commented Sep 28, 2020 •
edited by penpornk
Loading

support for pluggable device plugin loading mechanism, this PR is following Pluggable device for TensorFlow, which provides the capability of loading a pluggable device plugin in the TensorFlow Proper.

Added by @penpornk on 11/18/20:
This PR is part of an effort to let new third-party devices connect to TensorFlow modularly. There are two RFCs:

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Sep 28, 2020
@jzhoulon
Copy link
Contributor Author

jzhoulon commented Sep 28, 2020

@annarev @penpornk @yisitu The first PR is for PluggableDevice load mechanism, Thanks

@gbaned gbaned self-assigned this Sep 28, 2020
@penpornk penpornk requested a review from annarev September 28, 2020 17:42
@gbaned gbaned added the awaiting review Pull request awaiting review label Sep 30, 2020
annarev
annarev reviewed Sep 30, 2020
View reviewed changes
annarev
annarev reviewed Oct 1, 2020
View reviewed changes
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 3, 2020
@gbaned
Copy link
Contributor

gbaned commented Oct 6, 2020

@jzhoulon Can you please check @annarev's comments and keep us posted ? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Oct 6, 2020
@yiqianglee
Copy link
Contributor

yiqianglee commented Oct 7, 2020

@gbaned , sorry to late response, we are in a long public holiday last week, will address all comments soon. Thanks!

@jzhoulon
Copy link
Contributor Author

jzhoulon commented Oct 9, 2020

@annarev @gbaned I have addressed all the comments, please help to review. thanks.

@gbaned gbaned removed the stat:awaiting response Status - Awaiting response from author label Oct 9, 2020
@gbaned gbaned requested a review from annarev October 14, 2020 15:59
@gbaned gbaned added the awaiting review Pull request awaiting review label Oct 14, 2020
annarev
annarev reviewed Oct 14, 2020
View reviewed changes
@gbaned gbaned requested a review from annarev October 15, 2020 11:18
annarev
annarev reviewed Oct 17, 2020
View reviewed changes
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 19, 2020
@gbaned
Copy link
Contributor

gbaned commented Oct 19, 2020

@jzhoulon Can you please check @annarev's comments and keep us posted ? Thanks!

@gbaned gbaned requested a review from annarev October 23, 2020 18:16
@annarev
Copy link
Contributor

annarev commented Nov 24, 2020

Currently our preference is user no need call load_pluggable_device_librarymanually, plugin authors need to make sure that the package will be installed to the specified dir(…python_dir.../site-packages/tensorflow-plugins), it's optional for application programmers call this api to load the library if it is not installed to the specified directory(I didn't think out a usecase for this yet, @annarev do you have opinion on this?)

I don't have a usecase in mind either right now. We can always expose it later if we find such case.

@penpornk penpornk added the ready to pull PR ready for merge process label Nov 25, 2020
@penpornk
Copy link
Member

penpornk commented Nov 25, 2020

Tagging ready-to-pull back so this PR can re-enter the API review tomorrow.

@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 Nov 25, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 25, 2020
manivaradarajan
manivaradarajan approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@manivaradarajan manivaradarajan left a comment

Choose a reason for hiding this comment

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

For @tensorflow/api-owners: API changes LGTM. Please address other comments before merging.

@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Dec 3, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 3, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Dec 4, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 5, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 5, 2020
@kulinseth
Copy link
Contributor

kulinseth commented Dec 5, 2020

@fchollet Thank you for the review!

  • @kulinseth et al. are considering developing a Metal device plug-in. More details in the PluggableDevice design review notes [1, 2].

Thanks @penpornk and @fchollet, yes we are actively working on adding the Metal device plug-in using the modular design's pluggable interface.

manivaradarajan
manivaradarajan approved these changes Dec 6, 2020
View reviewed changes
Copy link
Member

@manivaradarajan manivaradarajan left a comment

Choose a reason for hiding this comment

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

Please address comment wording suggestions before merging.

@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Dec 6, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 6, 2020
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Dec 6, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 7, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 7, 2020
@copybara-service copybara-service bot merged commit d163783 into tensorflow:master Dec 9, 2020
@sclarkson sclarkson mentioned this pull request Jul 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rohan100jain rohan100jain Awaiting requested review from rohan100jain rohan100jain is a code owner

@sanjoy sanjoy Awaiting requested review from sanjoy

@manivaradarajan manivaradarajan Awaiting requested review from manivaradarajan

2 more reviewers

@fchollet fchollet fchollet left review comments

@annarev annarev annarev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gbaned gbaned

Labels

API review API Review cla: yes ready to pull PR ready for merge process size:L CL Change Size: Large

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.