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

Fix FromPixels type checker to avoid error in worker #5472

Merged
merged 3 commits into from Aug 16, 2021

Conversation

shaoboyan
Copy link
Contributor

@shaoboyan shaoboyan commented Aug 12, 2021

TFJS is possible running in worker, which doesn't have
ability to access DOM elements.

FromPixels op in tfjs webgpu backend type check
doesn't consider this scene. It will break the whole
program even if user provided correct input params.

Fix this by checking whether DOM is available.

Bug #5467

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


This change is Reviewable

TFJS is possible running in worker, which doesn't have
ability to access DOM elements.

FromPixels op in tfjs webgpu backend type check
doesn't consider this scene. It will break the whole
program even if user provided correct input params.

Fix this by checking whether DOM is available.

Bug tensorflow#5467
@google-cla google-cla bot added the cla: yes label Aug 12, 2021
@shaoboyan
Copy link
Contributor Author

@lina128 ,
Hi, Na,
I've tried to run the worker_test here(https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/worker_test.ts) in webgpu backend. But I've got an error that it cannot load the script from location.origin + '/base/tfjs/tfjs-core/tf-core.min.js' (https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/worker_test.ts#L29).
Do you know how to fix this error?

@shaoboyan
Copy link
Contributor Author

@qjia7 Also cc jiajia

Copy link
Collaborator

@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 a nit.
@lina128 take another look, thanks.

tfjs-backend-webgpu/src/kernels/FromPixels.ts Outdated Show resolved Hide resolved
@qjia7 qjia7 requested a review from lina128 August 13, 2021 01:40
Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @qjia7)


tfjs-backend-webgpu/src/kernels/FromPixels.ts, line 65 at r1 (raw file):

Previously, qjia7 (Jiajia Qin) wrote…

nit: Maybe this checking is not needed since we have done it in the upper level ( tfjs-core

throw new Error(
)

+1

@lina128
Copy link
Collaborator

lina128 commented Aug 13, 2021

@lina128 ,
Hi, Na,
I've tried to run the worker_test here(https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/worker_test.ts) in webgpu backend. But I've got an error that it cannot load the script from location.origin + '/base/tfjs/tfjs-core/tf-core.min.js' (https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/worker_test.ts#L29).
Do you know how to fix this error?

Hi @shaoboyan, to run the worker test, you need to have some additional karma setup, like the one in core: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/karma.conf.js#L63-L65

@shaoboyan
Copy link
Contributor Author

@qjia7 Done.

@qjia7 qjia7 merged commit f91c1d1 into tensorflow:master Aug 16, 2021
@qjia7
Copy link
Collaborator

qjia7 commented Aug 16, 2021

@lina128 ,
Hi, Na,
I've tried to run the worker_test here(https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/worker_test.ts) in webgpu backend. But I've got an error that it cannot load the script from location.origin + '/base/tfjs/tfjs-core/tf-core.min.js' (https://github.com/tensorflow/tfjs/blob/master/tfjs-core/src/worker_test.ts#L29).
Do you know how to fix this error?

Hi @shaoboyan, to run the worker test, you need to have some additional karma setup, like the one in core: https://github.com/tensorflow/tfjs/blob/master/tfjs-core/karma.conf.js#L63-L65

@shaoboyan Please follow up the worker test in a separate PR. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants