Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Apr 14, 2020

Node test use cpu backend: https://github.com/tensorflow/tfjs/blob/master/tfjs-node/src/image_test.ts#L224

The test total was 3738, comparing to 3724 after. The 14 tests belong to array_util_test and complex_util_test in backends, which is not backend agnostic and thus excluded from tests.ts. This is expected.


This change is Reviewable

@lina128 lina128 requested a review from pyu10055 April 15, 2020 00:14
Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

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

@lina128 Is that the only test in node backend that uses CPU?

I think this test should be changed as none of its behaviour depends on the CPU backend specifically. Rather than use the CPU backend it should create a mock backend and set it and run its test.

See engine_test for examples of this. kernel_registry_test also does uses the same approach.

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang, @pyu10055, and @tafsiri)

Copy link
Collaborator Author

@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.

Good idea. Done.

Reviewable status: 0 of 1 approvals obtained (waiting on @kangyizhang and @pyu10055)

@lina128 lina128 requested a review from tafsiri April 15, 2020 17:43
Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @kangyizhang, @lina128, @pyu10055, and @tafsiri)


tfjs-node/src/image_test.ts, line 227 at r1 (raw file):

    try {
      const testBackend = new TestKernelBackend();
      registerBackend('sync', () => testBackend);

nit: rename this to something else (e.g. 'testBackend'). the sync nature of it isn't particularly relevant here.


tfjs-node/src/nodejs_kernel_backend_test.ts, line 82 at r1 (raw file):

      const testBackend = new TestKernelBackend();
      tf.registerBackend('sync', () => testBackend);
      tf.setBackend('sync');

nit: same as above.

Copy link
Collaborator Author

@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.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @kangyizhang and @pyu10055)


tfjs-node/src/image_test.ts, line 227 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

nit: rename this to something else (e.g. 'testBackend'). the sync nature of it isn't particularly relevant here.

Done.


tfjs-node/src/nodejs_kernel_backend_test.ts, line 82 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

nit: same as above.

Done.

@lina128 lina128 changed the title Add cpu as a dev dependency to tfjs-node. Remove cpu backend in tfjs-node testing, use fake instead. Apr 15, 2020
@lina128 lina128 merged commit b05cfd2 into tensorflow:master Apr 15, 2020
@lina128 lina128 deleted the fix1 branch April 15, 2020 17:58
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.

3 participants