From 8a8c5a89c8be7a9ccfbc1ab3d84d981a6f4a3120 Mon Sep 17 00:00:00 2001 From: ifielker Date: Tue, 18 Feb 2020 12:25:55 -0500 Subject: [PATCH 1/5] Added CreateModel functionality and tests --- src/index.d.ts | 8 +- .../machine-learning-api-client.ts | 25 +++ .../machine-learning-utils.ts | 32 ++- src/machine-learning/machine-learning.ts | 123 ++++++++++- test/integration/machine-learning.spec.ts | 157 ++++++++++++++ test/resources/invalid_model.tflite | 1 + test/resources/model1.tflite | Bin 0 -> 736 bytes .../machine-learning/machine-learning.spec.ts | 203 ++++++++++++++++-- 8 files changed, 521 insertions(+), 28 deletions(-) create mode 100644 test/resources/invalid_model.tflite create mode 100644 test/resources/model1.tflite diff --git a/src/index.d.ts b/src/index.d.ts index 6c61e32b48..6c10e3bec1 100755 --- a/src/index.d.ts +++ b/src/index.d.ts @@ -5235,9 +5235,7 @@ declare namespace admin.machineLearning { displayName?: string; tags?: string[]; - tfLiteModel?: {gcsTFLiteUri: string;}; - - toJSON(forUpload?: boolean): object; + tfliteModel?: {gcsTfliteUri: string;}; } /** @@ -5247,8 +5245,8 @@ declare namespace admin.machineLearning { readonly modelId: string; readonly displayName: string; readonly tags?: string[]; - readonly createTime: number; - readonly updateTime: number; + readonly createTime: string; + readonly updateTime: string; readonly validationError?: string; readonly published: boolean; readonly etag: string; diff --git a/src/machine-learning/machine-learning-api-client.ts b/src/machine-learning/machine-learning-api-client.ts index 8331924371..295014fc01 100644 --- a/src/machine-learning/machine-learning-api-client.ts +++ b/src/machine-learning/machine-learning-api-client.ts @@ -52,6 +52,13 @@ export interface ModelResponse extends ModelContent { readonly modelHash?: string; } +export interface OperationResponse { + readonly name?: string; + readonly done: boolean; + readonly error?: StatusErrorResponse; + readonly response?: ModelResponse; +} + /** * Class that facilitates sending requests to the Firebase ML backend API. @@ -73,6 +80,24 @@ export class MachineLearningApiClient { this.httpClient = new AuthorizedHttpClient(app); } + public createModel(model: ModelContent): Promise { + if (!validator.isNonNullObject(model) || + !validator.isNonEmptyString(model.displayName)) { + const err = new FirebaseMachineLearningError('invalid-argument', 'Invalid model content.'); + return Promise.reject(err); + } + return this.getUrl() + .then((url) => { + const request: HttpRequestConfig = { + method: 'POST', + url: `${url}/models`, + data: model, + }; + return this.sendRequest(request); + }); + } + + public getModel(modelId: string): Promise { return Promise.resolve() .then(() => { diff --git a/src/machine-learning/machine-learning-utils.ts b/src/machine-learning/machine-learning-utils.ts index 0b4f84a64f..d0b881621a 100644 --- a/src/machine-learning/machine-learning-utils.ts +++ b/src/machine-learning/machine-learning-utils.ts @@ -25,9 +25,39 @@ export type MachineLearningErrorCode = | 'not-found' | 'resource-exhausted' | 'service-unavailable' - | 'unknown-error'; + | 'unknown-error' + | 'cancelled' + | 'deadline-exceeded' + | 'permission-denied' + | 'failed-precondition' + | 'aborted' + | 'out-of-range' + | 'data-loss' + | 'unauthenticated'; export class FirebaseMachineLearningError extends PrefixedFirebaseError { + public static fromOperationError(code: number, message: string): FirebaseMachineLearningError { + switch (code) { + case 1: return new FirebaseMachineLearningError('cancelled', message); + case 2: return new FirebaseMachineLearningError('unknown-error', message); + case 3: return new FirebaseMachineLearningError('invalid-argument', message); + case 4: return new FirebaseMachineLearningError('deadline-exceeded', message); + case 5: return new FirebaseMachineLearningError('not-found', message); + case 6: return new FirebaseMachineLearningError('already-exists', message); + case 7: return new FirebaseMachineLearningError('permission-denied', message); + case 8: return new FirebaseMachineLearningError('resource-exhausted', message); + case 9: return new FirebaseMachineLearningError('failed-precondition', message); + case 10: return new FirebaseMachineLearningError('aborted', message); + case 11: return new FirebaseMachineLearningError('out-of-range', message); + case 13: return new FirebaseMachineLearningError('internal-error', message); + case 14: return new FirebaseMachineLearningError('service-unavailable', message); + case 15: return new FirebaseMachineLearningError('data-loss', message); + case 16: return new FirebaseMachineLearningError('unauthenticated', message); + default: + return new FirebaseMachineLearningError('unknown-error', message); + } + } + constructor(code: MachineLearningErrorCode, message: string) { super('machine-learning', code, message); } diff --git a/src/machine-learning/machine-learning.ts b/src/machine-learning/machine-learning.ts index cb4ae0cf6a..0d1de4a2ba 100644 --- a/src/machine-learning/machine-learning.ts +++ b/src/machine-learning/machine-learning.ts @@ -14,16 +14,18 @@ * limitations under the License. */ - +import {Storage as StorageClient} from '@google-cloud/storage'; import {FirebaseApp} from '../firebase-app'; import {FirebaseServiceInterface, FirebaseServiceInternalsInterface} from '../firebase-service'; -import {MachineLearningApiClient, ModelResponse} from './machine-learning-api-client'; +import {ServiceAccountCredential, isApplicationDefault} from '../auth/credential'; +import {MachineLearningApiClient, + ModelResponse, OperationResponse, StatusErrorResponse} from './machine-learning-api-client'; import {FirebaseError} from '../utils/error'; +import * as utils from '../utils/index'; import * as validator from '../utils/validator'; import {FirebaseMachineLearningError} from './machine-learning-utils'; - -// const ML_HOST = 'mlkit.googleapis.com'; +import { deepCopy } from '../utils/deep-copy'; /** * Internals of an ML instance. @@ -62,6 +64,7 @@ export class MachineLearning implements FirebaseServiceInterface { private readonly client: MachineLearningApiClient; private readonly appInternal: FirebaseApp; + private readonly storageClient: StorageClient; /** * @param {FirebaseApp} app The app for this ML service. @@ -76,6 +79,44 @@ export class MachineLearning implements FirebaseServiceInterface { }); } + let storage: typeof StorageClient; + try { + storage = require('@google-cloud/storage').Storage; + } catch (err) { + throw new FirebaseError({ + code: 'ml/missing-dependencies', + message: + 'Failed to import the Cloud Storage client library for Node.js. ' + + 'Make sure to install the "@google-cloud/storage" npm package. ' + + `Original error: ${err}`, + }); + } + + const projectId: string | null = utils.getExplicitProjectId(app); + const credential = app.options.credential; + if (credential instanceof ServiceAccountCredential) { + this.storageClient = new storage({ + // When the SDK is initialized with ServiceAccountCredentials an + // explicit projectId is guaranteed to be available. + projectId: projectId!, + credentials: { + private_key: credential.privateKey, + client_email: credential.clientEmail, + }, + }); + } else if (isApplicationDefault(app.options.credential)) { + // Try to use the Google application default credentials. + this.storageClient = new storage(); + } else { + throw new FirebaseError({ + code: 'ml/invalid-credential', + message: + '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.', + }); + } + this.appInternal = app; this.client = new MachineLearningApiClient(app); } @@ -97,7 +138,9 @@ export class MachineLearning implements FirebaseServiceInterface { * @return {Promise} A Promise fulfilled with the created model. */ public createModel(model: ModelOptions): Promise { - throw new Error('NotImplemented'); + return convertOptionstoContent(model, true, this.storageClient) + .then((modelContent) => this.client.createModel(modelContent)) + .then((operation) => handleOperation(operation)); } /** @@ -173,7 +216,7 @@ export class MachineLearning implements FirebaseServiceInterface { } /** - * A Firebase ML Model output object + * A Firebase ML Model output object. */ export class Model { public readonly modelId: string; @@ -196,7 +239,7 @@ export class Model { !validator.isNonEmptyString(model.displayName) || !validator.isNonEmptyString(model.etag)) { throw new FirebaseMachineLearningError( - 'invalid-argument', + 'invalid-server-response', `Invalid Model response: ${JSON.stringify(model)}`); } @@ -252,13 +295,73 @@ export class ModelOptions { public displayName?: string; public tags?: string[]; - public tfliteModel?: { gcsTFLiteUri: string; }; + public tfliteModel?: { gcsTfliteUri: string; }; +} - protected toJSON(forUpload?: boolean): object { - throw new Error('NotImplemented'); +async function convertOptionstoContent( + options: ModelOptions, forUpload?: boolean, + storageClient?: StorageClient): Promise { + const modelContent = deepCopy(options); + if (forUpload && modelContent.tfliteModel?.gcsTfliteUri) { + if (!storageClient) { + throw new FirebaseMachineLearningError( + 'invalid-argument', + 'Must specify storage client if forUpload and gcs Uri are specified.', + ); + } + modelContent.tfliteModel.gcsTfliteUri = await signUrl(modelContent.tfliteModel.gcsTfliteUri, storageClient!); + } + return modelContent; +} + +async function signUrl(unsignedUrl: string, storageClient: StorageClient): Promise { + const MINUTES = 60 * 1000; // A minute in milliseconds. + const URL_VALID_DURATION = 10 * MINUTES; + + const gcsRegex = /^gs:\/\/([a-z0-9_.-]{3,63})\/(.+)$/; + const matches = gcsRegex.exec(unsignedUrl); + if (!matches) { + throw new FirebaseMachineLearningError( + 'invalid-argument', + `Invalid unsigned url: ${unsignedUrl}`); + } + const bucketName = matches[1]; + const blobName = matches[2]; + const bucket = storageClient.bucket(bucketName); + const blob = bucket.file(blobName); + + try { + const signedUrl = blob.getSignedUrl({ + action: 'read', + expires: Date.now() + URL_VALID_DURATION, + }).then((x) => x[0]); + return signedUrl; + } catch (err) { + throw new FirebaseMachineLearningError( + 'internal-error', + `Error during signing upload url: ${err.message}`, + ); } } function extractModelId(resourceName: string): string { return resourceName.split('/').pop()!; } + + +function handleOperation(op: OperationResponse): Model { + if (op.done) { + if (op.response) { + return new Model(op.response); + } else if (op.error) { + handleOperationError(op.error); + } + } + throw new FirebaseMachineLearningError( + 'invalid-server-response', + `Invalid Operation response: ${JSON.stringify(op)}`); +} + +function handleOperationError(err: StatusErrorResponse) { + throw FirebaseMachineLearningError.fromOperationError(err.code, err.message); +} diff --git a/test/integration/machine-learning.spec.ts b/test/integration/machine-learning.spec.ts index 1da8a69729..416b653df4 100644 --- a/test/integration/machine-learning.spec.ts +++ b/test/integration/machine-learning.spec.ts @@ -14,9 +14,117 @@ * limitations under the License. */ + +import path = require('path'); +import * as chai from 'chai'; import * as admin from '../../lib/index'; +import {Bucket} from '@google-cloud/storage'; + +const expect = chai.expect; describe('admin.machineLearning', () => { + + const modelsToDelete: string[] = []; + + function scheduleForDelete(model: admin.machineLearning.Model) { + modelsToDelete.push(model.modelId); + } + + function unscheduleForDelete(model: admin.machineLearning.Model) { + modelsToDelete.splice(modelsToDelete.indexOf(model.modelId), 1); + } + + function deleteTempModels(): Promise { + const promises: Array> = []; + modelsToDelete.forEach((modelId) => { + promises.push(admin.machineLearning().deleteModel(modelId)); + }); + modelsToDelete.splice(0, modelsToDelete.length); // Clear out the array. + return Promise.all(promises); + } + + function createTemporaryModel(options?: admin.machineLearning.ModelOptions) + : Promise { + let modelOptions: admin.machineLearning.ModelOptions = { + displayName: 'nodejs_integration_temp_model', + }; + if (options) { + modelOptions = options; + } + return admin.machineLearning().createModel(modelOptions) + .then((model) => { + scheduleForDelete(model); + return model; + }); + } + + function uploadModelToGcs(localFileName: string, gcsFileName: string): string { + const bucket: Bucket = admin.storage().bucket(); + const tfliteFileName = path.join(__dirname, `../resources/${localFileName}`); + bucket.upload(tfliteFileName, {destination: gcsFileName}); + return `gs://${bucket.name}/${gcsFileName}`; + } + + afterEach(() => { + return deleteTempModels(); + }); + + describe('createModel', () => { + it('creates a new Model without ModelFormat', () => { + const modelOptions: admin.machineLearning.ModelOptions = { + displayName: 'node-integration-test-create-1', + tags: ['tag123', 'tag345']}; + return admin.machineLearning().createModel(modelOptions) + .then((model) => { + scheduleForDelete(model); + verifyModel(model, modelOptions); + }); + }); + + it('creates a new Model with valid ModelFormat', () => { + // Upload a file to default gcs bucket + const gcsFileName = uploadModelToGcs('model1.tflite', 'valid_model.tflite'); + const modelOptions: admin.machineLearning.ModelOptions = { + displayName: 'node-integration-test-create-2', + tags: ['tag234', 'tag456'], + tfliteModel: { + gcsTfliteUri: gcsFileName, + }, + }; + return admin.machineLearning().createModel(modelOptions) + .then((model) => { + scheduleForDelete(model); + verifyModel(model, modelOptions); + }); + }); + + it('creates a new Model with invalid ModelFormat', () => { + // Upload a file to default gcs bucket + const gcsFileName = uploadModelToGcs('invalid_model.tflite', 'invalid_model.tflite'); + const modelOptions: admin.machineLearning.ModelOptions = { + displayName: 'node-integration-test-create-3', + tags: ['tag234', 'tag456'], + tfliteModel: { + gcsTfliteUri: gcsFileName, + }, + }; + return admin.machineLearning().createModel(modelOptions) + .then((model) => { + scheduleForDelete(model); + verifyModel(model, modelOptions); + }); + }); + + it ('rejects with invalid-argument when modelOptions are invalid', () => { + const modelOptions: admin.machineLearning.ModelOptions = { + displayName: 'Invalid Name#*^!', + }; + return admin.machineLearning().createModel(modelOptions) + .should.eventually.be.rejected.and.have.property('code', 'machine-learning/invalid-argument'); + }); + + }); + describe('getModel()', () => { it('rejects with not-found when the Model does not exist', () => { const nonExistingName = '00000000'; @@ -30,6 +138,16 @@ describe('admin.machineLearning', () => { .should.eventually.be.rejected.and.have.property( 'code', 'machine-learning/invalid-argument'); }); + + it('resolves with existing Model', () => { + return createTemporaryModel() + .then((expectedModel) => + admin.machineLearning().getModel(expectedModel.modelId) + .then((actualModel) => { + expect(actualModel).to.deep.equal(expectedModel); + }), + ); + }); }); describe('deleteModel()', () => { @@ -45,5 +163,44 @@ describe('admin.machineLearning', () => { .should.eventually.be.rejected.and.have.property( 'code', 'machine-learning/invalid-argument'); }); + + it('deletes existing Model', () => { + return createTemporaryModel().then((model) => { + return admin.machineLearning().deleteModel(model.modelId) + .then(() => { + return admin.machineLearning().getModel(model.modelId) + .should.eventually.be.rejected.and.have.property('code', 'machine-learning/not-found'); + }) + .then(() => { + unscheduleForDelete(model); // Already deleted. + }); + }); + }); }); + + function verifyModel(model: admin.machineLearning.Model, modelOptions: admin.machineLearning.ModelOptions) { + expect(model.displayName).to.equal(modelOptions.displayName); + expect(model.createTime).to.not.be.empty; + expect(model.updateTime).to.not.be.empty; + expect(model.etag).to.not.be.empty; + if (modelOptions.tags) { + expect(model.tags).to.deep.equal(modelOptions.tags); + } else { + expect(model.tags).to.be.empty; + } + if (modelOptions.tfliteModel) { + expect(model.tfliteModel!.gcsTfliteUri).to.equal(modelOptions.tfliteModel.gcsTfliteUri); + if (modelOptions.tfliteModel.gcsTfliteUri.endsWith('invalid_model.tflite')) { + expect(model.modelHash).to.be.empty; + expect(model.validationError).to.equal('Invalid flatbuffer format'); + } else { + expect(model.modelHash).to.not.be.empty; + expect(model.validationError).to.be.empty; + } + } else { + expect(model.validationError).to.equal('No model file has been uploaded.'); + } + expect(model.locked).to.be.false; + + } }); diff --git a/test/resources/invalid_model.tflite b/test/resources/invalid_model.tflite new file mode 100644 index 0000000000..d8482f4362 --- /dev/null +++ b/test/resources/invalid_model.tflite @@ -0,0 +1 @@ +This is not a tflite file. diff --git a/test/resources/model1.tflite b/test/resources/model1.tflite new file mode 100644 index 0000000000000000000000000000000000000000..c4b71b7a222ebc59ee9fa1239fe2b8efb382cf8b GIT binary patch literal 736 zcmaJY5FPb2r$h~!B1MWTQ%GV^!A6@CK}ZNlustqhi-Y8H-iIg%{+S@QcK(Dk zR)Rl3tSqc7Y;=9^?h;aY@NQ;j_RY-O-KvOmPg{E;TT&H6Oeso9%7|7F5m^Gpi-MRS zFR}v^fd$|pw*l-X(CyeA%O3exDvVXXE-Q$g0Ea*gAfI&%;7x1IS|70Au#+FH4KSD! zSQ8%k6Eu29ZW(^Feo)_K>{n|Oma%PM==n~V_^~%s4thu4$j6N3nHtV(0aQiaEoyRp zezXMpUIWy`Jtl%{m~A>Qxh()kG2`sRkJM$N(Aph1%|>7Ok%DczaXT3_&XwE0a6`}S z4OAy+#G&g)!6;Io$rCiN=X3S*IL!O-tl7r`=KFA-Gt`c~_y(?gfqS2GxQ`s3wFx?^MfSDns>_^X1%E{Y8Sb)GfRIXJ-KXm39D=`^W;!7eZm6%(eLy;H^LSfV^(T? zeSA40uL6|QKPM`rbs3=!d?x$wt<-YMb0LrVras*CGoXmIndd!cZ>NyH9V}M=02G>W Ai~s-t literal 0 HcmV?d00001 diff --git a/test/unit/machine-learning/machine-learning.spec.ts b/test/unit/machine-learning/machine-learning.spec.ts index bb0c5988f1..c172306559 100644 --- a/test/unit/machine-learning/machine-learning.spec.ts +++ b/test/unit/machine-learning/machine-learning.spec.ts @@ -19,10 +19,11 @@ import * as _ from 'lodash'; import * as chai from 'chai'; import * as sinon from 'sinon'; -import { MachineLearning } from '../../../src/machine-learning/machine-learning'; +import { MachineLearning, ModelOptions } from '../../../src/machine-learning/machine-learning'; import { FirebaseApp } from '../../../src/firebase-app'; import * as mocks from '../../resources/mocks'; -import { MachineLearningApiClient } from '../../../src/machine-learning/machine-learning-api-client'; +import { MachineLearningApiClient, + StatusErrorResponse, ModelResponse } from '../../../src/machine-learning/machine-learning-api-client'; import { FirebaseMachineLearningError } from '../../../src/machine-learning/machine-learning-utils'; import { deepCopy } from '../../../src/utils/deep-copy'; @@ -65,6 +66,56 @@ describe('MachineLearning', () => { }, }; + const STATUS_ERROR_RESPONSE: { + code: number; + message: string; + } = { + code: 3, + message: 'Invalid Argument message', + }; + + const OPERATION_RESPONSE: { + name?: string; + done: boolean; + error?: StatusErrorResponse; + response?: { + name: string; + createTime: string; + updateTime: string; + etag: string; + modelHash: string; + displayName?: string; + tags?: string[]; + state?: { + validationError?: { + code: number; + message: string; + }; + published?: boolean; + }; + tfliteModel?: { + gcsTfliteUri: string; + sizeBytes: number; + }; + } + } = { + done: true, + response: MODEL_RESPONSE, + }; + + const OPERATION_RESPONSE_ERROR: { + name?: string; + done: boolean; + error?: { + code: number; + message: string; + } + response?: ModelResponse; + } = { + done: true, + error: STATUS_ERROR_RESPONSE, + }; + const CREATE_TIME_UTC = 'Fri, 07 Feb 2020 23:45:23 GMT'; const UPDATE_TIME_UTC = 'Sat, 08 Feb 2020 23:45:23 GMT'; @@ -110,16 +161,14 @@ describe('MachineLearning', () => { + 'instance.'); }); - it('should reject when initialized without project ID', () => { - // Project ID not set in the environment. - delete process.env.GOOGLE_CLOUD_PROJECT; - delete process.env.GCLOUD_PROJECT; - const noProjectId = 'Failed to determine project ID. Initialize the SDK with service ' - + 'account credentials, or set project ID as an app option. Alternatively, set the ' - + 'GOOGLE_CLOUD_PROJECT environment variable.'; - const rulesWithoutProjectId = new MachineLearning(mockCredentialApp); - return rulesWithoutProjectId.getModel('test') - .should.eventually.rejectedWith(noProjectId); + it('should throw given invalid credential', () => { + 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.'; + expect(() => { + const machineLearningAny: any = MachineLearning; + return new machineLearningAny(mockCredentialApp); + }).to.throw(expectedError); }); it('should not throw given a valid app', () => { @@ -261,4 +310,134 @@ describe('MachineLearning', () => { return machineLearning.deleteModel('1234567'); }); }); + + describe('createModel', () => { + const GCS_TFLITE_URI = 'gs://test-bucket/Firebase/ML/Models/model1.tflite'; + const MODEL_OPTIONS_NO_GCS: ModelOptions = { + displayName: 'display_name', + tags: ['tag1', 'tag2'], + }; + const MODEL_OPTIONS_WITH_GCS: ModelOptions = { + displayName: 'display_name_2', + tags: ['tag3', 'tag4'], + tfliteModel: { + gcsTfliteUri: GCS_TFLITE_URI, + }, + }; + + it('should propagate API errors', () => { + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .rejects(EXPECTED_ERROR); + stubs.push(stub); + return machineLearning.createModel(MODEL_OPTIONS_NO_GCS) + .should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR); + }); + + it('should reject when API response is invalid', () => { + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(null); + stubs.push(stub); + return machineLearning.createModel(MODEL_OPTIONS_WITH_GCS) + .should.eventually.be.rejected.and.have.property( + 'message', 'Cannot read property \'done\' of null'); + }); + + it('should reject when API response does not contain a name', () => { + const op = deepCopy(OPERATION_RESPONSE); + op.response!.name = ''; + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(op); + stubs.push(stub); + return machineLearning.createModel(MODEL_OPTIONS_NO_GCS) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Model response: ${JSON.stringify(op.response)}`); + }); + + it('should reject when API response does not contain a createTime', () => { + const op = deepCopy(OPERATION_RESPONSE); + op.response!.createTime = ''; + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(op); + stubs.push(stub); + return machineLearning.createModel(MODEL_OPTIONS_NO_GCS) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Model response: ${JSON.stringify(op.response)}`); + }); + + it('should reject when API response does not contain a updateTime', () => { + const op = deepCopy(OPERATION_RESPONSE); + op.response!.updateTime = ''; + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(op); + stubs.push(stub); + return machineLearning.createModel(MODEL_OPTIONS_NO_GCS) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Model response: ${JSON.stringify(op.response)}`); + }); + + it('should reject when API response does not contain a displayName', () => { + const op = deepCopy(OPERATION_RESPONSE); + op.response!.displayName = ''; + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(op); + stubs.push(stub); + return machineLearning.createModel(MODEL_OPTIONS_NO_GCS) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Model response: ${JSON.stringify(op.response)}`); + }); + + it('should reject when API response does not contain an etag', () => { + const op = deepCopy(OPERATION_RESPONSE); + op.response!.etag = ''; + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(op); + stubs.push(stub); + return machineLearning.createModel(MODEL_OPTIONS_NO_GCS) + .should.eventually.be.rejected.and.have.property( + 'message', `Invalid Model response: ${JSON.stringify(op.response)}`); + }); + + it('should resolve with Model on success', () => { + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(OPERATION_RESPONSE); + stubs.push(stub); + + return machineLearning.createModel(MODEL_OPTIONS_WITH_GCS) + .then((model) => { + expect(model.modelId).to.equal('1234567'); + expect(model.displayName).to.equal('model_1'); + expect(model.tags).to.deep.equal(['tag_1', 'tag_2']); + expect(model.createTime).to.equal(CREATE_TIME_UTC); + expect(model.updateTime).to.equal(UPDATE_TIME_UTC); + expect(model.validationError).to.be.empty; + expect(model.published).to.be.true; + expect(model.etag).to.equal('etag123'); + expect(model.modelHash).to.equal('modelHash123'); + + const tflite = model.tfliteModel!; + expect(tflite.gcsTfliteUri).to.be.equal( + 'gs://test-project-bucket/Firebase/ML/Models/model1.tflite'); + expect(tflite.sizeBytes).to.be.equal(16900988); + }); + }); + + it('should resolve with Error on operation error', () => { + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'createModel') + .resolves(OPERATION_RESPONSE_ERROR); + stubs.push(stub); + + return machineLearning.createModel(MODEL_OPTIONS_WITH_GCS) + .should.eventually.be.rejected.and.have.property( + 'message', 'Invalid Argument message'); + }); + }); }); From bfc7fc9367d0807b1a2bd42f68a940f3ddebf6f4 Mon Sep 17 00:00:00 2001 From: ifielker Date: Tue, 18 Feb 2020 18:39:09 -0500 Subject: [PATCH 2/5] debugging --- .../machine-learning-api-client.ts | 7 + src/machine-learning/machine-learning.ts | 141 +++++++----------- test/integration/machine-learning.spec.ts | 46 +++--- .../machine-learning/machine-learning.spec.ts | 6 +- 4 files changed, 86 insertions(+), 114 deletions(-) diff --git a/src/machine-learning/machine-learning-api-client.ts b/src/machine-learning/machine-learning-api-client.ts index 295014fc01..c566ad5193 100644 --- a/src/machine-learning/machine-learning-api-client.ts +++ b/src/machine-learning/machine-learning-api-client.ts @@ -109,13 +109,16 @@ export class MachineLearningApiClient { } public deleteModel(modelId: string): Promise { + console.log(`DEBUGG: API going to delete ${modelId}`); return this.getUrl() .then((url) => { const modelName = this.getModelName(modelId); + console.log(`DEBUGG: Model Name is: ${modelName}`); const request: HttpRequestConfig = { method: 'DELETE', url: `${url}/${modelName}`, }; + console.log(`DEBUGG: sending request: ${JSON.stringify(request)}`); return this.sendRequest(request); }); } @@ -139,12 +142,16 @@ export class MachineLearningApiClient { } private sendRequest(request: HttpRequestConfig): Promise { + console.log('DEBUGG: sendREquest'); request.headers = FIREBASE_VERSION_HEADER; return this.httpClient.send(request) .then((resp) => { + console.log(`DEBUGG: Response is: ${JSON.stringify(resp)}`); + console.log(`DEBUGG: Response as T: ${JSON.stringify(resp.data as T)}`); return resp.data as T; }) .catch((err) => { + console.log(`DEBUGG: Error in sendREquest: ${JSON.stringify(err)}`); throw this.toFirebaseError(err); }); } diff --git a/src/machine-learning/machine-learning.ts b/src/machine-learning/machine-learning.ts index 0d1de4a2ba..e09080774c 100644 --- a/src/machine-learning/machine-learning.ts +++ b/src/machine-learning/machine-learning.ts @@ -14,15 +14,13 @@ * limitations under the License. */ -import {Storage as StorageClient} from '@google-cloud/storage'; import {FirebaseApp} from '../firebase-app'; import {FirebaseServiceInterface, FirebaseServiceInternalsInterface} from '../firebase-service'; -import {ServiceAccountCredential, isApplicationDefault} from '../auth/credential'; -import {MachineLearningApiClient, - ModelResponse, OperationResponse, StatusErrorResponse} from './machine-learning-api-client'; +import {Storage} from '../storage/storage'; +import {MachineLearningApiClient, ModelResponse, OperationResponse, + StatusErrorResponse} from './machine-learning-api-client'; import {FirebaseError} from '../utils/error'; -import * as utils from '../utils/index'; import * as validator from '../utils/validator'; import {FirebaseMachineLearningError} from './machine-learning-utils'; import { deepCopy } from '../utils/deep-copy'; @@ -64,7 +62,7 @@ export class MachineLearning implements FirebaseServiceInterface { private readonly client: MachineLearningApiClient; private readonly appInternal: FirebaseApp; - private readonly storageClient: StorageClient; + private readonly storage: Storage; /** * @param {FirebaseApp} app The app for this ML service. @@ -79,44 +77,7 @@ export class MachineLearning implements FirebaseServiceInterface { }); } - let storage: typeof StorageClient; - try { - storage = require('@google-cloud/storage').Storage; - } catch (err) { - throw new FirebaseError({ - code: 'ml/missing-dependencies', - message: - 'Failed to import the Cloud Storage client library for Node.js. ' + - 'Make sure to install the "@google-cloud/storage" npm package. ' + - `Original error: ${err}`, - }); - } - - const projectId: string | null = utils.getExplicitProjectId(app); - const credential = app.options.credential; - if (credential instanceof ServiceAccountCredential) { - this.storageClient = new storage({ - // When the SDK is initialized with ServiceAccountCredentials an - // explicit projectId is guaranteed to be available. - projectId: projectId!, - credentials: { - private_key: credential.privateKey, - client_email: credential.clientEmail, - }, - }); - } else if (isApplicationDefault(app.options.credential)) { - // Try to use the Google application default credentials. - this.storageClient = new storage(); - } else { - throw new FirebaseError({ - code: 'ml/invalid-credential', - message: - '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.', - }); - } - + this.storage = app.storage(); this.appInternal = app; this.client = new MachineLearningApiClient(app); } @@ -138,7 +99,7 @@ export class MachineLearning implements FirebaseServiceInterface { * @return {Promise} A Promise fulfilled with the created model. */ public createModel(model: ModelOptions): Promise { - return convertOptionstoContent(model, true, this.storageClient) + return this.convertOptionstoContent(model, true) .then((modelContent) => this.client.createModel(modelContent)) .then((operation) => handleOperation(operation)); } @@ -211,10 +172,55 @@ export class MachineLearning implements FirebaseServiceInterface { * @param {string} modelId The id of the model to delete. */ public deleteModel(modelId: string): Promise { + console.log(`DEBUGG: going to delete ${modelId}`); return this.client.deleteModel(modelId); } + + private convertOptionstoContent(options: ModelOptions, forUpload?: boolean): Promise { + const modelContent = deepCopy(options); + + if (forUpload && modelContent.tfliteModel?.gcsTfliteUri) { + return this.signUrl(modelContent.tfliteModel.gcsTfliteUri) + .then ((uri: string) => { + modelContent.tfliteModel!.gcsTfliteUri = uri; + return modelContent; + }) + .catch((err: Error) => { + throw new FirebaseMachineLearningError( + 'internal-error', + `Error during signing upload url: ${err.message}`); + + }); + } + + return Promise.resolve(modelContent); + } + + private signUrl(unsignedUrl: string): Promise { + const MINUTES_IN_MILLIS = 60 * 1000; + const URL_VALID_DURATION = 10 * MINUTES_IN_MILLIS; + + const gcsRegex = /^gs:\/\/([a-z0-9_.-]{3,63})\/(.+)$/; + const matches = gcsRegex.exec(unsignedUrl); + if (!matches) { + throw new FirebaseMachineLearningError( + 'invalid-argument', + `Invalid unsigned url: ${unsignedUrl}`); + } + const bucketName = matches[1]; + const blobName = matches[2]; + const bucket = this.storage.bucket(bucketName); + const blob = bucket.file(blobName); + return blob.getSignedUrl({ + action: 'read', + expires: Date.now() + URL_VALID_DURATION, + }).then((x) => { + return x[0] + }); + } } + /** * A Firebase ML Model output object. */ @@ -298,51 +304,6 @@ export class ModelOptions { public tfliteModel?: { gcsTfliteUri: string; }; } -async function convertOptionstoContent( - options: ModelOptions, forUpload?: boolean, - storageClient?: StorageClient): Promise { - const modelContent = deepCopy(options); - if (forUpload && modelContent.tfliteModel?.gcsTfliteUri) { - if (!storageClient) { - throw new FirebaseMachineLearningError( - 'invalid-argument', - 'Must specify storage client if forUpload and gcs Uri are specified.', - ); - } - modelContent.tfliteModel.gcsTfliteUri = await signUrl(modelContent.tfliteModel.gcsTfliteUri, storageClient!); - } - return modelContent; -} - -async function signUrl(unsignedUrl: string, storageClient: StorageClient): Promise { - const MINUTES = 60 * 1000; // A minute in milliseconds. - const URL_VALID_DURATION = 10 * MINUTES; - - const gcsRegex = /^gs:\/\/([a-z0-9_.-]{3,63})\/(.+)$/; - const matches = gcsRegex.exec(unsignedUrl); - if (!matches) { - throw new FirebaseMachineLearningError( - 'invalid-argument', - `Invalid unsigned url: ${unsignedUrl}`); - } - const bucketName = matches[1]; - const blobName = matches[2]; - const bucket = storageClient.bucket(bucketName); - const blob = bucket.file(blobName); - - try { - const signedUrl = blob.getSignedUrl({ - action: 'read', - expires: Date.now() + URL_VALID_DURATION, - }).then((x) => x[0]); - return signedUrl; - } catch (err) { - throw new FirebaseMachineLearningError( - 'internal-error', - `Error during signing upload url: ${err.message}`, - ); - } -} function extractModelId(resourceName: string): string { return resourceName.split('/').pop()!; diff --git a/test/integration/machine-learning.spec.ts b/test/integration/machine-learning.spec.ts index 416b653df4..83c4b208ce 100644 --- a/test/integration/machine-learning.spec.ts +++ b/test/integration/machine-learning.spec.ts @@ -58,14 +58,17 @@ describe('admin.machineLearning', () => { }); } - function uploadModelToGcs(localFileName: string, gcsFileName: string): string { + function uploadModelToGcs(localFileName: string, gcsFileName: string): Promise { const bucket: Bucket = admin.storage().bucket(); const tfliteFileName = path.join(__dirname, `../resources/${localFileName}`); - bucket.upload(tfliteFileName, {destination: gcsFileName}); - return `gs://${bucket.name}/${gcsFileName}`; + return bucket.upload(tfliteFileName, {destination: gcsFileName}) + .then(() => { + return `gs://${bucket.name}/${gcsFileName}`; + }); } afterEach(() => { + console.log('DEBUGG: Cleaning up.'); return deleteTempModels(); }); @@ -82,36 +85,37 @@ describe('admin.machineLearning', () => { }); it('creates a new Model with valid ModelFormat', () => { - // Upload a file to default gcs bucket - const gcsFileName = uploadModelToGcs('model1.tflite', 'valid_model.tflite'); const modelOptions: admin.machineLearning.ModelOptions = { displayName: 'node-integration-test-create-2', tags: ['tag234', 'tag456'], - tfliteModel: { - gcsTfliteUri: gcsFileName, - }, + tfliteModel: {gcsTfliteUri: 'this will be replaced below'}, }; - return admin.machineLearning().createModel(modelOptions) - .then((model) => { - scheduleForDelete(model); - verifyModel(model, modelOptions); - }); + return uploadModelToGcs('model1.tflite', 'valid_model.tflite') + .then((fileName: string) => { + modelOptions.tfliteModel!.gcsTfliteUri = fileName; + return admin.machineLearning().createModel(modelOptions) + .then((model) => { + scheduleForDelete(model); + verifyModel(model, modelOptions); + }); + }) }); it('creates a new Model with invalid ModelFormat', () => { // Upload a file to default gcs bucket - const gcsFileName = uploadModelToGcs('invalid_model.tflite', 'invalid_model.tflite'); const modelOptions: admin.machineLearning.ModelOptions = { displayName: 'node-integration-test-create-3', tags: ['tag234', 'tag456'], - tfliteModel: { - gcsTfliteUri: gcsFileName, - }, + tfliteModel: {gcsTfliteUri: 'this will be replaced below'}, }; - return admin.machineLearning().createModel(modelOptions) - .then((model) => { - scheduleForDelete(model); - verifyModel(model, modelOptions); + return uploadModelToGcs('invalid_model.tflite', 'invalid_model.tflite') + .then((fileName: string) => { + modelOptions.tfliteModel!.gcsTfliteUri = fileName; + return admin.machineLearning().createModel(modelOptions) + .then((model) => { + scheduleForDelete(model); + verifyModel(model, modelOptions); + }); }); }); diff --git a/test/unit/machine-learning/machine-learning.spec.ts b/test/unit/machine-learning/machine-learning.spec.ts index c172306559..5ed345a905 100644 --- a/test/unit/machine-learning/machine-learning.spec.ts +++ b/test/unit/machine-learning/machine-learning.spec.ts @@ -162,9 +162,9 @@ describe('MachineLearning', () => { }); it('should throw given invalid credential', () => { - 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 ' + + 'the available credential. Must initialize the SDK with a certificate credential ' + + 'or application default credentials to use Cloud Storage API.'; expect(() => { const machineLearningAny: any = MachineLearning; return new machineLearningAny(mockCredentialApp); From f266074dd252b230f05181d4c735939499668ae8 Mon Sep 17 00:00:00 2001 From: ifielker Date: Wed, 19 Feb 2020 15:46:07 -0500 Subject: [PATCH 3/5] review comments --- .../machine-learning-api-client.ts | 7 -- src/machine-learning/machine-learning.ts | 15 +-- test/integration/machine-learning.spec.ts | 37 +++--- .../machine-learning-api-client.spec.ts | 113 +++++++++++++++++- 4 files changed, 137 insertions(+), 35 deletions(-) diff --git a/src/machine-learning/machine-learning-api-client.ts b/src/machine-learning/machine-learning-api-client.ts index c566ad5193..295014fc01 100644 --- a/src/machine-learning/machine-learning-api-client.ts +++ b/src/machine-learning/machine-learning-api-client.ts @@ -109,16 +109,13 @@ export class MachineLearningApiClient { } public deleteModel(modelId: string): Promise { - console.log(`DEBUGG: API going to delete ${modelId}`); return this.getUrl() .then((url) => { const modelName = this.getModelName(modelId); - console.log(`DEBUGG: Model Name is: ${modelName}`); const request: HttpRequestConfig = { method: 'DELETE', url: `${url}/${modelName}`, }; - console.log(`DEBUGG: sending request: ${JSON.stringify(request)}`); return this.sendRequest(request); }); } @@ -142,16 +139,12 @@ export class MachineLearningApiClient { } private sendRequest(request: HttpRequestConfig): Promise { - console.log('DEBUGG: sendREquest'); request.headers = FIREBASE_VERSION_HEADER; return this.httpClient.send(request) .then((resp) => { - console.log(`DEBUGG: Response is: ${JSON.stringify(resp)}`); - console.log(`DEBUGG: Response as T: ${JSON.stringify(resp.data as T)}`); return resp.data as T; }) .catch((err) => { - console.log(`DEBUGG: Error in sendREquest: ${JSON.stringify(err)}`); throw this.toFirebaseError(err); }); } diff --git a/src/machine-learning/machine-learning.ts b/src/machine-learning/machine-learning.ts index e09080774c..7c87013dae 100644 --- a/src/machine-learning/machine-learning.ts +++ b/src/machine-learning/machine-learning.ts @@ -17,8 +17,7 @@ import {FirebaseApp} from '../firebase-app'; import {FirebaseServiceInterface, FirebaseServiceInternalsInterface} from '../firebase-service'; import {Storage} from '../storage/storage'; -import {MachineLearningApiClient, ModelResponse, OperationResponse, - StatusErrorResponse} from './machine-learning-api-client'; +import {MachineLearningApiClient, ModelResponse, OperationResponse} from './machine-learning-api-client'; import {FirebaseError} from '../utils/error'; import * as validator from '../utils/validator'; @@ -172,7 +171,6 @@ export class MachineLearning implements FirebaseServiceInterface { * @param {string} modelId The id of the model to delete. */ public deleteModel(modelId: string): Promise { - console.log(`DEBUGG: going to delete ${modelId}`); return this.client.deleteModel(modelId); } @@ -215,7 +213,7 @@ export class MachineLearning implements FirebaseServiceInterface { action: 'read', expires: Date.now() + URL_VALID_DURATION, }).then((x) => { - return x[0] + return x[0]; }); } } @@ -311,18 +309,17 @@ function extractModelId(resourceName: string): string { function handleOperation(op: OperationResponse): Model { + // Backend currently does not return operations that are not done. if (op.done) { + // Done operations must have either a response or an error. if (op.response) { return new Model(op.response); } else if (op.error) { - handleOperationError(op.error); + throw FirebaseMachineLearningError.fromOperationError( + op.error.code, op.error.message); } } throw new FirebaseMachineLearningError( 'invalid-server-response', `Invalid Operation response: ${JSON.stringify(op)}`); } - -function handleOperationError(err: StatusErrorResponse) { - throw FirebaseMachineLearningError.fromOperationError(err.code, err.message); -} diff --git a/test/integration/machine-learning.spec.ts b/test/integration/machine-learning.spec.ts index 83c4b208ce..b403a57f62 100644 --- a/test/integration/machine-learning.spec.ts +++ b/test/integration/machine-learning.spec.ts @@ -68,11 +68,10 @@ describe('admin.machineLearning', () => { } afterEach(() => { - console.log('DEBUGG: Cleaning up.'); return deleteTempModels(); }); - describe('createModel', () => { + describe('createModel()', () => { it('creates a new Model without ModelFormat', () => { const modelOptions: admin.machineLearning.ModelOptions = { displayName: 'node-integration-test-create-1', @@ -98,7 +97,7 @@ describe('admin.machineLearning', () => { scheduleForDelete(model); verifyModel(model, modelOptions); }); - }) + }); }); it('creates a new Model with invalid ModelFormat', () => { @@ -126,7 +125,6 @@ describe('admin.machineLearning', () => { return admin.machineLearning().createModel(modelOptions) .should.eventually.be.rejected.and.have.property('code', 'machine-learning/invalid-argument'); }); - }); describe('getModel()', () => { @@ -182,29 +180,32 @@ describe('admin.machineLearning', () => { }); }); - function verifyModel(model: admin.machineLearning.Model, modelOptions: admin.machineLearning.ModelOptions) { - expect(model.displayName).to.equal(modelOptions.displayName); + function verifyModel(model: admin.machineLearning.Model, expectedOptions: admin.machineLearning.ModelOptions) { + expect(model.displayName).to.equal(expectedOptions.displayName); expect(model.createTime).to.not.be.empty; expect(model.updateTime).to.not.be.empty; expect(model.etag).to.not.be.empty; - if (modelOptions.tags) { - expect(model.tags).to.deep.equal(modelOptions.tags); + if (expectedOptions.tags) { + expect(model.tags).to.deep.equal(expectedOptions.tags); } else { expect(model.tags).to.be.empty; } - if (modelOptions.tfliteModel) { - expect(model.tfliteModel!.gcsTfliteUri).to.equal(modelOptions.tfliteModel.gcsTfliteUri); - if (modelOptions.tfliteModel.gcsTfliteUri.endsWith('invalid_model.tflite')) { - expect(model.modelHash).to.be.empty; - expect(model.validationError).to.equal('Invalid flatbuffer format'); - } else { - expect(model.modelHash).to.not.be.empty; - expect(model.validationError).to.be.empty; - } + if (expectedOptions.tfliteModel) { + verifyTfliteModel(model, expectedOptions.tfliteModel.gcsTfliteUri); } else { expect(model.validationError).to.equal('No model file has been uploaded.'); } expect(model.locked).to.be.false; - } }); + +function verifyTfliteModel(model: admin.machineLearning.Model, expectedGcsTfliteUri: string) { + expect(model.tfliteModel!.gcsTfliteUri).to.equal(expectedGcsTfliteUri); + if (expectedGcsTfliteUri.endsWith('invalid_model.tflite')) { + expect(model.modelHash).to.be.empty; + expect(model.validationError).to.equal('Invalid flatbuffer format'); + } else { + expect(model.modelHash).to.not.be.empty; + expect(model.validationError).to.be.empty; + } +} diff --git a/test/unit/machine-learning/machine-learning-api-client.spec.ts b/test/unit/machine-learning/machine-learning-api-client.spec.ts index a3b3935fc6..6e8f3b3b64 100644 --- a/test/unit/machine-learning/machine-learning-api-client.spec.ts +++ b/test/unit/machine-learning/machine-learning-api-client.spec.ts @@ -19,7 +19,7 @@ import * as _ from 'lodash'; import * as chai from 'chai'; import * as sinon from 'sinon'; -import { MachineLearningApiClient } from '../../../src/machine-learning/machine-learning-api-client'; +import { MachineLearningApiClient, ModelContent } from '../../../src/machine-learning/machine-learning-api-client'; import { FirebaseMachineLearningError } from '../../../src/machine-learning/machine-learning-utils'; import { HttpClient } from '../../../src/utils/api-request'; import * as utils from '../utils'; @@ -78,6 +78,117 @@ describe('MachineLearningApiClient', () => { }); }); + describe('createModel', () => { + const NAME_ONLY_CONTENT: ModelContent = {displayName: 'name1'}; + const MODEL_RESPONSE = { + name: 'projects/test-project/models/1234567', + createTime: '2020-02-07T23:45:23.288047Z', + updateTime: '2020-02-08T23:45:23.288047Z', + etag: 'etag123', + modelHash: 'modelHash123', + displayName: 'model_1', + tags: ['tag_1', 'tag_2'], + state: {published: true}, + tfliteModel: { + gcsTfliteUri: 'gs://test-project-bucket/Firebase/ML/Models/model1.tflite', + sizeBytes: 16900988, + }, + }; + const STATUS_ERROR_RESPONSE = { + code: 3, + message: 'Invalid Argument message', + }; + const OPERATION_SUCCESS_RESPONSE = { + done: true, + response: MODEL_RESPONSE, + }; + const OPERATION_ERROR_RESPONSE = { + done: true, + error: STATUS_ERROR_RESPONSE, + }; + + const invalidContent: any[] = [null, undefined, {}, { tags: []}]; + invalidContent.forEach((content) => { + it(`should reject when called with: ${JSON.stringify(content)}`, () => { + return apiClient.createModel(content) + .should.eventually.be.rejected.and.have.property( + 'message', 'Invalid model content.'); + }); + }); + + it('should reject when project id is not available', () => { + return clientWithoutProjectId.createModel(NAME_ONLY_CONTENT) + .should.eventually.be.rejectedWith(noProjectId); + }); + + it('should throw when an error response is received', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom(ERROR_RESPONSE, 404)); + stubs.push(stub); + const expected = new FirebaseMachineLearningError('not-found', 'Requested entity not found'); + return apiClient.createModel(NAME_ONLY_CONTENT) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should resolve with the created resource on success', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .resolves(utils.responseFrom(OPERATION_SUCCESS_RESPONSE)); + stubs.push(stub); + return apiClient.createModel(NAME_ONLY_CONTENT) + .then((resp) => { + expect(resp.done).to.be.true; + expect(resp.name).to.be.empty; + expect(resp.response).to.deep.equal(MODEL_RESPONSE); + }); + }); + + it('should resolve with error when the operation fails', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .resolves(utils.responseFrom(OPERATION_ERROR_RESPONSE)); + stubs.push(stub); + return apiClient.createModel(NAME_ONLY_CONTENT) + .then((resp) => { + expect(resp.done).to.be.true; + expect(resp.name).to.be.empty; + expect(resp.error).to.deep.equal(STATUS_ERROR_RESPONSE); + }); + }); + + it('should throw unknown-error when error code is not present', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom({}, 404)); + stubs.push(stub); + const expected = new FirebaseMachineLearningError('unknown-error', 'Unknown server error: {}'); + return apiClient.createModel(NAME_ONLY_CONTENT) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should throw unknown-error for non-json response', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom('not json', 404)); + stubs.push(stub); + const expected = new FirebaseMachineLearningError( + 'unknown-error', 'Unexpected response with status: 404 and body: not json'); + return apiClient.createModel(NAME_ONLY_CONTENT) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should throw when rejected with a FirebaseAppError', () => { + const expected = new FirebaseAppError('network-error', 'socket hang up'); + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(expected); + stubs.push(stub); + return apiClient.createModel(NAME_ONLY_CONTENT) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + }); + describe('getModel', () => { const INVALID_NAMES: any[] = [null, undefined, '', 1, true, {}, []]; INVALID_NAMES.forEach((invalidName) => { From 7f847fe440cb0769e41411ea00e4ece100f37e3d Mon Sep 17 00:00:00 2001 From: ifielker Date: Wed, 19 Feb 2020 16:27:42 -0500 Subject: [PATCH 4/5] review comments 2 --- src/machine-learning/machine-learning.ts | 14 +++++--------- test/integration/machine-learning.spec.ts | 12 ++++++------ .../machine-learning-api-client.spec.ts | 8 ++++---- .../unit/machine-learning/machine-learning.spec.ts | 12 ------------ 4 files changed, 15 insertions(+), 31 deletions(-) diff --git a/src/machine-learning/machine-learning.ts b/src/machine-learning/machine-learning.ts index 7c87013dae..c9e7b8b515 100644 --- a/src/machine-learning/machine-learning.ts +++ b/src/machine-learning/machine-learning.ts @@ -16,8 +16,7 @@ import {FirebaseApp} from '../firebase-app'; import {FirebaseServiceInterface, FirebaseServiceInternalsInterface} from '../firebase-service'; -import {Storage} from '../storage/storage'; -import {MachineLearningApiClient, ModelResponse, OperationResponse} from './machine-learning-api-client'; +import {MachineLearningApiClient, ModelResponse, OperationResponse, ModelContent} from './machine-learning-api-client'; import {FirebaseError} from '../utils/error'; import * as validator from '../utils/validator'; @@ -61,7 +60,6 @@ export class MachineLearning implements FirebaseServiceInterface { private readonly client: MachineLearningApiClient; private readonly appInternal: FirebaseApp; - private readonly storage: Storage; /** * @param {FirebaseApp} app The app for this ML service. @@ -76,7 +74,6 @@ export class MachineLearning implements FirebaseServiceInterface { }); } - this.storage = app.storage(); this.appInternal = app; this.client = new MachineLearningApiClient(app); } @@ -174,7 +171,7 @@ export class MachineLearning implements FirebaseServiceInterface { return this.client.deleteModel(modelId); } - private convertOptionstoContent(options: ModelOptions, forUpload?: boolean): Promise { + private convertOptionstoContent(options: ModelOptions, forUpload?: boolean): Promise { const modelContent = deepCopy(options); if (forUpload && modelContent.tfliteModel?.gcsTfliteUri) { @@ -187,11 +184,10 @@ export class MachineLearning implements FirebaseServiceInterface { throw new FirebaseMachineLearningError( 'internal-error', `Error during signing upload url: ${err.message}`); - - }); + }) as Promise; } - return Promise.resolve(modelContent); + return Promise.resolve(modelContent) as Promise; } private signUrl(unsignedUrl: string): Promise { @@ -207,7 +203,7 @@ export class MachineLearning implements FirebaseServiceInterface { } const bucketName = matches[1]; const blobName = matches[2]; - const bucket = this.storage.bucket(bucketName); + const bucket = this.appInternal.storage().bucket(bucketName); const blob = bucket.file(blobName); return blob.getSignedUrl({ action: 'read', diff --git a/test/integration/machine-learning.spec.ts b/test/integration/machine-learning.spec.ts index b403a57f62..cb56438ce7 100644 --- a/test/integration/machine-learning.spec.ts +++ b/test/integration/machine-learning.spec.ts @@ -63,8 +63,8 @@ describe('admin.machineLearning', () => { const tfliteFileName = path.join(__dirname, `../resources/${localFileName}`); return bucket.upload(tfliteFileName, {destination: gcsFileName}) .then(() => { - return `gs://${bucket.name}/${gcsFileName}`; - }); + return `gs://${bucket.name}/${gcsFileName}`; + }); } afterEach(() => { @@ -93,10 +93,10 @@ describe('admin.machineLearning', () => { .then((fileName: string) => { modelOptions.tfliteModel!.gcsTfliteUri = fileName; return admin.machineLearning().createModel(modelOptions) - .then((model) => { - scheduleForDelete(model); - verifyModel(model, modelOptions); - }); + .then((model) => { + scheduleForDelete(model); + verifyModel(model, modelOptions); + }); }); }); diff --git a/test/unit/machine-learning/machine-learning-api-client.spec.ts b/test/unit/machine-learning/machine-learning-api-client.spec.ts index 6e8f3b3b64..b4348b5b7c 100644 --- a/test/unit/machine-learning/machine-learning-api-client.spec.ts +++ b/test/unit/machine-learning/machine-learning-api-client.spec.ts @@ -157,7 +157,7 @@ describe('MachineLearningApiClient', () => { }); }); - it('should throw unknown-error when error code is not present', () => { + it('should reject with unknown-error when error code is not present', () => { const stub = sinon .stub(HttpClient.prototype, 'send') .rejects(utils.errorFrom({}, 404)); @@ -167,7 +167,7 @@ describe('MachineLearningApiClient', () => { .should.eventually.be.rejected.and.deep.equal(expected); }); - it('should throw unknown-error for non-json response', () => { + it('should reject with unknown-error for non-json response', () => { const stub = sinon .stub(HttpClient.prototype, 'send') .rejects(utils.errorFrom('not json', 404)); @@ -178,7 +178,7 @@ describe('MachineLearningApiClient', () => { .should.eventually.be.rejected.and.deep.equal(expected); }); - it('should throw when rejected with a FirebaseAppError', () => { + it('should reject with when failed with a FirebaseAppError', () => { const expected = new FirebaseAppError('network-error', 'socket hang up'); const stub = sinon .stub(HttpClient.prototype, 'send') @@ -257,7 +257,7 @@ describe('MachineLearningApiClient', () => { .should.eventually.be.rejected.and.deep.equal(expected); }); - it('should reject when rejected with a FirebaseAppError', () => { + it('should reject when failed with a FirebaseAppError', () => { const expected = new FirebaseAppError('network-error', 'socket hang up'); const stub = sinon .stub(HttpClient.prototype, 'send') diff --git a/test/unit/machine-learning/machine-learning.spec.ts b/test/unit/machine-learning/machine-learning.spec.ts index 5ed345a905..7749c35544 100644 --- a/test/unit/machine-learning/machine-learning.spec.ts +++ b/test/unit/machine-learning/machine-learning.spec.ts @@ -121,13 +121,11 @@ describe('MachineLearning', () => { let machineLearning: MachineLearning; let mockApp: FirebaseApp; - let mockCredentialApp: FirebaseApp; const stubs: sinon.SinonStub[] = []; before(() => { mockApp = mocks.app(); - mockCredentialApp = mocks.mockCredentialApp(); machineLearning = new MachineLearning(mockApp); }); @@ -161,16 +159,6 @@ describe('MachineLearning', () => { + 'instance.'); }); - it('should throw given invalid credential', () => { - const expectedError = 'Failed to initialize Google Cloud Storage client with ' + - 'the available credential. Must initialize the SDK with a certificate credential ' + - 'or application default credentials to use Cloud Storage API.'; - expect(() => { - const machineLearningAny: any = MachineLearning; - return new machineLearningAny(mockCredentialApp); - }).to.throw(expectedError); - }); - it('should not throw given a valid app', () => { expect(() => { return new MachineLearning(mockApp); From 04b2e16b45f5a6e2e6a3c781bbb0d7b90c3ebe92 Mon Sep 17 00:00:00 2001 From: ifielker Date: Thu, 20 Feb 2020 17:38:10 -0500 Subject: [PATCH 5/5] review comments 3 --- .../machine-learning/machine-learning.spec.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/unit/machine-learning/machine-learning.spec.ts b/test/unit/machine-learning/machine-learning.spec.ts index 7749c35544..beddfd74d9 100644 --- a/test/unit/machine-learning/machine-learning.spec.ts +++ b/test/unit/machine-learning/machine-learning.spec.ts @@ -121,11 +121,13 @@ describe('MachineLearning', () => { let machineLearning: MachineLearning; let mockApp: FirebaseApp; + let mockCredentialApp: FirebaseApp; const stubs: sinon.SinonStub[] = []; before(() => { mockApp = mocks.app(); + mockCredentialApp = mocks.mockCredentialApp(); machineLearning = new MachineLearning(mockApp); }); @@ -159,6 +161,20 @@ describe('MachineLearning', () => { + 'instance.'); }); + it('should throw given invalid credential', () => { + const expectedError = 'Failed to initialize Google Cloud Storage client with ' + + 'the available credential. Must initialize the SDK with a certificate credential ' + + 'or application default credentials to use Cloud Storage API.'; + expect(() => { + const machineLearningAny: any = MachineLearning; + return new machineLearningAny(mockCredentialApp).createModel({ + displayName: 'foo', + tfliteModel: { + gcsTfliteUri: 'gs://some-bucket/model.tflite', + }}); + }).to.throw(expectedError); + }); + it('should not throw given a valid app', () => { expect(() => { return new MachineLearning(mockApp);