Support Promises for imageClassifier

[tmcw]

This includes:

• The return values of imageClassifier() and ImageClassifier.predict()
  are now promises.
• Uncomments, updates, and implements tests for ImageClassifier

The changes

ml5 classes have traditional constructors but many of them have asynchronous 'loading' requirements and then synchronous methods. ES6 class constructors can't be async methods (you can't say await new Foo()), so this does the following:

• All classes that need some async setup phase have a ready property that is a Promise that is resolved when they're ready
• The 'factory functions' that people use to get instances of those classes (like the imageClassifier class for ImageClassifier) have two behaviors:
  • The user provides a callback and they return the class instance
  • The user does not provide a callback and they return the class instance's 'ready' property, which is a promise that resolves to that class instance once it's ready.

So:

// callbacks
var ifier = imageClassifier('MobileNet', undefined, {}, onReady);
function onReady() {
  // do stuff with ifier
}

// promises
imageClassifier('MobileNet', undefined, {}).then(iffier => {
  // do stuff with iffier
});

// async/await syntax
async function run() {
  let iffier = await imageClassifier('MobileNet', undefined, {});
  // do stuff with iffier
}

Other changes

• Implements tests for everything that's testable!
  • This doesn't include ImageClassifier. The @tf-models/mobilenet module that powers ImageClassifier doesn't run in the karma/webpack environment for reasons I can't quite ascertain right now.
• Switches the karma reporter from progress, which is very minimal, to mocha, which is a bit more human. Karma re-runs portions of your tests and, especially developing new tests, I wanted to know which tests were running and which had passed/failed, instead of only getting details on failures and only getting summary numbers.

📖 The error around mobilenet

Error message:

    Error: Argument 'x' passed to 'conv2d' must be a Tensor, but got object.
        at c (src/ImageClassifier/index.test.js:1:2672)
        at a (src/ImageClassifier/index.test.js:1:2126)
        at n (src/ImageClassifier/index.test.js:1:2330)
        at Object.i (src/ImageClassifier/index.test.js:1:2355)
        at e.conv2d (src/ImageClassifier/index.test.js:1:25150)
        at src/ImageClassifier/index.test.js:1:82279
        at e.tidy (src/ImageClassifier/index.test.js:1:142745)
        at Object.n.value (src/ImageClassifier/index.test.js:1:82256)
        at src/ImageClassifier/index.test.js:1:487368
        at e.tidy (src/ImageClassifier/index.test.js:1:142745)

Having dug into this a bit, my best guess is that it's a problem with 2 instances of Tensorflow, so that there's a check in Tensorflow that does obj instanceof Tensor and it's comparing it to the wrong copy of tensorflow's Tensor. But I haven't been able to confirm this - it really does look like there's one copy of tfjs-core in the mix.

Things this doesn't resolve but I've noticed

• pitchDetection almost certainly has a race condition - its constructor loads a tf model asynchronously but nothing waits for that loading to end, and methods like processMicrophoneBuffer could conceivably run before this.model is defined / usable.
• poseNet's implementation doesn't appear to precisely match its docs - at least in my reading of the code, it'll call its callback for each frame with a new prediction, rather than calling the callback once with an array.
• I'm pretty sure that the YOLO documentation is also off by a bit: creating a YOLO object without a video parameter is not valid - i always calls this.loadVideo() right now.
• It appears that YOLO only works for 416x416 images?

Callback situation

Right now this doesn't attempt to change or make the callback system uniform, because the conversation in #143 is still ongoing. I don't anticipate a lot of code change based on the outcome of that discussion, it's mostly a design problem rather than a technical challenge.

[tmcw]

I'm going to guess that CircleCI is unhappy because of a WebGL support issue? Tests pass locally for me.

[cvalenzuela]

This is great, thanks @tmcw!

I'm not super familiar with running tests with WebGL support. Maybe @nsthorat might help with this?

[nsthorat]

I would use browserstack via travis! They are phenomenal and free for small projects (it's still free for us), they actually run your tests on real devices with WebGL enabled.

Here is our travis config which kicks off our tests:
https://github.com/tensorflow/tfjs-core/blob/master/.travis.yml#L6

Karma, the test runner, gets kicked off here:
https://github.com/tensorflow/tfjs-core/blob/master/scripts/test-travis.sh

Here is the karma config, which has info about browserstack as well as local testing:
https://github.com/tensorflow/tfjs-core/blob/master/karma.conf.js

Through travis, you add the environments for your browserstack key, which you can see above.

Let me know if you need any help!

[tmcw]

Okay, so - moving along with some of this:

• The karma setup could really use some sourcemaps - right now errors in tests all come from the same line of the same file - so I'm trying to add that in this PR, with... unfortunately pretty limited success, karma seems to hang once sourcemaps are added. Any karma whizzes, help would be appreciated; there aren't many clues for debugging, the browser page opens but is blank, no console errors.
• Looks like the answer to that question of promises or callbacks is - with the API as it is currently, it would be necessary to return the object if a callback is provided and a promise if it isn't - because right now the callbacks don't have any argument values.

[cvalenzuela]

@oveddan helped us with some setup of karma and sourcemaps, he might help us on this.

As for the promises/callback, I think that's the right approach. Return the object if a callback is provided and Promise if not.

[tmcw]

I think this is a bugfix - the previous behavior is that calling this method before the model is fully loaded would do nothing.

[cvalenzuela]

yep, thanks for the catch!

[tmcw]

I'd also posit that this is a bugfix; pitchdetection has an async loading step but no callback.

[cvalenzuela]

👍

[meiamsome]

Note that this doesn't actually add a callback to the constructor.

[tmcw]

minor typo fix here.

[cvalenzuela]

👍

[tmcw]

possibly there was some reason why this bit of code used XMLHttpRequest? It's not apparent in the source before, and I think it'd be ideal if fetch is used uniformly.

[nsthorat]

This was probably taken from the old checkpoint loader in deeplearnjs, there's likely no reason not to use fetch.

[cvalenzuela]

yes, we took this from the previous checkpoint loader. thanks for the update.

[tmcw]

Okay - we're getting there! This PR now includes promisification for all of the basic methods in ml5, and I've released a scoped testing module which works pretty well: https://beta.observablehq.com/d/d421a46185d4f063 This also confirms (phew) that Mobilenet isn't actually broken in general or anything - it's just being cranky about the testing environment.

The main tricky one is PoseNet:

The docs for PoseNet describe callback as A function to run once the model has been loaded. - which is definitely pretty far from what it actually does - the callback is called every time that poses are gotten from a video feed. The actual behavior is kind of tricky: If you don't provide a callback initially, then getting those poses is not obvious - it really looks like you'll end up with two tf.nextFrame() loops.

The most common abstraction for this is an event emitter:

let pn = await poseNet(video);
pn.on('pose', pose => {
   // do something with pose
});

Because really there are two stages here:

1. The asynchronous setup stage of posenet
2. The many poses that are emitted from it

My take is that we'd want to separate these and make the first one a promise or callback, and make the second one an event. But I'm happy to implement another thing if it fits better into the p5/ml5 ecosystem.

This is ready for a directional review if any folks have the time to spare!

[cvalenzuela]

This is a really amazing work @tmcw! thanks for all your work and help!

Since these are a lot of changes and some might affect the API, we will possible merge this after the beta release. I hope to go over all the changes more throughly tomorrow and also get some feedback from @shiffman.

[shiffman]

Wow, this is fantastic! @cvalenzuela, yes, I think a discussion at a working session would be great. Thank you @tmcw, we are so grateful for your contribution!

[oveddan]

My two cents: if you want to use promises/async, why not just call the original posenet api?

[nsthorat]

@tmcw most of these comments aren't necessarily geared towards you, sort of a code review of already submitted stuff :).

Feel free to ignore, just my 2c.

[nsthorat]

no need for tidy() here this actually won't do anything (there are no intermediate tensors)

[nsthorat]

cc @cvalenzuela one really useful thing to unit tests is memory usage.

One pattern we do is something like this:

const numTensors = tf.memory().numTensors;
apiCall();
expect(tf.memory().numTensors).toEqual(numTensors);

This makes sure you don't introduce leaks.

[cvalenzuela]

great!, I didn't knew that.

[nsthorat]

This was probably taken from the old checkpoint loader in deeplearnjs, there's likely no reason not to use fetch.

[nsthorat]

cc @cvalenzuela you may want to mock the actual fetch call in your unit tests so your tests are hermetic

[cvalenzuela]

can you expand a bit on this? I'm not sure what hermetic means in this context.

[nsthorat]

@cvalenzuela here and below the tensors are leaking. Same comment above, I'd add a unit test that asserts generate() doesn't create more tensors than you expect.

[cvalenzuela]

should we wrap this in tf.tidy()?

[nsthorat]

why not promise-ify these as well?

[meiamsome]

I'm not sure how I feel about the callback = () => {} and then not checking the value of callback later pattern - you might end up in a case where someone passes in a null to this function (Perhaps to add an errorCallback where these are supported without a regular callback, or as part of a wrapping function)

[tmcw]

@meiamsome I haven't converted this to the 'two callbacks pattern' or the 'node-style error argument' pattern because that discussion is ongoing. Regardless, if the two callbacks pattern is adopted, they could use undefined and it would properly fall back to the optional value. ml5/p5's APIs are very tolerant, I suppose, so maybe it should also check whether the value is falsy, but that's really a style decision for the maintainers.

[cvalenzuela]

Sorry for the delay in reviewing this @tmcw. I'm still learning about best practices and patterns with await/ES6.

My personal take with the callback pattern is to go with the conventional JS style. Since this would play better with more JS libraries. Although I should mention that we have been also discussing a more formal p5 support (#92). That support might change the callback pattern conversation a bit. I hope this will be something we can resolve in the next few days.

Regarding the other comments:

• pitchDetection needs some clean up and changes. We are tracking it here Pitch Detection API changes #158 and @handav was working on it. Once this PR is merged we should also update that method to follow the pattern we take here.
• poseNet will also get a general clean up and better usage with the original api.
• YOLO does only work with 416x416 videos for now. That's why that class creates a custom video element.
• After this gets merged, I will pushed another PR with better testing/docs, Browserstack support and a few changes following @nsthorat recommendations.

thanks!

[tmcw]

Okay!

• Implemented the (error, callback) pattern everywhere I could find, implementing a tiny helper method callCallback to cut down on the boilerplate and to make sure that it's all well-behaved, so that if you use callbacks you never get warnings about uncaught errors in promises. This PR is about +45loc net, which isn't that bad.
• PoseNet now extends EventEmitter and emits pose events.
• Added a small test for LSTMGenerator that will pass once the testing environment is tensorflow-friendly.

This doesn't yet include the bits about counting tensors and memory leaks because, unfortunately, tensorflow is not yet happy in the Karma setup. Besides that, ready for a re-review.

[nsthorat]

Just curious, what is not happy in the karma setup?

[shiffman]

I'm good for this and ml5js/ml5-examples#44 to merge whenever you like @cvalenzuela!