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

webgpu: fix: conditionally call deprecated GPUAdapter.requestAdapterInfo #8392

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

mattvr
Copy link
Contributor

@mattvr mattvr commented Oct 2, 2024

Context

GPUAdapter.requestAdapterInfo is deprecated and non-standard.

It's in use by tfjs-backend-webgpu, but the result is optional, and only used for a single optimization. Yet if called in an environment which doesn't support it, it will throw.

By checking the feature's available before calling it, this unbreaks Deno runtime support for tfjs-backend-webgpu, which doesn't support this feature and throws.

Testing/Verification

yarn test and yarn lint succeed. Verified basic example now works in Deno.

import * as tf from "@tensorflow/tfjs-core";
import "@tensorflow/tfjs-backend-webgpu";

await tf.ready();
tf.randomGamma([2, 2], 1).print();

Related

denoland/deno#22029

Copy link

google-cla bot commented Oct 2, 2024

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.

@mattsoulanille
Copy link
Member

/gcbrun

@mattsoulanille mattsoulanille self-requested a review October 10, 2024 22:28
@mattsoulanille
Copy link
Member

I'm seeing this error in the build:

tfjs-backend-webgpu/src/base.ts:64:38 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'GPUAdapterInfo'.
  Type '{}' is missing the following properties from type 'GPUAdapterInfo': __brand, vendor, architecture, device, description

64     return new WebGPUBackend(device, adapterInfo);
                                        ~~~~~~~~~~~

Maybe our WebGPU types from NPM are out of date? Does yarn build work in the tfjs-backend-webgpu directory for you?

@mattvr
Copy link
Contributor Author

mattvr commented Oct 10, 2024

@mattsoulanille Thank you for reviewing, yeah – I'm seeing that too with the previous commit. Oudated WebGPU types was it!

The latest commit fixes the error. It keeps the adapter.requestAdapterInfo() fallback for backwards compatibility, to prevent breaking older environments. This required an any typecast though, let me know if you'd like any changes.

@mattsoulanille
Copy link
Member

/gcbrun

@mattsoulanille
Copy link
Member

Thanks! The any is probably okay since it gets immediately passed to another function that has a defined type for that argument, so the any type shouldn't leak anywhere.

@mattsoulanille mattsoulanille enabled auto-merge (squash) October 11, 2024 04:43
Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

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

Looks like the linter isn't happy with any.

tfjs-backend-webgpu/src/base.ts Show resolved Hide resolved
auto-merge was automatically disabled October 11, 2024 06:47

Head branch was pushed to by a user without write access

@mattvr mattvr force-pushed the mattvr/requestAdapterInfo branch from 15d292c to b243896 Compare October 11, 2024 06:47
@mattvr
Copy link
Contributor Author

mattvr commented Oct 11, 2024

@mattsoulanille Sorry for the back-and-forth, could've sworn build and test succeeded before I submitted the last two commits. Should be fixed now! 🤞

@mattsoulanille
Copy link
Member

No worries! Our build and test scripts are not particularly easy to use (sorry). There's a yarn lint command you can run at the root of the repo to check this.

@mattsoulanille
Copy link
Member

/gcbrun

@mattsoulanille mattsoulanille enabled auto-merge (squash) October 11, 2024 07:10
@mattsoulanille
Copy link
Member

/gcbrun

@mattsoulanille mattsoulanille merged commit 33d939a into tensorflow:master Oct 11, 2024
2 checks passed
@mattvr mattvr deleted the mattvr/requestAdapterInfo branch November 7, 2024 14:32
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.

3 participants