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] Remove extra device stuff passing to the constructor #6689

Merged
merged 3 commits into from
Jul 29, 2022

Conversation

haoyunfeix
Copy link
Contributor

@haoyunfeix haoyunfeix commented Jul 28, 2022

BUG

While passing both device and device feature to the constructor,
there is a potential mismatch between them and cause error, so only keep
unique device in backend constructor's parameter.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@haoyunfeix
Copy link
Contributor Author

@qjia7 PTAL

Copy link
Contributor

@qjia7 qjia7 left a comment

Choose a reason for hiding this comment

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

LGTM with one nit. Thanks.

@@ -155,6 +155,14 @@ export class WebGPUBackend extends KernelBackend {
type: 'timestamp',
count: 2,
});
} else {
console.warn(
`This device doesn't support timestamp-query extension. ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove the intermediate +. Only one pair of `` is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only one pair of `` will separate long warnings to several lines and keep extra spaces at the beginning of each line. image
Below is what is looks like now (in one line):
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can adjust your output layout.
For example:

      console.warn(`
This device doesn't support timestamp-query extension.
Start Chrome browser with flag
--disable-dawn-features=disallow_unsafe_apis then try again.
Or zero will shown for the kernel time when profiling mode is
enabled. Using performance.now is not workable for webgpu since
it doesn't support synchronously to read data from GPU.`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this also break one sentence into 6 lines, sometimes may cause the typography issue.
For example when console window is less than 70 width(debugging in a small corner), 6 lines will became 6 paragraphs(12+ lines), instead 1 line message will stay in 1 paragraph, easy to read.

@qjia7 qjia7 requested a review from gyagp July 28, 2022 06:50
`--disable-dawn-features=disallow_unsafe_apis then try again. ` +
`Or zero will shown for the kernel time when profiling mode is` +
`enabled. Using performance.now is not workable for webgpu since` +
`it doesn't support synchronously to read data from GPU.`);
Copy link

Choose a reason for hiding this comment

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

I don't quite understand why we always need this warning. This is a development only extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed a bug #6691 related with it. Agree with Yang's opinion. We should only print this warning when the users really use it.

BUG

While passing both device and device feature to the constructor,
there is a potential mismatch between them and cause error, so only keep
unique device in backend constructor's parameter.
Copy link

@gyagp gyagp left a comment

Choose a reason for hiding this comment

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

LGTM

@gyagp gyagp merged commit 51da198 into tensorflow:master Jul 29, 2022
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