ImageClassifier catches an error which should be thrown

[lindapaiste]

One of my general concerns for the package is that it is too hard for users to discover what they are doing wrong when things don't work as expected.

Here's an example from ImageClassifier. If you create the model with an invalid model name then when you call model.predict(image) you will get an error:

TypeError: h.model.predict is not a function

That error doesn't indicate the true problem which is the bad name. We are actually logging the real error to the console:

Error: Failed to parse model JSON of response from xxx. Please make sure the server is serving valid JSON for this request.

But that error is caught and not re-thrown so this is treated as a "success". The Promise will resolve and the user gets an ImageClassifier instance, but none of the methods will work properly.

For this particular model, the problem is lines 116-118. I suspect that other models have similar issues. We should not resolve a Promise if the model is in an invalid state.

It's actually extremely hard for the user to validate that they got a functioning model:

// error is undefined
// model is an instance of ImageClassifier
function magicFunction(error, model) {
  // will never be true because error is undefined
  if (error) {
    console.log("caught error", error);
    return;
  }
  // will never be true because the instance is returned
  if (!model) {
    console.log("no model");
    return;
  }
  // will never be true because it's a Promise object
  if (!model.ready) {
    console.log("model not ready");
    return;
  }
  // will never be true because model is set to an Error object
  if (!model.model) {
    console.log("model not created");
    return;
  }
  // winds up here
  console.log("no conditions matched");
  console.log(model);
  // this is an error
  model.predict(createImage()).then(setPrediction);
}

// call with an invalid model
ml5.imageClassifier("xxx", magicFunction);

CodeSandbox

[lindapaiste]

I knew that something was very wrong with the callbacks but it took me a while to figure out exactly what it is. It's that callCallback is always resolved and never rejected. It catches the error and returns the error, so the value returned from callCallback is a Promise that resolves to the error.

Most of the models set this.ready like this:

this.ready = callCallback(this.loadModel(), callback);

So this.ready is never rejected. It is either pending, fulfilled with a model, or fulfilled with an error.

Sometimes the model classes use await this.ready in their methods to prevent the method from running before the model is available. This successfully delays if the model can be loaded. If there is an error loading the model, we want this.ready to be rejected so that the method is rejected and/or calls the method callback with the error. But this.ready always resolves, so the the body of the method is always executed even when this.model is never set. This likely leads to runtime errors which will be caught by the Promise or callback of the method, but it's not the "right" error.

Here's a demo where you can play with the inputs on the last few lines and check the console output. With the current callCallback implementation (improvedCallback: false in the demo) we get TypeError: Cannot read property 'toUpperCase' of undefined. We want Error: Some specific model load error.

I recommend changing the callCallback function to this:

const callCallback = async ( promise, callback ) => {
  if (!callback) return promise;
  return new Promise((resolve, reject) => {
    promise
      .then((result) => {
        callback(undefined, result);
        resolve(result);
      })
      .catch((error) => {
        callback(error);
        reject(error);
      });
  });
};

I am replacing return result and return error with resolve(result) and reject(error).