Added GetModel functionality and tests #781

ifielker · 2020-02-08T00:32:43Z

Added Firebase Machine Learning GetModel functionality and tests

hiranya911

Looks pretty solid. My main points of feedback are:

Remove anything that's not directly related to the getModel API from this PR.
Add unit tests for the API client implementation.

hiranya911 · 2020-02-08T00:52:52Z

test/integration/machine-learning.spec.ts

+
+describe('admin.machineLearning', () => {
+  describe('getModel()', () => {
+    it('rejects with not-found when the Ruleset does not exist', () => {


s/Ruleset/Model

hiranya911 · 2020-02-08T00:53:30Z

test/integration/machine-learning.spec.ts

+    });
+
+    it('rejects with invalid-argument when the ModelId is invalid', () => {
+      return admin.machineLearning().getModel('abc')


Nit: Use something more explicit like invalid-model-id to be clear instead of abc.

hiranya911 · 2020-02-08T00:54:01Z

test/integration/machine-learning.spec.ts

+          'code', 'machine-learning/invalid-argument');
+    });
+
+    it('resolves with existing Model', () => {


Remove from this PR. Makes future additions easier to review.

hiranya911 · 2020-02-08T00:55:22Z

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

+  }
+
+  public listModels(options: {listFilter?: string, pageSize?: number, pageToken?: string}) {
+    throw new Error('NotImplemented');


Please remove all the not-implemented stuff from this PR. Makes the future additions easier to review.

hiranya911 · 2020-02-08T00:57:15Z

src/machine-learning/machine-learning-utils.ts

+
+import { PrefixedFirebaseError } from '../utils/error';
+
+export type MachineLearningErrorCode =


Remove any errors codes that you're not explicitly referencing for now. You can add them in the future PR if they become necessary.

These are all possible to come back from the server.

hiranya911 · 2020-02-08T00:59:36Z

src/machine-learning/machine-learning.ts

+    this.published = model.state?.published || false;
+    this.etag = model.etag;
+    if (model.modelHash) {
+      this.modelHash = model.modelHash;


If this is set conditionally, change the type declaration to be modelHash?. Both here in the class, and also in the d.ts file.

hiranya911 · 2020-02-08T01:07:56Z

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

@@ -0,0 +1,244 @@
+/*!


Also add unit tests for machine-learning-api-client.

hiranya911

LGTM with a couple of nits 👍

hiranya911 · 2020-02-10T19:06:08Z

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

+  }
+
+
+  private getUrl(): Promise<string> {


Nit: I'd recommend pushing helpers like this to the bottom of the class, so that more important functionality like getResouce appears first.

hiranya911 · 2020-02-10T19:07:55Z

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

+        });
+    });
+
+    it('should throw when a full platform error response is received', () => {


should reject

Here and other similar tests.

* Firebase ML Node.js SDK Structure (#778) * Firebase ML Node.js SDK Structure * added tests * Added GetModel functionality and tests (#781) * Added GetModel functionality and tests * Added DeleteModel functionality and tests (#782) * Added DeleteModel functionality and tests * Added CreateModel functionality and tests (#788) * Added CreateModel functionality and tests * Added UpdateModel, publishModel,and unpublishModel functionality + tests (#791) * Added UpdateModel, publishModel,and unpublishModel functionality plus tests * Added ListModels functionality for Firebase ML (#795) * Added ListModels functionality for Firebase ML * Firebase ML changed endpoint (#813) * Add ML APIs to docgen toc (#847) * Docstring fixes and additions (#848) * Docstring fixes and additions * A few edits * Ml merge (#851) * Custom Action for sending Tweets (#784) * Experimental custom Action for sending Tweets * Added license headers * Added README file * Updated package descriptions * Improve customClaims Typing (#768) * chore: Experimental release flow based on Actions (#780) * chore: Experimental release flow based on Actions * Added tarball verification step; Simplified CI trigger * Splitting staging and publish phases into separate jobs * Fleshed out the full workflow * Trigger RC build * chore: Migrated to ESlint (#790) * chore: Migrated to ESlint * Added licesne header * Enabling additional ESLint checks (#794) * chore: Enabling more ESLint checks and fixing errors (#797) * Fix compilation error in integration tests (#798) Introduced by #790 * Build integration tests during CI (and release) (#800) Note that this won't actually run them. Additionally, the *unit* tests are also built, impying that we're building them twice (once during this step, and possibly again when running the unit tests.) * Fix revokeRefreshTokens to round consistently with the other platforms. (#801) This also makes it consistent with the comments a few lines above, as well as the integration test. * feat(auth): Multi-factor Auth support with SMS for Google Cloud Identity Platform (#804) Defines multi-factor auth administrative APIs for Google Cloud Identity Platform. * Defines new MFA types in toc.yaml. (#807) * Removes special char from index.d.ts. (#808) This is causing errors in the reference generation process. * Defines MultiFactor{Create|Update}Settings interfaces. (#809) * Defines MultiFactor{Create|Update}Settings interfaces. * chore: Adding a .npmrc file to the root of the repo (#810) * chore: Adding a .npmrc file to the root of the repo * Removing the root-level .npmrc file * [chore] Release 8.10.0 (#811) * [chore] Release 8.10.0 (take 2) (#812) * Bump acorn from 6.1.1 to 6.4.1 (#815) Bumps [acorn](https://github.com/acornjs/acorn) from 6.1.1 to 6.4.1. - [Release notes](https://github.com/acornjs/acorn/releases) - [Commits](acornjs/acorn@6.1.1...6.4.1) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fixing Android notification options descriptions (#820) * Fixing doc bug that conflated sound and tag options for Android notifications. * Removing duplicate documentation for tag. * Adding tag details in the right place this time, hopefully. * chore: Splitting the index.d.ts file into smaller files (#751) * Splitting the index.d.ts file into smaller files * Database return type fixed * chore: Cleaning up package verification scripts (#822) * chore: Cleaning up package verification scripts * Added package metadata to test package.json file * fix(auth): Fixing UserImportRecord typings declaration (#835) * fix(auth): Fixing UserImportRecord typings declaration * Fixing more integration test compilation errors * Trigger CI * Removed redundant line * Bump minimist from 1.2.0 to 1.2.3 (#839) Bumps [minimist](https://github.com/substack/minimist) from 1.2.0 to 1.2.3. - [Release notes](https://github.com/substack/minimist/releases) - [Commits](https://github.com/substack/minimist/compare/1.2.0...1.2.3) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remerge conflict Co-authored-by: Hiranya Jayathilaka <[email protected]> Co-authored-by: William Sedlacek <[email protected]> Co-authored-by: rsgowman <[email protected]> Co-authored-by: bojeil-google <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: egilmorez <[email protected]> * Added Firebase ML API requirements to Contributing doc (#853) Co-authored-by: Kevin Cheung <[email protected]> Co-authored-by: Hiranya Jayathilaka <[email protected]> Co-authored-by: William Sedlacek <[email protected]> Co-authored-by: rsgowman <[email protected]> Co-authored-by: bojeil-google <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: egilmorez <[email protected]>

Added GetModel functionality and tests

5bf7a12

ifielker requested a review from hiranya911 February 8, 2020 00:34

ifielker assigned hiranya911 Feb 8, 2020

hiranya911 suggested changes Feb 8, 2020

View reviewed changes

hiranya911 assigned ifielker and unassigned hiranya911 Feb 8, 2020

added apiClient tests

f4ff6e9

ifielker assigned hiranya911 Feb 8, 2020

hiranya911 approved these changes Feb 10, 2020

View reviewed changes

hiranya911 removed their assignment Feb 10, 2020

review suggestions

ef69f32

ifielker merged commit 3f2ec51 into ml Feb 10, 2020

ifielker deleted the ml-2 branch February 10, 2020 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added GetModel functionality and tests #781

Added GetModel functionality and tests #781

ifielker commented Feb 8, 2020

hiranya911 left a comment

hiranya911 Feb 8, 2020

ifielker Feb 8, 2020

hiranya911 Feb 8, 2020

ifielker Feb 8, 2020

hiranya911 Feb 8, 2020

ifielker Feb 8, 2020

hiranya911 Feb 8, 2020

ifielker Feb 8, 2020

hiranya911 Feb 8, 2020

ifielker Feb 8, 2020

hiranya911 Feb 8, 2020

ifielker Feb 8, 2020

hiranya911 Feb 8, 2020

ifielker Feb 8, 2020

hiranya911 left a comment

hiranya911 Feb 10, 2020

ifielker Feb 10, 2020

hiranya911 Feb 10, 2020

ifielker Feb 10, 2020


		import { PrefixedFirebaseError } from '../utils/error';

		export type MachineLearningErrorCode =

Added GetModel functionality and tests #781

Added GetModel functionality and tests #781

Conversation

ifielker commented Feb 8, 2020

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment