Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Add exports to remove importing from dist in other packages. #1856

Merged
merged 9 commits into from
Aug 5, 2019

Conversation

WenheLI
Copy link
Contributor

@WenheLI WenheLI commented Jul 24, 2019

This pr is to fix dist importing happens in tfjs-node.
A corresponding pr is here, tensorflow/tfjs-node#289


This change is Reviewable

@WenheLI WenheLI changed the title Fix lint Fix export Jul 25, 2019
Copy link
Contributor

@nsthorat nsthorat 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: 0 of 1 approvals obtained (waiting on @WenheLI)


src/index.ts, line 61 at r1 (raw file):

export * from './ops/ops';
export {Conv2DInfo, Conv3DInfo} from './ops/conv_util';

these should already be available under backend_util

Copy link
Contributor Author

@WenheLI WenheLI 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: 0 of 1 approvals obtained (waiting on @nsthorat)


src/index.ts, line 61 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

these should already be available under backend_util

It's weird though I can not have them under backend_util. Below is the screenshot of what I got by logging the backend_util. This might be related that they are actually type not function?
screen.png

Copy link
Contributor

@nsthorat nsthorat 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: 0 of 1 approvals obtained (waiting on @WenheLI)


src/index.ts, line 61 at r1 (raw file):

Previously, WenheLI wrote…

It's weird though I can not have them under backend_util. Below is the screenshot of what I got by logging the backend_util. This might be related that they are actually type not function?
screen.png

You can't console.log() some of these because they are types, try looking in vscode.

Copy link
Contributor Author

@WenheLI WenheLI 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: 0 of 1 approvals obtained (waiting on @nsthorat)


src/index.ts, line 61 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

You can't console.log() some of these because they are types, try looking in vscode.

Thx! I have removed this export.

Copy link
Contributor

@nsthorat nsthorat 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: 0 of 1 approvals obtained (waiting on @nsthorat and @WenheLI)


src/index.ts, line 61 at r2 (raw file):

export * from './ops/ops';
export {Activation} from './ops/fused_util';

lets export this in backend_util as well, you can reexport from that file


src/index.ts, line 73 at r2 (raw file):

export {Platform} from './platforms/platform';

export {EPSILON_FLOAT32} from './backends/backend';

this should also probably be in backend_util -- but just curious why you need this?

Copy link
Contributor Author

@WenheLI WenheLI 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: 0 of 1 approvals obtained (waiting on @nsthorat)


src/index.ts, line 73 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this should also probably be in backend_util -- but just curious why you need this?

This one is not in the backend_util, I can put it in.
And it was used in https://github.com/tensorflow/tfjs-node/blob/11014ab624b07ce1bb107955d9877dbb356cf92e/src/nodejs_kernel_backend.ts#L20

Copy link
Contributor

@nsthorat nsthorat 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: 0 of 1 approvals obtained (waiting on @nsthorat and @WenheLI)


src/index.ts, line 73 at r2 (raw file):

Previously, WenheLI wrote…

This one is not in the backend_util, I can put it in.
And it was used in https://github.com/tensorflow/tfjs-node/blob/11014ab624b07ce1bb107955d9877dbb356cf92e/src/nodejs_kernel_backend.ts#L20

Ah got it -- in that case can we just duplicate the constant in tfjs-node instead of exporting it here? It will never change

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @WenheLI)


src/index.ts, line 58 at r3 (raw file):

export {Scalar, Tensor, Tensor1D, Tensor2D, Tensor3D, Tensor4D, Tensor5D, TensorBuffer, variable, Variable} from './tensor';
export {GradSaveFunc, NamedTensorMap, TensorContainer, TensorContainerArray, TensorContainerObject} from './tensor_types';
export {BackendValues, DataType, DataTypeMap, DataValues, Rank, ShapeMap, TensorLike} from './types';

I don't think you need this in tfjs-node, it seems unused

@nsthorat
Copy link
Contributor

Make sure you get the build to pass -- can you see the GCP logs?

Copy link
Contributor Author

@WenheLI WenheLI left a comment

Choose a reason for hiding this comment

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

This build should not pass in theory. Since we made changes at tfjs-core which is depended by tfjs-node, our tfjs-core test requires to do integration test over tfjs-node as well. Thus, until the changes for tfjs-node are merged, we can not have the test past. This also happens to tfjs-node.
Am I thinking it in the wrong direction here?

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


src/index.ts, line 58 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I don't think you need this in tfjs-node, it seems unused

This was imported here https://github.com/WenheLI/tfjs-node/blob/9254cc253610dbdfa8d9088e74b41c500454654e/src/nodejs_kernel_backend.ts#L22

Copy link
Contributor

@nsthorat nsthorat 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 @WenheLI)


src/index.ts, line 58 at r3 (raw file):

Previously, WenheLI wrote…

This was imported here https://github.com/WenheLI/tfjs-node/blob/9254cc253610dbdfa8d9088e74b41c500454654e/src/nodejs_kernel_backend.ts#L22

Ah thanks. I think this should also be exported in backend_util

Copy link
Contributor Author

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


src/index.ts, line 58 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Ah thanks. I think this should also be exported in backend_util

Added it to backend_util. Hope everything is fine now.

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nsthorat nsthorat changed the title Fix export Add exports to remove importing from dist in other packages. Aug 5, 2019
@nsthorat nsthorat merged commit eb2e989 into tensorflow:master Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants