Skip to content
This repository has been archived by the owner on Jul 20, 2023. It is now read-only.

feat(samples): adds samples for enhanced version of library #16

Merged
merged 39 commits into from
Jan 13, 2021

Conversation

telpirion
Copy link
Contributor

This PR updates the following samples to use the enhanced client library:

  • All create_batch_prediction_job samples for video
  • All create_training_pipeline samples for image, text, video, and tabular
  • All predict samples for image, tabular, and text

This PR relies on the PR that contains the enhanced client library.

@product-auto-label product-auto-label bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 18, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 18, 2020
@snippet-bot
Copy link

snippet-bot bot commented Nov 18, 2020

Here is the summary of changes.

You added 16 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #16 (fb1ec49) into master (be5d384) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   98.80%   98.82%   +0.01%     
==========================================
  Files          15       15              
  Lines       20493    20493              
  Branches      575      575              
==========================================
+ Hits        20249    20253       +4     
+ Misses        236      232       -4     
  Partials        8        8              
Impacted Files Coverage Δ
src/v1beta1/dataset_service_client.ts 99.18% <0.00%> (+0.06%) ⬆️
src/v1beta1/endpoint_service_client.ts 99.05% <0.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be5d384...fb1ec49. Read the comment docs.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

A couple initial comments for you.

@@ -8,5 +8,5 @@ env_vars: {

env_vars: {
key: "SECRET_MANAGER_KEYS"
value: "nodejs-ai-platform-env"
value: "nodejs-ai-platform-env, ucaip_samples_secrets"
Copy link
Contributor

Choose a reason for hiding this comment

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

we'll need to add .kokoro/presubmit/node12/samples-test.cfg to excludes, in synth.py, so that it's not overwritten when the templates update nightly.

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.

// Create batch prediction job request
const [response] = await jobServiceClient.createBatchPredictionJob(request);

console.log(`Create batch prediction job video classification response`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log(`Create batch prediction job video classification response`);
console.log('Create batch prediction job video classification response');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a problem across all of these samples. I've addressed them in spots, where I've found them, but we need to root them all out.

I've logged issue #38 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think we can mark #38 as fixed with this PR. I've removed all of the console.log statements with the unneeded back ticks.


const modelParameters = modelParamsObj.toValue();

const inputConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

there's enough setup here, that I would be tempted to add a couple comments that include links to the corresponding documentation.

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.

}

const dedicatedResource = response.dedicatedResource;
console.log(`\tDedicated resources`);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of logic here for the printing logic, I'd be tempted to see if I could abstract it a bit into a helper, that's also between the START/END blocks, something like:

function printDedicatedResource(dedicatedResource) {
  
}

function printCompletionStats () {

}

...

If you moved these down to the bottom of the sample, I think you would draw better attention to the more important part of samples which is the calls to the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Earlier versions of the Python canonicals had more verbose responses printed out. Now they're much shorter.

@telpirion telpirion marked this pull request as ready for review January 12, 2021 19:42
@telpirion telpirion requested review from a team as code owners January 12, 2021 19:42
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This looks pretty much ready to go to me, I think we'll want to address the await issue I brought up, as it will make the samples embed better on cloud site.

console.log(response);
}
// [END aiplatform_create_batch_prediction_job_video_classification]
await createBatchPredictionJobVideoClassification();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't have the createBatchPredictionJobVideoClassification inside the snippet, we'll show example code that doesn't actually invoke to the customer, instead do:

createBatchPredictionJobVideoClassification();

Dropping the await.

And you can add this:

rocess.on('unhandledRejection', err => {
  console.error(err.message);
  process.exitCode = 1;
});

Just above your invocation of main.

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.

console.log(response);
}
// [END aiplatform_create_training_pipeline_text_sentiment_analysis]
await createTrainingPipelineTextSentimentAnalysis();
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we'll need to address the await issue across the samles.

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.

console.log('Create batch prediction job video classification response');
console.log(`Name : ${response.name}`);
console.log('Raw response:');
console.log(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is an object with nested keys, you might want to do:

console.log(JSON.stringify(response, null, 2));

I like this approach a lot better than having a huge amount of logic for printing.

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.

@telpirion telpirion requested a review from bcoe January 13, 2021 19:21
@bcoe bcoe changed the title feat: adds samples for enhanced version of library feat(samples): adds samples for enhanced version of library Jan 13, 2021
@bcoe bcoe merged commit aef443c into master Jan 13, 2021
@bcoe bcoe deleted the encl-samples branch January 13, 2021 19:51
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants