FirebaseML - adding Long Running Operation support #951

ifielker · 2020-07-15T20:32:12Z

Add long running operation support

hiranya911

Some suggestions for improvement. Didn't look at the unit tests yet. Will get to them in the next round.

hiranya911 · 2020-07-16T20:28:00Z

package.json

@@ -62,7 +62,7 @@
  },
  "optionalDependencies": {
    "@google-cloud/firestore": "^4.0.0",
-    "@google-cloud/storage": "^5.0.0"
+    "@google-cloud/storage": "^5.1.2"


Why was this needed?

I don't think it was, I just installed the latest, I guess it changed this file too. I'll revert.

src/machine-learning/machine-learning-api-client.ts

src/machine-learning/machine-learning.ts

hiranya911

Looks mostly good. I went through the tests as well. Had a couple of questions around toJSON and equals plus some other suggestions.

hiranya911 · 2020-07-17T22:01:53Z

src/machine-learning/machine-learning-api-client.ts

+      return this.getModel(modelId);
+    }
+
+    return this.pollOperationWithExponentialBackoff(opName, options);


opName is first used here. May be just remove the variable and use op.name! directly?

src/machine-learning/machine-learning-api-client.ts

src/machine-learning/machine-learning.ts

test/unit/machine-learning/machine-learning-api-client.spec.ts

test/unit/machine-learning/machine-learning.spec.ts

src/machine-learning/machine-learning.ts

hiranya911 · 2020-07-17T23:09:33Z

Also since there's a new API here, shouldn't there be some changes in the d.ts file?

src/machine-learning/machine-learning.ts

hiranya911

Looks great. Just had a few nits. I'll be at an LGTM once they are addressed.

Adding @egilmorez to review the changes to d.ts file.

src/index.d.ts

src/machine-learning/machine-learning.ts

hiranya911

Thanks. LGTM with a nit.

hiranya911 · 2020-07-21T17:28:41Z

src/machine-learning/machine-learning.ts

+      // because models with active operations came back from the server and
+      // were constructed with a non-empty client.
+      return this.client!.handleOperation(this.model.activeOperations![0], { wait: true, maxTimeMillis })
+        .then((modelResponse) => this.updateFromResponse(modelResponse));


Nit: updateFromResponse is now a one-liner. Just this.model = Model.validateAndClone(modelResponse); and remove the updateFromResponse() helper.

egilmorez

The index.d.ts changes look good, just a couple nits. Thanks!

egilmorez · 2020-07-22T18:20:09Z

src/index.d.ts

@@ -1317,12 +1317,18 @@ declare namespace admin.machineLearning {
    /**
     * Wait for the model to be unlocked.
     *
-     * @param {number} maxTimeSeconds The maximum time in seconds to wait.
+     * @param {number} maxTimeMillis The maximum time in milliseconds to wait.
+     *     If not specified, a default maximum of 2 minutes will be used.


We try to avoid "will." Suggest just "is used."

egilmorez · 2020-07-22T18:21:13Z

src/index.d.ts

+    waitForUnlocked(maxTimeMillis?: number): Promise<void>;
+
+    /**
+     * Return the JSON format of the model.


This feels slightly weird, unless "format" is a special noun in your context.

Would "Return the model in JSON format" or "as a JSON object" work?

FirebaseML - adding Long Running Operation support

edd48dd

ifielker assigned hiranya911 Jul 15, 2020

hiranya911 suggested changes Jul 16, 2020

View reviewed changes

hiranya911 assigned ifielker and unassigned hiranya911 Jul 16, 2020

review changes

2404911

ifielker assigned hiranya911 Jul 17, 2020

hiranya911 suggested changes Jul 17, 2020

View reviewed changes

hiranya911 reviewed Jul 18, 2020

View reviewed changes

src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved

hiranya911 removed their assignment Jul 20, 2020

review comments 2

49e4aab

ifielker assigned hiranya911 Jul 20, 2020

hiranya911 suggested changes Jul 21, 2020

View reviewed changes

src/index.d.ts Show resolved Hide resolved

src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved

src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved

src/machine-learning/machine-learning.ts Outdated Show resolved Hide resolved

hiranya911 requested a review from egilmorez July 21, 2020 00:13

hiranya911 assigned egilmorez and unassigned hiranya911 Jul 21, 2020

review comments 3

ccbf8da

hiranya911 approved these changes Jul 21, 2020

View reviewed changes

review comments 4

3b4d469

egilmorez reviewed Jul 22, 2020

View reviewed changes

review comments 5

66a8f8c

ifielker merged commit 8b7ce46 into ml-automl Jul 22, 2020

ifielker deleted the if-automl-1 branch July 22, 2020 20:38

FirebaseML - adding Long Running Operation support #951

FirebaseML - adding Long Running Operation support #951

Uh oh!

Conversation

ifielker commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 commented Jul 17, 2020

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egilmorez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ifielker commented Jul 15, 2020 •

edited

Loading