-
Notifications
You must be signed in to change notification settings - Fork 429
add PluggableDeviceLibrary #402
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
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
cf5c7d4
to
92aa58f
Compare
92aa58f
to
5d7572d
Compare
Hmmm, how can I test this...? |
For mac see #387 |
directml plugin for windows native may not work. See |
|
||
#[ignore] | ||
#[test] | ||
fn load_pluggable_device_library() { |
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.
We might as well leave the test out, since there's no reasonable way to run it (as far as I know). We have similar issues with TF_LoadLibrary.
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.
Yes, right. I tried some, but none of them succeeded. There is too little information on this API.
Is there anything else I can do with this pull request?
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.
In #387 they solved it (at least for macOS) by installing tensorflow-metal and loading that. If you want to leave this test out, I can merge this first and ask them to rebase their changes on top of this, so we'd at least have a test that runs in the CI, even if not on everyone's local machine.
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 adjusting the PRs. If that order is acceptable, please do so.
A while ago, I was trying to see if the C-API would work with a plugin for Windows, and latest plugin version (currently build from source only) seemed to work with TF 2.12. I'm thinking of trying to see if I can support that as well (apart from this PR) when the version of tensorflow here is updated.
Closes #381