Skip to content

Conversation

@lina128
Copy link
Collaborator

@lina128 lina128 commented Mar 27, 2020

This PR fixes issues found during release. Note, the issues were found right at the spot and were fixed right away. The release versions were correctly published, so the fixes in this PR were tested in the release of 1.7.1.

This PR has below changes:

  1. Update dependency list, because Layers is recently added as a dependency for Node: https://github.com/tensorflow/tfjs/blob/master/tfjs-node/package.json#L50

  2. node-gpu needs an additional step to build.

  3. wasm needs an additional step to build.

  4. The publish script has to use this command to run YARN_REGISTRY="https://registry.npmjs.org/" yarn publish-npm. The previous try that adds npm registry information in the last step works on my macbook but not on workstation. On workstation, it is still using yarn registry. Above command works on both macbook and workstation.


This change is Reviewable

@lina128 lina128 requested review from dsmilkov and nsthorat March 27, 2020 19:52
@lina128 lina128 changed the title Release Fix release script. Mar 27, 2020
@lina128 lina128 requested review from kangyizhang and pyu10055 March 28, 2020 01:05
Copy link
Contributor

@kangyizhang kangyizhang left a comment

Choose a reason for hiding this comment

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

Thank you Na for putting this together which makes the release process much more smooth!

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @kangyizhang, @lina128, @nsthorat, and @pyu10055)


scripts/release-util.ts, line 51 at r1 (raw file):

export const NODE_PHASE: Phase = {
  packages: ['tfjs-node', 'tfjs-node-gpu'],
  deps: ['tfjs', 'tfjs-core', 'tfjs-layers'],

I saw that tfjs-layers is added as devDependency in https://github.com/tensorflow/tfjs/pull/2854/files#diff-48e8607fcaefdc233be729e07c18f766R48. Can you provide some context why we did this? tfjs-node depends on the tfjs union package, so it should already have tfjs-layers and tfjs-core installed as dependency.

@lina128
Copy link
Collaborator Author

lina128 commented Mar 28, 2020

Thank you Na for putting this together which makes the release process much more smooth!

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @kangyizhang, @lina128, @nsthorat, and @pyu10055)

scripts/release-util.ts, line 51 at r1 (raw file):

export const NODE_PHASE: Phase = {
  packages: ['tfjs-node', 'tfjs-node-gpu'],
  deps: ['tfjs', 'tfjs-core', 'tfjs-layers'],

I saw that tfjs-layers is added as devDependency in https://github.com/tensorflow/tfjs/pull/2854/files#diff-48e8607fcaefdc233be729e07c18f766R48. Can you provide some context why we did this? tfjs-node depends on the tfjs union package, so it should already have tfjs-layers and tfjs-core installed as dependency.

Hi Kangyi, thanks for the review. We added tfjs-core and tfjs-layers because they were directly used in the code instead of through tfjs, see https://github.com/tensorflow/tfjs/blob/master/tfjs-node/src/io/file_system_test.ts#L19-L20. If they are directly used, they need to be in the dependency list, see this discussion: #2762 and #2761

Copy link
Contributor

@dsmilkov dsmilkov 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 @dsmilkov, @kangyizhang, @nsthorat, and @pyu10055)


scripts/release-util.ts, line 51 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Hi Kangyi, thanks for the review. We added tfjs-core and tfjs-layers because they were directly used in the code instead of through tfjs, see https://github.com/tensorflow/tfjs/blob/master/tfjs-node/src/io/file_system_test.ts#L19-L20. If they are directly used, they need to be in the dependency list, see this discussion: #2762 and #2761

Na, let's add what you just said in a comment above the deps line. Say something like: "Node has unit tests that use tf.layers to test serialization of models in node."

Something to think about for later: Ideally that unit test should live in layers, because it depends on platform specific stuff (node writing files to disk), nothing fundamental to the node backend.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

LGTM, with 1 comment to follow up.

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @kangyizhang, @nsthorat, and @pyu10055)

@lina128
Copy link
Collaborator Author

lina128 commented Mar 30, 2020

Node has unit tests that use tf.layers to test serialization of models in node.

Done. Thank you for review, Daniel!

@lina128 lina128 requested a review from kangyizhang March 30, 2020 17:03
Copy link
Contributor

@kangyizhang kangyizhang 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! 2 of 1 approvals obtained (waiting on @dsmilkov, @kangyizhang, @lina128, @nsthorat, and @pyu10055)


scripts/release-util.ts, line 51 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Na, let's add what you just said in a comment above the deps line. Say something like: "Node has unit tests that use tf.layers to test serialization of models in node."

Something to think about for later: Ideally that unit test should live in layers, because it depends on platform specific stuff (node writing files to disk), nothing fundamental to the node backend.

Thank you Na for the explanation!

@lina128 lina128 merged commit 1ad92e9 into tensorflow:master Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants