Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ListModels functionality for Firebase ML #795

Merged
merged 5 commits into from
Feb 28, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5208,7 +5208,7 @@ declare namespace admin.machineLearning {
* Interface representing options for listing Models.
*/
interface ListModelOptions {
listFilter?: string;
filter?: string;
pageSize?: number;
pageToken?: string;
}
Expand Down Expand Up @@ -5322,7 +5322,7 @@ declare namespace admin.machineLearning {
* token. For the last page, an empty list of models and no page token
* are returned.
*/
listModels(options: ListModelOptions): Promise<ListModelsResult>;
listModels(options?: ListModelOptions): Promise<ListModelsResult>;

/**
* Deletes a model from Firebase ML.
Expand Down
37 changes: 37 additions & 0 deletions src/machine-learning/machine-learning-api-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ export interface ModelUpdateOptions extends ModelOptions {
state?: { published?: boolean; };
}

/** Interface representing listModels options. */
export interface ListModelsOptions {
hiranya911 marked this conversation as resolved.
Show resolved Hide resolved
filter?: string;
pageSize?: number;
pageToken?: string;
}

export interface ModelContent {
readonly displayName?: string;
readonly tags?: string[];
Expand All @@ -66,6 +73,11 @@ export interface ModelResponse extends ModelContent {
readonly modelHash?: string;
}

export interface ListModelsResponse {
readonly models?: ModelResponse[];
readonly nextPageToken?: string;
}

export interface OperationResponse {
readonly name?: string;
readonly done: boolean;
Expand Down Expand Up @@ -140,6 +152,31 @@ export class MachineLearningApiClient {
});
}

public listModels(options: ListModelsOptions = {}): Promise<ListModelsResponse> {
if (typeof options.filter !== 'undefined' && !validator.isNonEmptyString(options.filter)) {
hiranya911 marked this conversation as resolved.
Show resolved Hide resolved
const err = new FirebaseMachineLearningError('invalid-argument', 'Invalid list filter.');
return Promise.reject(err);
}
if (typeof options.pageSize !== 'undefined' && !validator.isNumber(options.pageSize)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a cap on the pageSize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const err = new FirebaseMachineLearningError('invalid-argument', 'Invalid page size.');
return Promise.reject(err);
}
if (typeof options.pageToken !== 'undefined' && !validator.isNonEmptyString(options.pageToken)) {
const err = new FirebaseMachineLearningError(
'invalid-argument', 'Next page token must be a non-empty string.');
return Promise.reject(err);
}
return this.getUrl()
.then((url) => {
const request: HttpRequestConfig = {
method: 'GET',
url: `${url}/models`,
data: options,
};
return this.sendRequest<ListModelsResponse>(request);
});
}

public deleteModel(modelId: string): Promise<void> {
return this.getUrl()
.then((url) => {
Expand Down
30 changes: 19 additions & 11 deletions src/machine-learning/machine-learning.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import {FirebaseApp} from '../firebase-app';
import {FirebaseServiceInterface, FirebaseServiceInternalsInterface} from '../firebase-service';
import {MachineLearningApiClient, ModelResponse, OperationResponse,
ModelOptions, ModelUpdateOptions} from './machine-learning-api-client';
ModelOptions, ModelUpdateOptions, ListModelsOptions} from './machine-learning-api-client';
import {FirebaseError} from '../utils/error';

import * as validator from '../utils/validator';
Expand All @@ -41,13 +41,6 @@ class MachineLearningInternals implements FirebaseServiceInternalsInterface {
}
}

/** Interface representing listModels options. */
export interface ListModelsOptions {
listFilter?: string;
pageSize?: number;
pageToken?: string;
}

/** Response object for a listModels operation. */
export interface ListModelsResult {
models: Model[];
Expand Down Expand Up @@ -161,8 +154,24 @@ export class MachineLearning implements FirebaseServiceInterface {
* token. For the last page, an empty list of models and no page token are
* returned.
*/
public listModels(options: ListModelsOptions): Promise<ListModelsResult> {
throw new Error('NotImplemented');
public listModels(options: ListModelsOptions = {}): Promise<ListModelsResult> {
return this.client.listModels(options)
.then((resp) => {
if (resp == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

throw new FirebaseMachineLearningError(
'invalid-argument',
`Invalid ListModels response: ${JSON.stringify(resp)}`);
}
let models: Model[] = [];
if (resp && resp.models) {
models = resp.models.map((rs) => new Model(rs));
}
const result: ListModelsResult = {models};
if (resp && resp.nextPageToken) {
result.pageToken = resp.nextPageToken;
}
return result;
});
}

/**
Expand Down Expand Up @@ -268,7 +277,6 @@ export class Model {
sizeBytes: model.tfliteModel.sizeBytes,
};
}

}

public get locked(): boolean {
Expand Down
120 changes: 119 additions & 1 deletion test/integration/machine-learning.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ describe('admin.machineLearning', () => {
});

it('updates the tflite file', () => {
Promise.all([
return Promise.all([
createTemporaryModel(),
uploadModelToGcs('model1.tflite', 'valid_model.tflite')])
.then(([model, fileName]) => {
Expand Down Expand Up @@ -324,6 +324,124 @@ describe('admin.machineLearning', () => {
});
});

describe('listModels()', () => {
hiranya911 marked this conversation as resolved.
Show resolved Hide resolved
it('resolves with a list of models', () => {
return Promise.all([
createTemporaryModel({displayName: 'node-integration-list1a'}),
createTemporaryModel({displayName: 'node-integration-list1b'}),
])
.then(([model1, model2]) => {
return admin.machineLearning().listModels({pageSize: 100})
.then((modelList) => {
expect(modelList.models.length).to.be.at.least(2);
expect(modelList.models).to.deep.include(model1);
expect(modelList.models).to.deep.include(model2);
expect(modelList.pageToken).to.be.empty;
});
});
});

it('respects page size', () => {
return Promise.all([
createTemporaryModel({displayName: 'node-integration-list2a'}),
createTemporaryModel({displayName: 'node-integration-list2b'}),
createTemporaryModel({displayName: 'node-integration-list2c'}),
])
.then(([]) => {
return admin.machineLearning().listModels({pageSize: 2})
.then((modelList) => {
expect(modelList.models.length).to.equal(2);
expect(modelList.pageToken).not.to.be.empty;
});
});
});

it('filters by exact displayName', () => {
return createTemporaryModel({displayName: 'node-integration-list3'})
.then((model) => {
return admin.machineLearning().listModels({filter: 'displayName=node-integration-list3'})
.then((modelList) => {
expect(modelList.models.length).to.equal(1);
expect(modelList.models[0]).to.deep.equal(model);
expect(modelList.pageToken).to.be.empty;
});
});
});

it('filters by displayName prefix', () => {
return Promise.all([
createTemporaryModel({displayName: 'node-integration-list4a'}),
createTemporaryModel({displayName: 'node-integration-list4b'}),
createTemporaryModel({displayName: 'node-integration-list4c'}),
])
.then(([model1, model2, model3]) => {
return admin.machineLearning().listModels({filter: 'displayName:node-integration-list4*', pageSize: 100})
.then((modelList) => {
expect(modelList.models.length).to.be.at.least(3);
expect(modelList.models).to.deep.include(model1);
expect(modelList.models).to.deep.include(model2);
expect(modelList.models).to.deep.include(model3);
expect(modelList.pageToken).to.be.empty;
});
});
});

it('filters by tag', () => {
return Promise.all([
createTemporaryModel({displayName: 'node-integration-list5a', tags: ['node-integration-tag5']}),
createTemporaryModel({displayName: 'node-integration-list5b', tags: ['node-integration-tag5']}),
createTemporaryModel({displayName: 'node-integration-list5c', tags: ['node-integration-tag5']}),
])
.then(([model1, model2, model3]) => {
return admin.machineLearning().listModels({filter: 'tags:node-integration-tag5', pageSize: 100})
.then((modelList) => {
expect(modelList.models.length).to.be.at.least(3);
expect(modelList.models).to.deep.include(model1);
expect(modelList.models).to.deep.include(model2);
expect(modelList.models).to.deep.include(model3);
expect(modelList.pageToken).to.be.empty;
});
});
});

it('handles pageTokens properly', () => {
return Promise.all([
createTemporaryModel({displayName: 'node-integration-list6a'}),
createTemporaryModel({displayName: 'node-integration-list6b'}),
createTemporaryModel({displayName: 'node-integration-list6c'}),
])
.then(([]) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just then(() =>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return admin.machineLearning().listModels({filter: 'displayName:node-integration-list6*', pageSize: 2})
.then((modelList) => {
expect(modelList.models.length).to.equal(2);
expect(modelList.pageToken).not.to.be.empty;
return admin.machineLearning().listModels({
filter: 'displayName:node-integration-list6*',
pageSize: 2,
pageToken: modelList.pageToken})
.then((modelList2) => {
expect(modelList2.models.length).to.be.at.least(1);
expect(modelList2.pageToken).to.be.empty;
});
});
});
});

it('successfully returns an empty list of models', () => {
return admin.machineLearning().listModels({filter: 'displayName=non-existing-model'})
.then((modelList) => {
expect(modelList.models.length).to.equal(0);
expect(modelList.pageToken).to.be.empty;
});
});

it('rejects with invalid argument if the filter is invalid', () => {
return admin.machineLearning().listModels({filter: 'invalidFilterItem=foo'})
.should.eventually.be.rejected.and.have.property(
'code', 'machine-learning/invalid-argument');
});
});

describe('deleteModel()', () => {
it('rejects with not-found when the Model does not exist', () => {
const nonExistingName = '00000000';
Expand Down
Loading