Skip to content

Add reduce and made some changes as requested #2617

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Dec 30, 2019
Merged

Add reduce and made some changes as requested #2617

merged 10 commits into from
Dec 30, 2019

Conversation

@NALLEIN
Copy link
Contributor

@NALLEIN NALLEIN commented Dec 24, 2019 •
edited by nsthorat
Loading

[webgpu] Add sub, abs, min, max kernels. #2365
Made some changed as requested and rebase this branch.
To see the logs from the Cloud Build CI, please join either our [discussion]
(https://groups.google.com/a/tensorflow.org/forum/#!forum/tfjs) or announcement mailing list.

This change is Reviewable

annxingyuan
annxingyuan reviewed Dec 26, 2019
View reviewed changes
tfjs-backend-webgpu/src/backend_webgpu.ts Outdated
import {FusedConv2DConfig} from '@tensorflow/tfjs-core/dist/ops/fused_util';
// TODO TODO to import computeOptimalWindowSize and other reduce_util function
// from backend_util, rather than from 'dist'
import {assertAxesAreInnerMostDims, computeOutAndReduceShapes} from '@tensorflow/tfjs-core/src/ops/axis_util';
Copy link
Contributor

@annxingyuan annxingyuan Dec 26, 2019

Choose a reason for hiding this comment

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

You can delete this import and the "TODO" comment above, and call backend_util.assertAxesAreInnerMostDims and backend_util.computeOutAndReduceShapes - they are already exported as part of backend_util.

annxingyuan
annxingyuan reviewed Dec 26, 2019
View reviewed changes
annxingyuan
annxingyuan reviewed Dec 26, 2019
View reviewed changes
annxingyuan
annxingyuan reviewed Dec 26, 2019
View reviewed changes
annxingyuan
annxingyuan suggested changes Dec 26, 2019
View reviewed changes
Copy link
Contributor

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

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

Thank you @NALLEIN ! I just left a few more tiny comments - sorry to be tedious but I just want to make sure we minimize imports from non-public API whenever possible. Very excited to merge this soon!

annxingyuan
annxingyuan reviewed Dec 30, 2019
View reviewed changes
tfjs-core/src/index.ts Outdated
export {Scalar, Tensor, Tensor1D, Tensor2D, Tensor3D, Tensor4D, Tensor5D, TensorBuffer, Variable} from './tensor';
export {GradSaveFunc, NamedTensorMap, TensorContainer, TensorContainerArray, TensorContainerObject} from './tensor_types';
export {DataType, DataTypeMap, DataValues, Rank, RecursiveArray, ShapeMap, TensorLike} from './types';
// TODO : import sumOutType directly from '@tensorflow/tfjs-core'.
Copy link
Contributor

@annxingyuan annxingyuan Dec 30, 2019

Choose a reason for hiding this comment

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

Remove this TODO statement and move to backend_webgpu.ts:28 instead.

annxingyuan
annxingyuan reviewed Dec 30, 2019
View reviewed changes
tfjs-core/src/backends/backend_util.ts Outdated
export * from '../ops/conv_util';
export {Activation, FusedConv2DConfig} from '../ops/fused_util';

// TODO : use backend_util.reduce_util with the next release of tfjs-core.
Copy link
Contributor

@annxingyuan annxingyuan Dec 30, 2019

Choose a reason for hiding this comment

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

Move this TODO statement to backend_webgpu.ts:27 instead.

@annxingyuan annxingyuan merged commit b8caa14 into tensorflow:master Dec 30, 2019
@qjia7
Copy link
Contributor

qjia7 commented Dec 31, 2019

FYI, with this change, I met below errors. Do you need to update the tfjs-core version?

Uncaught Error: Can't find @tensorflow/tfjs-core/src/ops/reduce_util [/Users/wp/workspace/jiajia/tfjs/tfjs-backend-webgpu/node_modules/@tensorflow/tfjs-core/src/ops/reduce_util.ts] (required by /Users/wp/workspace/jiajia/tfjs/tfjs-backend-webgpu/src/backend_webgpu.ts)
    at require (commonjs.js:13)
    at commonjs.js:18
    at Object.global.wrappers./Users/wp/workspace/jiajia/tfjs/tfjs-backend-webgpu/src/backend_webgpu.ts../flags_webgpu (backend_webgpu.ts:28)
    at require (commonjs.js:17)
    at commonjs.js:18
    at Object.global.wrappers./Users/wp/workspace/jiajia/tfjs/tfjs-backend-webgpu/src/backend_webgpu_test.ts.@tensorflow/tfjs-core (backend_webgpu_test.ts:21)
    at require (commonjs.js:17)
    at commonjs.js:38
    at Array.forEach (<anonymous>)
    at commonjs.js:37
    at commonjs.js:40

@annxingyuan
Copy link
Contributor

annxingyuan commented Dec 31, 2019

@qjia7 yes - thank you - I'm seeing those too. I have a pull request in progress (#2629) that links WebGPU to tfjs-core@master so we can avoid these issues in the future.

dsmilkov
dsmilkov reviewed Jan 9, 2020
View reviewed changes
tfjs-core/src/index.ts
export {Scalar, Tensor, Tensor1D, Tensor2D, Tensor3D, Tensor4D, Tensor5D, TensorBuffer, Variable} from './tensor';
export {GradSaveFunc, NamedTensorMap, TensorContainer, TensorContainerArray, TensorContainerObject} from './tensor_types';
export {DataType, DataTypeMap, DataValues, Rank, RecursiveArray, ShapeMap, TensorLike} from './types';
export {DataType, DataTypeMap, DataValues, Rank, RecursiveArray, ShapeMap, sumOutType, TensorLike} from './types';
Copy link
Contributor

@dsmilkov dsmilkov Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

this introduces a top-level tf.sumOutType method, which we don't want as part of our API since we are aligning with TF Python. All the other exports from './types' are interfaces/types that do not exist at runtime. We should instead expose sumOutType as part of the backend_util namespace.

@NALLEIN NALLEIN deleted the add_reduce branch April 15, 2020 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@dsmilkov dsmilkov dsmilkov left review comments

@annxingyuan annxingyuan annxingyuan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@NALLEIN @qjia7 @annxingyuan @dsmilkov @googlebot