-
Notifications
You must be signed in to change notification settings - Fork 2k
[node]Test against head. #2854
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
[node]Test against head. #2854
Conversation
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 10 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @lina128, and @nsthorat)
tfjs-converter/tsconfig.json, line 26 at r1 (raw file):
}, "include": [ "src/"
this means that scripts/**.ts are not going to be compiled anymore (you can introduce a compile error and it won't be caught). If not possible to keep here, can you introduce another tsconfig in scripts and make sure we build them on ci
|
One comment about scripts. Everything else looks great |
Hi Daniel, Besides gen-json, other scripts are exactly same as tfjs-layers and tfjs-data, which don't have scripts in the their tsconfig, and have been working fine. Given that scripts are not public APIs and not part of package(?), as long as they are compiled and executed, I think we can live without tsconfig for simplicity. WDYT? |
dsmilkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @nsthorat)
tfjs-converter/tsconfig.json, line 26 at r1 (raw file):
Previously, lina128 (Na Li) wrote…
Hi Daniel, I think it will still get compiled, the tsconfig is inferred in this case, see this reply for example: microsoft/vscode#17480 (comment) Another observation, under scripts folder, gen-json.ts is called in CI: https://github.com/tensorflow/tfjs/blob/master/tfjs-converter/cloudbuild.yml#L38 and it works fine for this PR, as shown here: https://pantheon.corp.google.com/cloud-build/builds/87bee5cc-452e-4d70-b629-9d7db9a58a0d;step=4?project=learnjs-174218
Besides gen-json, other scripts are exactly same as tfjs-layers and tfjs-data, which don't have scripts in the their tsconfig, and have been working fine.
Given that scripts are not public APIs and not part of package(?), as long as they are compiled and executed, I think we can live without tsconfig for simplicity. WDYT?
That makes sense. Thanks for checking that they get compiled and executed on CI. LGTM!
Add necessary commands for linking and building dependency packages.
p.s. it explicitly build dependencies including the nested dependencies. This is because, if we rely on transitive dependency building, which means the union package, tfjs's, build-ci has to include build-layers-ci, build-data-ci, etc, which will result in these packages built twice, one from cloudbuild.yml, and one from build. This adds too much hidden processes and is error-prone. To make things simple, packages that have nested dependencies, currently only tfjs-node, should take care of building all the dependencies it needs. This is the same behavior if we use yalc link/publish, which doesn't take care of nested dependency either.
p.p.s. Need to remove 'scripts' from tfjs-converter's tsconfig file, and also change the main entry and type in the package.json file. Otherwise, typescript can't find modules symlinked. This issue is reported in typescript's repo, solution is to change from dist/src/index.js to dist/index.js: microsoft/TypeScript#23502
We shouldn't be too worried, because after the change, tfjs-converter's tsconfig and main/type entry will be exactly same as data and layers, which should have been the same in the first place.
This change is