Skip to content

Added CreateModel functionality and tests #788

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Feb 20, 2020
Merged

Added CreateModel functionality and tests #788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a8c5a8
Added CreateModel functionality and tests
ifielker Feb 18, 2020
bfc7fc9
debugging
ifielker Feb 18, 2020
f266074
review comments
ifielker Feb 19, 2020
7f847fe
review comments 2
ifielker Feb 19, 2020
04b2e16
review comments 3
ifielker Feb 20, 2020
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
14 changes: 5 additions & 9 deletions src/machine-learning/machine-learning.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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.
Expand All @@ -76,7 +74,6 @@ export class MachineLearning implements FirebaseServiceInterface {
});
}

this.storage = app.storage();
this.appInternal = app;
this.client = new MachineLearningApiClient(app);
}
Expand Down Expand Up @@ -174,7 +171,7 @@ export class MachineLearning implements FirebaseServiceInterface {
return this.client.deleteModel(modelId);
}

private convertOptionstoContent(options: ModelOptions, forUpload?: boolean): Promise<object> {
private convertOptionstoContent(options: ModelOptions, forUpload?: boolean): Promise<ModelContent> {
const modelContent = deepCopy(options);

if (forUpload && modelContent.tfliteModel?.gcsTfliteUri) {
Expand All @@ -187,11 +184,10 @@ export class MachineLearning implements FirebaseServiceInterface {
throw new FirebaseMachineLearningError(
'internal-error',
`Error during signing upload url: ${err.message}`);

});
}) as Promise<ModelContent>;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const modelContent = deepCopy(options) as ModelContent;

Copy link
Contributor Author

@ifielker ifielker Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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>)

}

return Promise.resolve(modelContent);
return Promise.resolve(modelContent) as Promise<ModelContent>;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

private signUrl(unsignedUrl: string): Promise<string> {
Expand All @@ -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',
Expand Down
12 changes: 6 additions & 6 deletions test/integration/machine-learning.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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);
});
});
});

Expand Down
8 changes: 4 additions & 4 deletions test/unit/machine-learning/machine-learning-api-client.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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')
Expand Down Expand Up @@ -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')
Expand Down
12 changes: 0 additions & 12 deletions test/unit/machine-learning/machine-learning.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

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);
Expand Down