Added ListModels functionality for Firebase ML #795

ifielker · 2020-02-27T19:37:20Z

Added ListModels functionality for Firebase ML

hiranya911

Looks pretty solid. Just a few nits and one suggestion on integration tests.

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

hiranya911 · 2020-02-27T20:53:01Z

src/machine-learning/machine-learning.ts

+ public listModels(options: ListModelsOptions = {}): Promise<ListModelsResult> {
+ return this.client.listModels(options)
+ .then((resp) => {
+ if (resp == null) {


Check for validator.isNonNullObject(resp) here. Then you can simplify the subsequent checks to just if (resp.models) and if (resp.nextPageToken).

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

hiranya911 · 2020-02-27T21:30:35Z

test/integration/machine-learning.spec.ts

+ createTemporaryModel({displayName: 'node-integration-list6b'}),
+ createTemporaryModel({displayName: 'node-integration-list6c'}),
+ ])
+ .then(([]) => {


Just then(() =>

hiranya911 · 2020-02-27T21:32:24Z

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

+ const err = new FirebaseMachineLearningError('invalid-argument', 'Invalid list filter.');
+ return Promise.reject(err);
+ }
+ if (typeof options.pageSize !== 'undefined' && !validator.isNumber(options.pageSize)) {


Put a cap on the pageSize.

There's a cap on the number of models people are allowed to create, so it automatically functions as a pageSize cap too. Is that sufficient?

We usually put an explicit cap on all list operations. This is documented and the inputs are checked and rejected in the SDK if they exceed the cap. I think we need something similar here. You can use the same underlying cap as the backend API.

test/integration/machine-learning.spec.ts

hiranya911 · 2020-02-27T21:38:27Z

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

@@ -190,6 +227,26 @@ describe('MachineLearning', () => {
 });
 });

+ describe('model', () => {


hiranya911

LGTM 👍

Please add the cap on pageSize and a test case.

hiranya911 · 2020-02-27T23:22:04Z

test/integration/machine-learning.spec.ts

+ let model3: admin.machineLearning.Model;
+
+ before(() => {
+ return Promise.all([


Loop might be slightly more readable here:

const promises = []; for (let i = 0; i < 3; i++) { promises.push(...); } return Promise.all(promises);

But it's your call.

* 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 ListModels functionality for Firebase ML

973585e

ifielker assigned hiranya911 Feb 27, 2020

fixed tests

1af8727

hiranya911 suggested changes Feb 27, 2020

View reviewed changes

hiranya911 assigned ifielker and unassigned hiranya911 Feb 27, 2020

ifielker added 2 commits February 27, 2020 17:56

review changes

8d9e0ed

review changes 2

3b91a58

ifielker assigned hiranya911 Feb 27, 2020

hiranya911 approved these changes Feb 27, 2020

View reviewed changes

hiranya911 removed their assignment Feb 27, 2020

adding pageSize limit

1624c12

ifielker merged commit eabe9ba into ml Feb 28, 2020

ifielker deleted the ml-6 branch February 28, 2020 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ListModels functionality for Firebase ML #795

Added ListModels functionality for Firebase ML #795

ifielker commented Feb 27, 2020

hiranya911 left a comment

hiranya911 Feb 27, 2020

ifielker Feb 27, 2020

hiranya911 Feb 27, 2020

ifielker Feb 27, 2020

hiranya911 Feb 27, 2020

ifielker Feb 27, 2020

hiranya911 Feb 27, 2020

hiranya911 Feb 27, 2020

ifielker Feb 27, 2020

hiranya911 left a comment

hiranya911 Feb 27, 2020

Added ListModels functionality for Firebase ML #795

Added ListModels functionality for Firebase ML #795

Conversation

ifielker commented Feb 27, 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

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment