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

add PluggableDeviceLibrary #402

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

dskkato
Copy link
Contributor

@dskkato dskkato commented Mar 4, 2023

Closes #381

@google-cla
Copy link

google-cla bot commented Mar 4, 2023

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.

@dskkato
Copy link
Contributor Author

dskkato commented Mar 5, 2023

Hmmm, how can I test this...?

@dskkato
Copy link
Contributor Author

dskkato commented Mar 5, 2023

For mac see #387

@dskkato
Copy link
Contributor Author

dskkato commented Mar 6, 2023

@adamcrume adamcrume self-requested a review March 7, 2023 04:23

#[ignore]
#[test]
fn load_pluggable_device_library() {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

https://github.com/microsoft/tensorflow-directml-plugin

@dskkato dskkato changed the title [WIP] add PluggableDeviceLibrary add PluggableDeviceLibrary Mar 7, 2023
@dskkato dskkato requested a review from adamcrume March 25, 2023 23:56
@adamcrume adamcrume merged commit ae92c63 into tensorflow:master Apr 21, 2023
@dskkato dskkato deleted the add_pluggable_device branch April 21, 2023 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TF_LoadPluggableDeviceLibrary API
2 participants