Skip to content

Conversation

@etiennedupont
Copy link
Contributor

@etiennedupont etiennedupont commented Feb 16, 2020

Fixes #2761

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@etiennedupont
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Hi @etiennedupont , package @tensorflow/tfjs is a union package, which includes tfjs-core. So no need to include it again.

@etiennedupont
Copy link
Contributor Author

Hi @lina128,
as discussed in #2761, this way of making transitive dependencies not only makes it impossible to use tfjs-node with Yarn pnp v1, but is also prone to bugs (see #2761 (comment)). Including this additional line in the package.json would not mean to include the package again but to build correct hoistings.

@lina128 lina128 requested a review from nsthorat February 28, 2020 18:19
Copy link
Contributor

@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 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @nsthorat)

@nsthorat
Copy link
Contributor

The CI is failing because we have a little script that makes sure tfjs-node and tfjs-node-gpu package.json line up, can you make the change there as well?

https://github.com/tensorflow/tfjs/blob/master/tfjs-node-gpu/package.json#L57

Thanks so much! This change is great :)

@etiennedupont
Copy link
Contributor Author

Hi @nsthorat, thank you for your answer, I made the required change!

@lina128 lina128 merged commit 1a94219 into tensorflow:master Feb 29, 2020
@lina128 lina128 mentioned this pull request Mar 28, 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.

Explicitely make tfjs-core a dependency of tfjs-node

4 participants