Update build process: local dependencies, build in correct order

[davidsoergel]

There may be a lot to discuss here. This works great for me locally, and we'll see if it's OK on Travis.

• Bump all version numbers to (n+1)-dev to reflect the fact that new commits are not included in the already-published versions.
• Use 'yarn link' to let packages depend on local WIP in other packages.
• Set the dependency version to 'latest' for such local packages.
• Merge models/util.ts into src/util.ts and remove models/package.json.
• Rewrite build_and_lint_all as a simpler bash script that guarantees build order, so that the latest versions of local dependencies are used.
• Fix a couple lint errors.
• Add a couple .gitignore rules.

One thing I don't like about this, but seems unavoidable, is that 'yarn link' doesn't work if we bump the dependency version number (See yarnpkg/yarn#2611). E.g., since the models/* packages now require the util.ts update in this PR, they can't depend on DLJS 0.5.0. I wanted to make them depend on ~0.5.1-dev and rely on 'yarn link' to resolve that version-- yet 'yarn' still complains that the version doesn't exist because it's not published to NPM. That's one reason to just depend on 'latest' for packages within the same repo. The actual version should get pinned in yarn.lock at the time of publication anyway.

---

This change is 

[nsthorat]

High level question - what's the motivation for this change? Is there some error you were getting?

A few thoughts:

• npm link registers the current package as the global 'deeplearn'. I don't think we should be mucking global NPM state, which is why we used file everywhere to keep it localized. Can you explain why you're suggesting we switch to that?
• Why the multiple prep scripts?

---

Review status: 0 of 25 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

.gitignore, line 13 at r1 (raw file):

dist/
.idea/
.vscode/

we shouldn't be ignoring .vscode - this is checked in so we share vscode settings.

---

package.json, line 3 at r1 (raw file):

{
  "name": "deeplearn",
  "version": "0.5.1-dev",

why -dev? This will mess up our versioning scripts.

---

demos/game_of_life/game_of_life.ts, line 82 at r1 (raw file):

  // TODO(kreeger, #409): Drop this when math.pad() is ready.
  private static padArray(array: dl.Tensor): dl.Tensor2D {
    const x1: number = array.shape[0];

you shouldn't need this, array.shape is number[]

---

models/package.json, line 1 at r1 (raw file):

{

why are you removing this? the other packages extend from this.

---

models/knn_image_classifier/package.json, line 9 at r1 (raw file):

  "types": "dist/knn_image_classifier/index.d.ts",
  "dependencies": {
    "deeplearn": "latest",

I don't think we should be doing this type of versioning. We did this on purpose so we can gradually make changes, now if we publish a new breaking version this will break out of the box.

---

src/util.ts, line 42 at r1 (raw file):

export function topK<T extends Float32Array|Int32Array|Uint8Array>(
    values: T, k: number): {values: T, indices: Int32Array} {

I don't think we're quite ready to graduate this method. We're still thinking about what a GPU implementation would be.

---

starter/es6/package.json, line 36 at r1 (raw file):

  },
  "scripts": {
    "localprep": "yarn link deeplearn && yarn run prep",

I don't totally understand what would be the difference between "prep" and "localprep" here.

---

Comments from Reviewable

[dsmilkov]

I agree with Nikhil here. Also since this turned out to be big invasive change, how about we chat in person and figure out what problems you were facing with the current build system and gradually (one change at a time) move it towards something better.

---

Review status: 0 of 25 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

.gitignore, line 13 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

we shouldn't be ignoring .vscode - this is checked in so we share vscode settings.

+1 If you are using vscode, you also use those settings.

---

package.json, line 3 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

why -dev? This will mess up our versioning scripts.

This is very unusual - can you elaborate a bit on -dev to reflect the fact that new commits are not included in the already-published versions. ?

Looking at popular js libraries, they use regular version (e.g. https://github.com/vuejs/vue/blob/dev/package.json#L3). The consensus is to update package.version only when we publish new version to npm.

If people get confused by this, we can revisit. So far, we've had no issues.

---

demos/package.json, line 55 at r1 (raw file):

    "@bower_components/webcomponentsjs": "webcomponents/webcomponentsjs#^0.7.24",
    "chart.js": "~2.7.1",
    "deeplearn": "latest",

curious, what was the problem with file:./ ?

I agree on the part that demos should use the models dl-knn-image-classifier and dl-squeezenet at HEAD/latest

---

demos/package.json, line 84 at r1 (raw file):

    "localprep": "yarn link deeplearn && yarn link deeplearn-knn-image-classifier && yarn link deeplearn-squeezenet && yarn run prep",
    "prep": "yarn",
    "localbuild": "yarn run build && yarn link",

no need for yarn run x, can just say yarn x

---

demos/tsconfig.json, line 6 at r1 (raw file):

  "compilerOptions": {
    "declaration": false,
    "outDir": "./dist",

+1 to not mixing demos output with core lib output

---

models/package.json, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

why are you removing this? the other packages extend from this.

+1 this is important and should stay. this is basically a peerDeps when building standalone module for the browser. browserify-shim avoids putting core deeplearn library twice in the bundle when we package standalone knn-image-classifier in the browser. knn-image-classifier depends on both dl and squeezenet (squeezenet depends on dl) as well.

---

models/knn_image_classifier/package.json, line 9 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I don't think we should be doing this type of versioning. We did this on purpose so we can gradually make changes, now if we publish a new breaking version this will break out of the box.

+1 We want to keep models/** separated from our latest published version, since we publish them on npm, and we want to update them at our own pace, and not break people over night.

demos/ on the other hand are never meant to be published on npm, so we make them depend at master.

---

models/yolo_mobilenet/package.json, line 8 at r1 (raw file):

  "unpkg": "dist/bundle.js",
  "types": "dist/index.d.ts",
  "dependencies": {

peerDep is the correct thing to do here, otherwise we'll end up with several copies of core dljs, because of transitive deps and when you bundle, you'll have 3x bundle sizes. Also models SHOULD share the same core dljs engine.

---

Comments from Reviewable

[dsmilkov]

Also let's aim to have a build system that requires the least amount of changes to scripts/workflow when we migrate models/* to its own repo and demos to its own repo. In the current system at HEAD you can replace file:// with npm-pck-name when you migrate out. In the new proposed system we will have to remove "localprep" and localbuild" and change our habits when we transition.

---

Review status: 0 of 25 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[dsmilkov]

cc @tafsiri who has experience with the JS OSS setup and community consensus

[davidsoergel]

Thanks for the helpful comments! First of all I want to be clear that I'm not trying to be disruptive-- sorry if it came across that way! There are a couple of things actually blocking me in here, and the rest was just stuff I happened across along the way. I agree these could be split up into simpler PRs, but also thought it might be easier to discuss all in one place first.

The motivation is that I'm simultaneously trying to add features to DLJS and to use those features in a separate package that depends on DLJS. Thus I'll have open branches in both repos. For a given feature, I'll ultimately want to submit a DLJS PR first, followed by the dependent PR in my project repo-- but I don't want to submit either one until I'm confident that they work together correctly. So during development, I want to be able to make a coordinated change in both repos/branches, build DLJS, build my project, and test the combined result. This is exactly the situation that DLJS will be in if you put demos and models in separate repos as Daniel said, so this can be seen as preparing for that eventuality.

NPM/Yarn/etc. seems to have poor support for this use case. How might we solve it? Some options:

0. Don't solve it. Require DLJS changes to be in a release before attempting to use them downstream. I think this is what Daniel's suggestion to "replace file:// with npm-pck-name when you migrate out" does. I think that would hamper development across the board; for instance, I don't think you'd want to postpone even starting to update the demos until after a release is out.

1. Use a dependency like file:../deeplearnjs even across repos, requiring developers to put their DLJS and demos working directories next to each other or in some other prescribed directory structure. Seems dodgy, and in any case can only work for projects that will not be published in NPM (since the file: dependencies can't resolve in that context).

2. Require anybody wanting to do this to manually "cd demos; npm install ../deeplearnjs", which just puts a symlink in node_modules. Unfortunately "npm install" clobbers such symlinks with a download of the latest release, so that's unreliable and confusing. "yarn" does not clobber the symlinks, but also "yarn add ../deeplearnjs" puts a file: dependency in package.json, so that would set us back to option 1.

3. Require cross-repo developers to manually "cd demos; yarn link deeplearnjs". This works (so long as we stick with yarn and avoid npm), but, as Daniel pointed out, it's extra effort for each person and doesn't guarantee that we all have a consistent setup.

4. Use "yarn link" consistently, and try to automate this to the extent possible via package.json scripts. I figured that's the least-worst option, so that's what I'm trying here. The purpose of the localprep and localbuild scripts is to manage the yarn links--i.e., you'd want to use these for "local" development where you're developing against unreleased changes in sister repos (e.g., developing against HEAD). That's different from the regular prep and build scripts, which should work independent of any local links by downloading the latest release of each dependency. When we migrate demos out, we would not remove the localprep and localbuild scripts; they provide the mechanism to find the working directory of DLJS. I agree with Nikhil that this approach munges the global yarn state, and that is sad, but I don't see how else to do it.

Most of the changes here follow from the general logic of option 4. If there is a better way to do this, I'm all ears. Thanks!

---

Review status: 0 of 20 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

.gitignore, line 13 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

+1 If you are using vscode, you also use those settings.

Oops, I confused myself because I had made changes there that were personal preferences, not to be checked in. Fixed.

---

package.json, line 3 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

This is very unusual - can you elaborate a bit on -dev to reflect the fact that new commits are not included in the already-published versions. ?

Looking at popular js libraries, they use regular version (e.g. https://github.com/vuejs/vue/blob/dev/package.json#L3). The consensus is to update package.version only when we publish new version to npm.

If people get confused by this, we can revisit. So far, we've had no issues.

Between releases, the code is neither the prior release version nor the following one. If we add a feature to DLJS and then want to use it in the development of a different package, how can we specify the dependency? If we depend on 0.5.0, it will not work (in the absence of a 'yarn link' to HEAD) because the feature is not present. If we depend on 0.5.1, it doesn't work because that version doesn't exist. See also point 9 at semver.org.

I didn't look at the versioning scripts yet but would be happy to update those too to take this into account.

---

demos/package.json, line 55 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

curious, what was the problem with file:./ ?

I agree on the part that demos should use the models dl-knn-image-classifier and dl-squeezenet at HEAD/latest

For deeplearn, 'latest' amounts to the same thing as file:./ here, because of the link setup. I only changed it for consistency and to look forward to separating demos into their own repo, at which point file: deps don't make sense anyway.

For the model dependencies, I thought the file: style a bit weird because they're published packages, but I don't think it actually makes a difference there either for now. (Where it wouldn't work is e.g. knn-image-classifier depending on squeezenet, because knn-image-classifier is published).

---

demos/package.json, line 84 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

no need for yarn run x, can just say yarn x

Yes, except that breaks when x happens to be a yarn command. The 'yarn run x' form is unambiguous so I'm much less confused about what will happen.

---

demos/tsconfig.json, line 6 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

+1 to not mixing demos output with core lib output

This one blocks 'yarn link deeplearn' entirely and so is critical for me.

---

demos/game_of_life/game_of_life.ts, line 82 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

you shouldn't need this, array.shape is number[]

I agree, but without it I get a lint error at "x1 + pad" below. Maybe there's a tslint version issue...??

---

models/knn_image_classifier/package.json, line 9 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

+1 We want to keep models/** separated from our latest published version, since we publish them on npm, and we want to update them at our own pace, and not break people over night.

demos/ on the other hand are never meant to be published on npm, so we make them depend at master.

+1, I get it now. I mistakenly thought that yarn-lock.json and/or package-lock.json would pin the version at publication time, but apparently those aren't published, so I agree package.json has to pin the version.

That produces a really confusing situation, though, because the demos should depend on DLJS at head (whether specified as file:../ or latest), and they also depend on models/*, which may in turn depend on an older DLJS. I understand the whole point of NPM is that both versions of DLJS get installed in this situation and it actually works. Still, for this to be happening within a single repo seems unexpected.

What I was trying to do, more or less, was to let the models break locally during development. It seems to me that would be informative-- i.e. it would be reassuring to see continuous verification that the models are not breaking due to core changes, and useful to be notified when a breakage does occur. In this case a breaking change would force either a) updating the model right away, or b) pinning the version if/when a breakage actually happens.

So I think a good thing to do might be to let models depend on deeplearn ~0.5.0, but to try to make these dependencies track the deeplearn version instead of falling behind. In particular, it would be good to update them as part of any breaking release. I think this would cause the demos to install only the latest DLJS and not also an older one. WDYT?

---

models/yolo_mobilenet/package.json, line 8 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

peerDep is the correct thing to do here, otherwise we'll end up with several copies of core dljs, because of transitive deps and when you bundle, you'll have 3x bundle sizes. Also models SHOULD share the same core dljs engine.

+1, got it, thanks for the explanation and sorry for my confusion! Fixed.

---

starter/es6/package.json, line 36 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I don't totally understand what would be the difference between "prep" and "localprep" here.

The idea was that localprep uses the local working directory of DLJS via the link, whereas prep doesn't rely on that and can just download DLJS if needed. Normal users will want prep, but cross-repo developers will want localprep. build_and_lint_all uses the local* versions since it's clearly aimed at the cross-package (future cross-repo) integration case.

One hiccup is that if you run localprep and then later just prep, you'll still have the link. Maybe there should also be a prep script that specifically unlinks things. Then localprep could be called "linkprep" and the converse could be "unlinkprep", or some such.

---

models/package.json, line 1 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

+1 this is important and should stay. this is basically a peerDeps when building standalone module for the browser. browserify-shim avoids putting core deeplearn library twice in the bundle when we package standalone knn-image-classifier in the browser. knn-image-classifier depends on both dl and squeezenet (squeezenet depends on dl) as well.

I am starting to understand the browserify-shim stuff (i.e. now I get the reasoning for peerDeps). However I don't see how this file helps.

What do you mean "the other packages extend?" There is no inheritance among nested package.jsons as far as I can tell. It appears to me that this file isn't doing anything, both because I don't see any mechanism for it to have an effect, and because the models/* package.json files already inline all of the content anyway (including, in particular, the browserify-shim entry).

Am I missing something?

---

Aha, update: browserify-shim complains if this is not here, but only because of models/util.ts. I've reverted it now, but see my comment below about ditching models/util.ts in another PR.

---

src/util.ts, line 42 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I don't think we're quite ready to graduate this method. We're still thinking about what a GPU implementation would be.

OK. I was trying to get rid of models/util.ts because the models/* packages were directly including that as a source file outside of their local tree (i.e., import '../util.ts'), which seemed dodgy to me. If you don't want it in DLJS yet, I see two other solutions:

1. create a new 'model-util' package so that the models can depend on it, or

2. just paste topK() into each model somewhere, with a TODO to use the DLJS version when it's ready.

I think I'd favor b; WDYT?

In any case, this issue is orthogonal to everything else so I reverted all of it (except the little type update, now in models/util.ts, for 0.5.0 compatibility).

---

Comments from Reviewable

[davidsoergel]

P.S. I know the Travis test failed, but I think the reasons are minor hiccups (not worth addressing until/unless we agree on the overall approach).

---

Review status: 0 of 20 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[davidsoergel]

Oh, one more thing: Yarn Workspaces are supposed to solve much of this (https://yarnpkg.com/lang/en/docs/workspaces/). At some point along the way I convinced myself that they wouldn't work, but now I can't figure out why, so maybe it's a great solution! If so, I think that would basically automate all of the 'yarn link's in one swoop, and would remove the need for the local* scripts.

There's also Lerna (https://lernajs.io/) and other similar tools specifically addressing this problem. I was just hoping we could solve it without throwing yet another package manager into the mix.

---

Review status: 0 of 20 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[tafsiri]

@dsmilkov @nsthorat @davidsoergel lets chat in person today. First impressions are that automating yarn link seems like a good idea rather than depending on file urls which will break as we move things into other repos. I'd have a preference for specific versions in the package.json over latest, but that mainly ends up being a workflow issue.

[davidsoergel]

OK so the offline consensus, IIUC, is:

• Try to reduce the need for inter-package dependencies at head (e.g., by getting rid of the demos package.json, deciding what to do about the models/* circular dependency, etc. ) and
• expect developers to manage their own yarn links as needed.

However: in the interim, before we get to that state, I think we still need the Travis build to use deeplearn head when building demos and models (but not starters!). So I updated build_and_lint_all to reflect the minimum necessary yarn links given the status quo.

---

Review status: 0 of 20 files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

package.json, line 3 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Between releases, the code is neither the prior release version nor the following one. If we add a feature to DLJS and then want to use it in the development of a different package, how can we specify the dependency? If we depend on 0.5.0, it will not work (in the absence of a 'yarn link' to HEAD) because the feature is not present. If we depend on 0.5.1, it doesn't work because that version doesn't exist. See also point 9 at semver.org.

I didn't look at the versioning scripts yet but would be happy to update those too to take this into account.

OK it sounds like the convention is just to leave the version string at the old value while developing towards a new release. This still smells wrong to me, because this package.json is making an assertion about the version of the code that is not true at head. But, in practice, using 'yarn link' or depending on a specific commit does work around this, so I'll drop the issue. I reverted all these -dev versions.

---

demos/package.json, line 55 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

For deeplearn, 'latest' amounts to the same thing as file:./ here, because of the link setup. I only changed it for consistency and to look forward to separating demos into their own repo, at which point file: deps don't make sense anyway.

For the model dependencies, I thought the file: style a bit weird because they're published packages, but I don't think it actually makes a difference there either for now. (Where it wouldn't work is e.g. knn-image-classifier depending on squeezenet, because knn-image-classifier is published).

Per offline discussion the plan is now to ditch demos/package.json at some point. In the meantime there's no harm in leaving the file: dependency here.

---

starter/es6/package.json, line 36 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

The idea was that localprep uses the local working directory of DLJS via the link, whereas prep doesn't rely on that and can just download DLJS if needed. Normal users will want prep, but cross-repo developers will want localprep. build_and_lint_all uses the local* versions since it's clearly aimed at the cross-package (future cross-repo) integration case.

One hiccup is that if you run localprep and then later just prep, you'll still have the link. Maybe there should also be a prep script that specifically unlinks things. Then localprep could be called "linkprep" and the converse could be "unlinkprep", or some such.

Offline consensus is to ditch the local* scripts and just make developers manage their yarn links manually (or via a workspace). I think we still need some yarn links in build_and_run_all, though, for the sake of Travis.

---

Comments from Reviewable

[dsmilkov]

Reviewed 1 of 12 files at r2, 2 of 16 files at r3.
Review status: 3 of 20 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

---

package.json, line 46 at r3 (raw file):

  },
  "scripts": {
    "prep": "yarn && mkdirp dist && yarn build && cd demos/ && yarn prep && cd ..",

if the dist directory doesn't exist, I think yarn build/tsc might fail? (since tsconfig.outDir is './dist')

---

demos/package.json, line 55 at r3 (raw file):

    "@bower_components/webcomponentsjs": "webcomponents/webcomponentsjs#^0.7.24",
    "chart.js": "~2.7.1",
    "deeplearn": "file:../dist/",

i think this is dangerous, since it will include all ../node_modules (all deps of the main lib, and significantly slow down compilation). could also be related to spurious lint errors you are getting.

---

demos/tsconfig.json, line 6 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

This one blocks 'yarn link deeplearn' entirely and so is critical for me.

Yup, I was +1ing your change ;)

---

demos/game_of_life/game_of_life.ts, line 82 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

I agree, but without it I get a lint error at "x1 + pad" below. Maybe there's a tslint version issue...??

Yeah, strange that you are getting that error. Try to npm unlink everything and see if you are still getting those errors? Also see comment above in demos/package.json where you did s / file:../dist / file:.. /

---

models/util.ts, line 18 at r3 (raw file):

 */

export function topK<T extends Float32Array|Int32Array|Uint8Array>(

strange that none of us are getting compilation error here. make sure you are using tsc 2.7.2.

---

models/knn_image_classifier/package.json, line 9 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

+1, I get it now. I mistakenly thought that yarn-lock.json and/or package-lock.json would pin the version at publication time, but apparently those aren't published, so I agree package.json has to pin the version.

That produces a really confusing situation, though, because the demos should depend on DLJS at head (whether specified as file:../ or latest), and they also depend on models/*, which may in turn depend on an older DLJS. I understand the whole point of NPM is that both versions of DLJS get installed in this situation and it actually works. Still, for this to be happening within a single repo seems unexpected.

What I was trying to do, more or less, was to let the models break locally during development. It seems to me that would be informative-- i.e. it would be reassuring to see continuous verification that the models are not breaking due to core changes, and useful to be notified when a breakage does occur. In this case a breaking change would force either a) updating the model right away, or b) pinning the version if/when a breakage actually happens.

So I think a good thing to do might be to let models depend on deeplearn ~0.5.0, but to try to make these dependencies track the deeplearn version instead of falling behind. In particular, it would be good to update them as part of any breaking release. I think this would cause the demos to install only the latest DLJS and not also an older one. WDYT?

Per offline discussion, models should be ideally pinned to an exact version, so we can update them at our own pace (imagine if models/* is something we don't have control over, like a third-party library using us)

---

models/knn_image_classifier/package.json, line 9 at r3 (raw file):

  "types": "dist/knn_image_classifier/index.d.ts",
  "peerDependencies": {
    "deeplearn": "~0.5.0"

let's not update this yet, since we haven't migrated the code for knn_image_classifier to use 0.5.0, nor tested it manually.

---

models/knn_image_classifier/package.json, line 21 at r3 (raw file):

    "browserify": "~14.5.0",
    "browserify-shim": "~3.8.14",
    "deeplearn": "~0.5.0",

let's not update this yet, since we haven't migrated the code to use 0.5.0 yet

---

models/knn_image_classifier/package.json, line 30 at r3 (raw file):

mkdirp dist

if you don't make a dist directory, the next command (yarn build) will fail, since tsc has -o dist/bundle.js flag

---

models/mobilenet/package.json, line 9 at r3 (raw file):

  "types": "dist/mobilenet/index.d.ts",
  "peerDependencies": {
    "deeplearn": "~0.5.0"

let's not update this yet, since we haven't migrated the code to use 0.5.0 yet

---

models/mobilenet/package.json, line 27 at r3 (raw file):

  },
  "scripts": {
    "prep": "yarn && mkdirp dist",

same here, i think if dist doesn't exist (new user) , yarn build might fail.

---

models/squeezenet/package.json, line 9 at r3 (raw file):

  "types": "dist/squeezenet/index.d.ts",
  "peerDependencies": {
    "deeplearn": "~0.5.0"

let's not update this yet, since we haven't migrated the code to use 0.5.0 yet

---

models/yolo_mobilenet/package.json, line 9 at r3 (raw file):

  "types": "dist/index.d.ts",
  "peerDependencies": {
    "deeplearn": "~0.5.0"

same here, revert to 0.3.15 (last version we tested this model on)

---

scripts/build_and_lint_all, line 26 at r3 (raw file):

cd $ROOTDIR

# Build all models against head.

let's not build models againts head, since they are/will be published on npm. Let's think of them as something we don't control (like a thiry-party lib that happens to use dljs)

---

src/util.ts, line 42 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

OK. I was trying to get rid of models/util.ts because the models/* packages were directly including that as a source file outside of their local tree (i.e., import '../util.ts'), which seemed dodgy to me. If you don't want it in DLJS yet, I see two other solutions:

1. create a new 'model-util' package so that the models can depend on it, or

2. just paste topK() into each model somewhere, with a TODO to use the DLJS version when it's ready.

I think I'd favor b; WDYT?

In any case, this issue is orthogonal to everything else so I reverted all of it (except the little type update, now in models/util.ts, for 0.5.0 compatibility).

I'm fine with 2), but we can push this to a later PR

---

Comments from Reviewable

[davidsoergel]

Review status: 3 of 20 files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

---

package.json, line 46 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

if the dist directory doesn't exist, I think yarn build/tsc might fail? (since tsconfig.outDir is './dist')

No problem-- tsc creates ./dist automatically.

---

demos/package.json, line 55 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

i think this is dangerous, since it will include all ../node_modules (all deps of the main lib, and significantly slow down compilation). could also be related to spurious lint errors you are getting.

Depending directly on dist/ is invalid since there is no package.json there. Depending on .. should be fine, because the package.json includes "main": "dist/index.js". Also, node_modules is always ignored per the package.json docs (https://docs.npmjs.com/files/package.json#files).

---

demos/tsconfig.json, line 6 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Yup, I was +1ing your change ;)

Yep and I was +1ing your +1-- sorry that was unclear! :)

---

demos/game_of_life/game_of_life.ts, line 82 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Yeah, strange that you are getting that error. Try to npm unlink everything and see if you are still getting those errors? Also see comment above in demos/package.json where you did s / file:../dist / file:.. /

I'm running tsc 2.7.2, and just updated tslint to ~5.9.1. Still getting "(restrict-plus-operands) ... Operands of '+' operation must either be both strings or both numbers". VSCode understands that x1 and x2 are numbers already, so I can't explain it :(

---

models/util.ts, line 18 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

strange that none of us are getting compilation error here. make sure you are using tsc 2.7.2.

This is specifically for Mobilenet, and I think the reason for this is its DLJS dependency version. mobilenet.ts line 141 has "model_util.topK(await predictions.data(), topK);", where (at HEAD) predictions.data() is a TypedArray. Previously Mobilenet was building against DLJS 0.3.15, where data() provided just a Float32Array.

See thread below about whether we should build models against head or not. If we don't, we could leave this unchanged, but we'd just have to make the change again when updating Mobilenet anyway. ¯_(ツ)_/¯

---

models/knn_image_classifier/package.json, line 9 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

let's not update this yet, since we haven't migrated the code for knn_image_classifier to use 0.5.0, nor tested it manually.

Obsoleted by #769

---

models/knn_image_classifier/package.json, line 30 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

mkdirp dist

if you don't make a dist directory, the next command (yarn build) will fail, since tsc has -o dist/bundle.js flag

Works for me. tsc creates ./dist as needed.

---

models/mobilenet/package.json, line 9 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

let's not update this yet, since we haven't migrated the code to use 0.5.0 yet

Done.

---

models/mobilenet/package.json, line 27 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

same here, i think if dist doesn't exist (new user) , yarn build might fail.

no problem

---

scripts/build_and_lint_all, line 26 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

let's not build models againts head, since they are/will be published on npm. Let's think of them as something we don't control (like a thiry-party lib that happens to use dljs)

I'd think it's still a good idea to build and lint them, just to get early notification of breaking changes. If we didn't build them, then the name "build_and_lint_all" would be a bit misleading.

However I agree about not using these head builds for anything. Further down I commented out the "yarn link" entries for these, so that the demos will be built against the latest published npm version.

Does that make sense?

---

src/util.ts, line 42 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

I'm fine with 2), but we can push this to a later PR

Sounds good, will do a followup PR

---

Comments from Reviewable

[davidsoergel]

Abandoning this for now per offline discussion; we'll roll whatever useful bits remain into a cleanup later.