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

How to support tensorflow JS types? #334

Open
MorganR opened this issue Sep 3, 2020 · 9 comments
Open

How to support tensorflow JS types? #334

MorganR opened this issue Sep 3, 2020 · 9 comments
Labels

Comments

@MorganR
Copy link

MorganR commented Sep 3, 2020

Hello, I'm trying to use TSCC to compile typescript code that depends on Tensorflow JS. TFJS has nice type declaration files in its NPM package, and these are structured like this:

node_modules/
  @tensorflow/
    tfjs/dist/index.d.ts    <-- This exports all the symbols from the other directories
    tfjs-backend-webgl/dist/*.d.ts
    tfjs-core/dist/*.d.ts
    ...

To use this in my code, I have an import: import * as tf from '@tensorflow/tfjs';

My tsconfig.json includes all the typescript .d.ts files, eg:

"compilerOptions": {
  "moduleResolution": "node",
  ...
},
"include": [
  "myscripts/**/*.ts",
  "node_modules/@tensorflow/tfjs/dist/*.d.ts",
  "node_modules/@tensorflow/tfjs-backend-webgl/dist/*.d.ts",
  ...
]

My tscc.spec.json looks like this:

{
  "modules": {
    "out": {
      "entry": "myscripts/app.ts"
    }
  },
  "external": {
    "@tensorflow/tfjs": "tf"
  }
}

However, when I try to compile this with TSCC, I get this error: Module name node_modules._tensorflow.tfjs_core.dist.base.d was not provided as a closure compilation source

I've tried enabling persistArtifacts to look at the intermediate output, and the only tensorflow output I see is node_modules/@tensorflow/tfjs/dist/index.js, which just contains:

goog.module('@tensorflow/tfjs')
/** Generated by TSCC */
exports = tf;

Shouldn't there be an externs.js file somewhere with the type information? How can I resolve this issue?

@theseanl
Copy link
Owner

theseanl commented Sep 6, 2020

Could you provide steps to reproduce? I tried to import @tensorflowjs/tfjs but due to a type conflict of WebGL2RenderingContext in @types/webgl2 and lib.dom.d.ts, even the tsc command fails.

@theseanl
Copy link
Owner

theseanl commented Sep 6, 2020

I can now reproduce the issue with skipLibCheck: true as suggested in tensorflow/tfjs#2007. Is this your setup?

It seems that this option is problematic, no externs are generated with it. There may be other issues coming into play here, such as the one regarding external module referencing other npm packages for type declaration. However, in order to investigate it, we first need a setup that works with tsc without skipLibCheck: true hack.

@theseanl theseanl added the bug Something isn't working label Sep 7, 2020
@theseanl
Copy link
Owner

Update on this: as I understand solving the following problems consists of resolving the issue.

  1. When an external module's type declaration file merely re-export something from yet another external module, tsickle sometimes may emit goog.requireType(..) pointing directly to certain file in that module. To address this, I can re-use an approach using typescript transformers which is how external module support was implemented before switching to gluing modules.
  2. In addition, we have to include every .d.ts file that may appear in the transpilation result of source code (every .ts files that is not .d.ts) to externs generation. Previously, only those of external modules designated in the spec files were included. This is required as e.g. @tensorflowjs/tfjs's type depends on @tensorflowjs/tfjs-core and so on.
  • An easy way(for me) would be to require users to provide every such package's name: in this case, the spec file may look like
"external": {
      "@tensorflow/tfjs": "tf",
      "@tensorflow/tfjs-backend-cpu": "tf_backend_cpu",
      "@tensorflow/tfjs-backend-webgl": "tf_backend_webgl",
      "@tensorflow/tfjs-converter": "tf_converter",
      "@tensorflow/tfjs-core": "tfjs_core",
      "@tensorflow/tfjs-data": "tfjs_data",
      "@tensorflow/tfjs-layers": "tsjf_layers"
}

which is tedious and counterintuitive.

  • At the other extreme, we can include every .d.ts that typescript "sees". This will lead to the most correct behavior in some sense, because this is something you see in your IDE. But this may potentially lead to enormous amount of externs file and slow down the compilation as it will include everything in node_modules/@types directory unless you use types[root] compiler option. This may also cause more bugs coming from incompatibility between typescript and the closure side.

So I am looking for a solution in between two of these; I'm currently researching if it is possible to get package/file lists which depends on certain package's type declaration file via typescript API.

@theseanl
Copy link
Owner

Note: Typescript 4.1 may introduce some option which may significantly reduce the issue and may render above consideration unnecessary.
microsoft/TypeScript#40124 - Investigate Preventing Global Conflicts with Strict Environment Configuration

@MorganR
Copy link
Author

MorganR commented Sep 15, 2020

Sorry for the delay! Re:

I can now reproduce the issue with skipLibCheck: true as suggested in tensorflow/tfjs#2007. Is this your setup?

It's also possible to compile things without skipLibCheck. I've made a sample program here: https://github.com/MorganR/tensorflow-tscc

The typescript can be correctly compiled, but TSCC still emits the error: Module name node_modules._tensorflow.tfjs_core.dist.tensor.d was not provided as a closure compilation source

In the meantime, I have managed to get my app working using regular TSC + Rollup + Terser, but I'd love to get the closure compiler working at some point!

theseanl added a commit that referenced this issue Sep 17, 2020
to determine a set of files to pass to tsickle. Also implements a
tranasformer that rewrites goog.requireType calls to module-dts to
global variable aliases. For more information: see the respective
file comments. This partly addresses
#334.
@theseanl
Copy link
Owner

theseanl commented Sep 28, 2020

Hi, finally the version which compiles with tensorflowjs is published. Please try with @tscc/tscc 0.6.0!

I've noticed that your repro repo https://github.com/MorganR/tensorflow-tscc contains an invalid closure compiler option "target", you can replace it to "language_out" which is a closure compiler flag for output JS version.

@MorganR
Copy link
Author

MorganR commented Sep 29, 2020

This is awesome! Thank you so much. I can definitely get some things working now.

Two issues I'm still running into:

  1. Anything below the top tf level is inaccessible once compiled. For example, tf.browser.fromPixels(). The browser part gets mangled during compilation, which breaks the call. In the .d.ts files, these are connected as follows:

    • tfjs/dist/index.d.ts:

      ...
      export * from '@tensorflow/tfjs-core';
      ...
    • tfjs-core/dist/index.d.ts:

      ...
      export * from './base';
      ...
    • tfjs-core/dist/base.d.ts:

      ...
      import * as browser from './ops/browser';
      ...
      export { browser, ... };
      ...
    • tfjs-core/dist/ops/browser.d.ts:

      ...
      declare function fromPixels_(...): Tensor3D;
      
      export declare function toPixels(...): Promise<Uint8ClampedArray>;
      export declare const fromPixels: typeof fromPixels_;
      export {};

      Is it possible to fix this via the config or within TSCC? Or will this be fundamentally incompatible?

  2. I do see this warning during compilation (doesn't seem to break anything, but just FYI in case it's a bug): externs_generated.js:2638:91: WARNING - [JSC_TYPE_PARSE_ERROR] Bad type annotation. expected closing } See https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler for more information. 2638| * @implements {node_modules.$001stensorflow.tfjs$0019core.dist.tensor$.d$.ts.TensorTracker|node_modules.$001stensorflow.tfjs$0019core.dist.backends.backend$.d$.ts.DataMover}

    I noticed the surrounding lines in the externs_generated.js use $ instead of . in most of the type prefix (eg. node_modules$$001stensorflow$tfjs$ instead of node_modules.$001stensorflow.tfjs$), in case that's related.

@theseanl theseanl reopened this Sep 29, 2020
@theseanl
Copy link
Owner

Thank you for your follow ups. I'm aware of the issue 2, I think it is something to be fixed from tsickle. Despite of this issue, as I understand closure compiler mostly backs off for property names appearing in externs, so it is mostly fine. That said, I've tracked down a cause and will file an issue report in the tsickle repository.

Regarding the first issue, it is another issue with tsickle's externs generation. For example currently it does not generate externs for export * from "..." statement, which shows up several times in tensorflowjs's declaration files. However this browser specific issue seems to be yet another issue. So what I can tell is that compiling tensorflowjs with tscc will only be safe after at least a next version of tsickle is released.

@theseanl
Copy link
Owner

Relevant issues: angular/tsickle#1202, angular/tsickle#1203

@theseanl theseanl added external tsickle and removed bug Something isn't working external labels Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants