Skip to content
This repository has been archived by the owner on Sep 17, 2022. It is now read-only.

Remove dist importing #289

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Remove dist importing #289

wants to merge 10 commits into from

Conversation

WenheLI
Copy link

@WenheLI WenheLI commented Jul 24, 2019

This pr is to remove dist importing and add no-import-from-dist rule.


This change is Reviewable

Copy link

@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/nodejs_kernel_backend_test.ts, line 20 at r1 (raw file):

import * as tf from '@tensorflow/tfjs-core';
import {Tensor5D} from '@tensorflow/tfjs-core';
// tslint:disable-next-line:max-line-length

you c an remove this comment now


src/nodejs_kernel_backend_test.ts, line 23 at r1 (raw file):

import {test_util} from '@tensorflow/tfjs-core';
import {NodeJSKernelBackend} from './nodejs_kernel_backend';
const {expectArraysClose} = test_util;

lets just call test_util.expectArraysClose instead of doing this unpacking


src/io/file_system_test.ts, line 28 at r1 (raw file):

import {NodeFileSystem, nodeFileSystemRouter} from './file_system';

const {expectArraysClose} = test_util;

same here lets use test_util.expectArraysClose below

Copy link

@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 6 of 8 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@nsthorat
Copy link

Ah looks like those exports don't actually exist... cc @annxingyuan did we start exporting these for WebGPU?

@WenheLI
Copy link
Author

WenheLI commented Jul 25, 2019

@nsthorat There is a corresponding PR for the extra exports.
tensorflow/tfjs-core#1856

Copy link
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/nodejs_kernel_backend.ts, line 31 at r4 (raw file):

import {TensorMetadata, TFEOpAttr, TFJSBinding} from './tfjs_binding';

type Conv2DInfo = backend_util.Conv2DInfo;

I added this line here rather than adding to each use of Conv2DInfo is to avoid an exceeding of max line length.

nsthorat pushed a commit to tensorflow/tfjs-core that referenced this pull request Aug 5, 2019
DEV

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

Thanks for this PR! We're waiting on 1.2.8 @tensorflow/tfjs to be published before we can merge this.

@nsthorat
Copy link

Let's reopen this PR on the monorepo once we move tfjs-node there because we'll have a top-level tslint folder which we can share among every package.

1.2.8 is also a large change for tfjs-node which will take some time for us to do.

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.

3 participants