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

Tensorflow NPM Packages are shipping TypeScript sources instead of just the distrubtion files [URGENT] #3617

Closed
jbis9051 opened this issue Jul 17, 2020 · 11 comments · Fixed by #3653
Assignees
Labels
type:bug Something isn't working type:build/install

Comments

@jbis9051
Copy link

jbis9051 commented Jul 17, 2020

This is in my package.json

    "@tensorflow/tfjs": "^2.0.1",

I'm getting this error:

/path/to/project/node_modules/@tensorflow/tfjs-core/src/environment.ts
TypeScript error in /path/to/project/node_modules/@tensorflow/tfjs-core/src/environment.ts(45,3):
Property 'platformName' has no initializer and is not definitely assigned in the constructor.  TS2564

    43 |   private urlFlags: Flags = {};
    44 | 
  > 45 |   platformName: string;
       |   ^
    46 |   platform: Platform;
    47 | 
    48 |   // tslint:disable-next-line: no-any

I also tried "skipLibCheck": true in tsconfig.json but that didn't work either.

In the case of tfjs, there are actual .ts files (e.g. node_modules/@tensorflow/tfjs-core/src/environment.ts) which cause the failure.

Generally npm modules should ship compiled files and their public APIs, so anything happening internally (in js files) should not be reported.

Related https://www.reddit.com/r/typescript/comments/hjgd30/skiplibcheck_but_for_ts_files/

@jbis9051 jbis9051 changed the title Property 'platformName' has no initializer and is not definitely assigned in the constructor. Tensorflow NPM Packages are shipping TypeScript sources instead of just the distrubtion files Jul 17, 2020
@jbis9051 jbis9051 changed the title Tensorflow NPM Packages are shipping TypeScript sources instead of just the distrubtion files Tensorflow NPM Packages are shipping TypeScript sources instead of just the distrubtion files [URGENT] Jul 17, 2020
@rthadur
Copy link
Contributor

rthadur commented Jul 18, 2020

@jbis9051 can you please provide steps to reproduce above error ?

@rthadur rthadur self-assigned this Jul 18, 2020
@jbis9051
Copy link
Author

jbis9051 commented Jul 18, 2020

@rthadur The linked reddit post describes why it's an issue better.

Reproduce

$ mkdir test
$ cd test
$ npm init
$ npm install --save @tensorflow/tfjs

In test/node_modules/@tensorflow there is a bunch of packages:

.
├── tfjs
├── tfjs-backend-cpu
├── tfjs-backend-webgl
├── tfjs-converter
├── tfjs-core
├── tfjs-data
└── tfjs-layers

Inside all of these packages is something similar to the following:

├── tfjs-core
│   ├── BUILD.bazel
│   ├── README.md
│   ├── cloudbuild.yml
│   ├── development
│   ├── dist
│   ├── package.json
│   ├── scripts
│   ├── src
│   ├── test.html
│   └── tsconfig.test.json

Instead, only the dist folder should be there.

Why this is an issue?

For starters, including the source files (used for developing) increases package sizes tremendously and unnecessarily.

Additionally, and more importantly, if you are using typescript in a project TypeScript will do type checks on the tensorflow source despite excluding node_modules and "skipLibCheck": true being included in your own tsconfig.json. Since most typescript configs don't line up with tensorflow's typescript config, the compilation will fail. The only workaround is to manually delete the src folders in every folder in @tensorflow/.

How do you guys fix it?

For TypeScript based packages you should only publish the built (dist) files with the deceleration [*.d.ts] files. This will allow "skipLibCheck": true to work properly.

If you look at bodyPix, that package is setup properly:

test/node_modules/@tensorflow-models
└── body-pix
    ├── README-v1.md
    ├── README.md
    ├── cloudbuild.yml
    ├── diff
    ├── dist
    ├── package.json
    └── run_tests.ts

Despite BodyPix having a src directory (https://github.com/tensorflow/tfjs-models/tree/master/body-pix/src), only the dist folder is published and therefore installed, not the src folder.

If we go into the dist folder then we can see all the compiled source and deceleration files.

@dosomder
Copy link

Same issue here

@rthadur rthadur assigned tylerzhu-github and unassigned rthadur Jul 20, 2020
@rthadur
Copy link
Contributor

rthadur commented Jul 20, 2020

@tylerzhu-github can you please help with above request ?

@TylerShin
Copy link

same here

@tafsiri
Copy link
Contributor

tafsiri commented Jul 21, 2020

@jbis9051 What version of typescript are you using? Could you post your tsconfig (or a similar tsconfig that reproduces the error), we've been shipping those up to NPM for a while and it would be nice to know why skipLibCheck/exclude does not work in this instance.

@tafsiri tafsiri assigned tafsiri and unassigned tylerzhu-github Jul 21, 2020
@tafsiri tafsiri removed the bodyPix label Jul 21, 2020
@jbis9051
Copy link
Author

jbis9051 commented Jul 21, 2020

@tafsiri Sure. skipLibCheck doesn't work because, again, those are source Typescript files (*.ts) not declaration files (*.d.ts). See here https://www.typescriptlang.org/docs/handbook/compiler-options.html

--skipLibCheck | boolean | false | Skip type checking of all declaration files (*.d.ts).

Not sure about why exclude doesn't work.

Version: "typescript": { "version": "3.7.5",

Here's a TSConfig:

{
  "compilerOptions": {
    "target": "es5",
    "lib": [
      "dom",
      "dom.iterable",
      "esnext"
    ],
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react",
    "experimentalDecorators": true
  },
  "include": [
    "src"
  ],
  "exclude": [
    "node_modules"
  ]
}

It isn't compatible because "strict": true, isn't compatible with tensorflow's tsconfig.

@tafsiri tafsiri added the type:bug Something isn't working label Jul 24, 2020
tafsiri added a commit that referenced this issue Jul 24, 2020
Also add a custom lint rule to disable such imports.
We should be able to import from a relative within the src folder of a
given folder. Not doing so changes the resolution path  for typescript
users in the published packaged.

fixes #3617
@tafsiri
Copy link
Contributor

tafsiri commented Jul 24, 2020

Found the bug, it was an errant import path in our source. Should be resolved in the next release goes out.

@jbis9051
Copy link
Author

\o/

@forivall
Copy link

Yeah, i independently found the problem (using tsc --traceResolution), and my workaround right now is to use https://npm.im/patch-package with the following patch:

patches/@tensorflow+tfjs-core+2.0.1.patch

diff --git a/node_modules/@tensorflow/tfjs-core/dist/kernel_names.d.ts b/node_modules/@tensorflow/tfjs-core/dist/kernel_names.d.ts
index 126a329..b287921 100644
--- a/node_modules/@tensorflow/tfjs-core/dist/kernel_names.d.ts
+++ b/node_modules/@tensorflow/tfjs-core/dist/kernel_names.d.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  * =============================================================================
  */
-import { ExplicitPadding } from '../src/ops/conv_util';
+import { ExplicitPadding } from './ops/conv_util';
 import { NamedTensorInfoMap, TensorInfo } from './kernel_registry';
 import { DataType, PixelData } from './types';
 export declare const Add = "Add";
diff --git a/node_modules/@tensorflow/tfjs-core/src/kernel_names.ts b/node_modules/@tensorflow/tfjs-core/src/kernel_names.ts
index 4e9a822..c998fdb 100644
--- a/node_modules/@tensorflow/tfjs-core/src/kernel_names.ts
+++ b/node_modules/@tensorflow/tfjs-core/src/kernel_names.ts
@@ -18,7 +18,7 @@
 // tslint:disable: variable-name
 // Unfortunately just enabling PascalCase per file (tslint:enable:
 // allow-pascal-case) doesn't work.
-import {ExplicitPadding} from '../src/ops/conv_util';
+import {ExplicitPadding} from './ops/conv_util';
 
 import {NamedTensorInfoMap, TensorInfo} from './kernel_registry';
 import {DataType, PixelData} from './types';

@google-ml-butler
Copy link

Are you satisfied with the resolution of your issue?
Yes
No

tafsiri added a commit that referenced this issue Jul 27, 2020
BUG

Also adds a custom lint rule to disable such imports.

Fixes #3617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working type:build/install
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants