Added CreateModel functionality and tests #788

ifielker · 2020-02-18T17:27:03Z

Added CreateModel functionality and tests

hiranya911

Looks mostly good. Encountered a couple of broken promise chains, and please access Storage APIs via FirebaseApp to avoid duplication.

src/machine-learning/machine-learning.ts

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

test/integration/machine-learning.spec.ts

hiranya911

Pretty good. Just some feedback on naming and deferring the call admin.storage() until it's really needed.

src/machine-learning/machine-learning.ts

hiranya911 · 2020-02-19T20:54:11Z

src/machine-learning/machine-learning.ts

@@ -213,8 +173,52 @@ export class MachineLearning implements FirebaseServiceInterface {
  public deleteModel(modelId: string): Promise<void> {
    return this.client.deleteModel(modelId);
  }
+
+  private convertOptionstoContent(options: ModelOptions, forUpload?: boolean): Promise<object> {


Promise<ModelContent>

src/machine-learning/machine-learning.ts

test/integration/machine-learning.spec.ts

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

hiranya911 · 2020-02-19T21:05:30Z

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

-      const expectedError = 'Failed to initialize ML client with the available ' +
-          'credential. Must initialize the SDK with a certificate credential or application default ' +
-          'credentials to use Firebase ML API.';
+      const expectedError = 'Failed to initialize Google Cloud Storage client with ' +


Once you defer calling admin.storage(), I'd expect this error to not occur until createModel() is called.

hiranya911

LGTM with a couple of suggestions.

hiranya911 · 2020-02-19T23:37:45Z

src/machine-learning/machine-learning.ts

@@ -187,11 +184,10 @@ export class MachineLearning implements FirebaseServiceInterface {
          throw new FirebaseMachineLearningError(
            'internal-error',
            `Error during signing upload url: ${err.message}`);
-
-        });
+        }) as Promise<ModelContent>;


This is kind of weird. Just define your constant to be ModelContent.

const modelContent = deepCopy(options) as ModelContent;

If I do that I wouldn't be able to assign to it here: modelContent.tfliteModel!.gcsTfliteUri = uri;
because all the ModelContent properties are read only.
(That's why I initially left it as Promise<object>)

hiranya911 · 2020-02-19T23:38:08Z

src/machine-learning/machine-learning.ts

    }

-    return Promise.resolve(modelContent);
+    return Promise.resolve(modelContent) as Promise<ModelContent>;


If you type modelContent you won't need the cast here.

As above. I need to cast at the end after all the assignments are complete.

hiranya911 · 2020-02-19T23:39:22Z

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

@@ -161,16 +159,6 @@ describe('MachineLearning', () => {
              + 'instance.');
    });



Does it make sense to keep this test, but just update it to call createModel()?

* 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] <support@github.com> 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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * remerge conflict Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com> Co-authored-by: William Sedlacek <wsedlacekc@gmail.com> Co-authored-by: rsgowman <rgowman@google.com> Co-authored-by: bojeil-google <bojeil-google@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: egilmorez <egilmore@google.com> * Added Firebase ML API requirements to Contributing doc (#853) Co-authored-by: Kevin Cheung <kevinthecheung@users.noreply.github.com> Co-authored-by: Hiranya Jayathilaka <hiranya911@gmail.com> Co-authored-by: William Sedlacek <wsedlacekc@gmail.com> Co-authored-by: rsgowman <rgowman@google.com> Co-authored-by: bojeil-google <bojeil-google@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: egilmorez <egilmore@google.com>

Added CreateModel functionality and tests

8a8c5a8

ifielker requested a review from hiranya911 February 18, 2020 17:29

ifielker assigned hiranya911 Feb 18, 2020

hiranya911 suggested changes Feb 18, 2020

View reviewed changes

hiranya911 assigned ifielker and unassigned hiranya911 Feb 18, 2020

ifielker added 2 commits February 18, 2020 18:39

debugging

bfc7fc9

review comments

f266074

ifielker assigned hiranya911 Feb 19, 2020

hiranya911 suggested changes Feb 19, 2020

View reviewed changes

hiranya911 removed their assignment Feb 19, 2020

review comments 2

7f847fe

ifielker assigned hiranya911 Feb 19, 2020

hiranya911 approved these changes Feb 19, 2020

View reviewed changes

hiranya911 removed their assignment Feb 20, 2020

review comments 3

04b2e16

ifielker merged commit 27f4fb2 into ml Feb 20, 2020

ifielker deleted the ml-4 branch February 21, 2020 03:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added CreateModel functionality and tests #788

Added CreateModel functionality and tests #788

ifielker commented Feb 18, 2020

hiranya911 left a comment

hiranya911 left a comment

hiranya911 Feb 19, 2020

ifielker Feb 19, 2020

hiranya911 Feb 19, 2020

ifielker Feb 19, 2020

hiranya911 left a comment

hiranya911 Feb 19, 2020

ifielker Feb 20, 2020 •

edited

Loading

hiranya911 Feb 19, 2020

ifielker Feb 20, 2020

hiranya911 Feb 19, 2020

ifielker Feb 20, 2020

		@@ -161,16 +159,6 @@ describe('MachineLearning', () => {
		+ 'instance.');
		});

Added CreateModel functionality and tests #788

Added CreateModel functionality and tests #788

Conversation

ifielker commented Feb 18, 2020

hiranya911 left a 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

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifielker Feb 20, 2020 • edited Loading

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

ifielker Feb 20, 2020 •

edited

Loading