-
Notifications
You must be signed in to change notification settings - Fork 24
samples: adds custom model, action recognition samples and tests #111
Conversation
Warning: This pull request is touching the following templated files:
|
samples/snippets/nbactions.xml
Outdated
@@ -0,0 +1,19 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright & license on human created files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ew, I didn't mean to include that. Sorry! I'll remove that from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider comments
//import com.google.cloud.aiplatform.v1beta1.ClassOfConditionalParameterDecay; | ||
//import com.google.cloud.aiplatform.v1beta1.ClassOfConditionalParameterLearningRate; | ||
//import com.google.cloud.aiplatform.v1beta1.ClassOfDiscreteValueSpec; | ||
//import com.google.cloud.aiplatform.v1beta1.ClassOfDoubleValueSpec; | ||
//import com.google.cloud.aiplatform.v1beta1.ClassOfMetric; | ||
//import com.google.cloud.aiplatform.v1beta1.ClassOfParameter; | ||
//import com.google.cloud.aiplatform.v1beta1.ClassOfParameterSpec; | ||
//import com.google.cloud.aiplatform.v1beta1.ClassOfParentDiscreteValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't usually leave these in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
PipelineServiceSettings settings = | ||
PipelineServiceSettings.newBuilder() | ||
.setEndpoint("us-central1-aiplatform.googleapis.com:443") | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged.
AI Platform doesn't have a global hostname, only locale-specific hostnames.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to this effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged.
We've used this technique on every single sample in this repo. I'll create a GitHub issue to go back and add a comment in all of the places where we do this.
// A working docker image can be found at | ||
// gs://cloud-samples-data/ai-platform/mnist_tfrecord/custom_job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a docker image, why not use gcr.io or it's successor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. @dizcology can give more context.
See Python canonical:
https://github.com/googleapis/python-aiplatform/blob/master/samples/snippets/create_training_pipeline_custom_job_sample.py#L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we are pointing the user to the Dockerfile and its content, since real use cases require the user to build and bring their own.
Value trainingTaskInputs = trainingTaskInputsBuilder.build(); | ||
String trainingTaskDefinition = | ||
"gs://google-cloud-aiplatform/schema/trainingjob/definition/custom_task_1.0.0.yaml"; | ||
String imageUri = "gcr.io/cloud-aiplatform/prediction/tf-cpu.1-15:latest"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:latest is typically frowned upon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. @dizcology can give more context.
See Python canonical:
https://github.com/googleapis/python-aiplatform/blob/master/samples/snippets/create_training_pipeline_custom_job_sample.py#L58
Warning: This pull request is touching the following templated files:
|
Warning: This pull request is touching the following templated files:
|
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
=========================================
Coverage 78.22% 78.22%
Complexity 704 704
=========================================
Files 48 48
Lines 5124 5124
Branches 69 69
=========================================
Hits 4008 4008
Misses 1027 1027
Partials 89 89 Continue to review full report at Codecov.
|
Warning: This pull request is touching the following templated files:
|
Warning: This pull request is touching the following templated files:
|
Warning: This pull request is touching the following templated files:
|
Warning: This pull request is touching the following templated files:
|
Warning: This pull request is touching the following templated files:
|
Thank you for the review, @lesv ! @chingor13 : It looks like this PR is still having issues with env vars. Your help is appreciated. Similarly, we accidentally deleted some of our test resources recently. I think that might be causing some of the 'Model is missing' errors in this PR. |
samples/snippets/pom.xml
Outdated
<groupId>org.glassfish</groupId> | ||
<artifactId>javax.json</artifactId> | ||
<version>1.1</version> | ||
<type>jar</type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use either Jackson2 or GSON here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
JsonObject jsonModelParameters = Json.createObjectBuilder().build(); | ||
Value.Builder modelParametersBuilder = Value.newBuilder(); | ||
JsonFormat.parser().merge(jsonModelParameters.toString(), modelParametersBuilder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty confusing - it looks like jsonModelParameters would be something empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is correct. You need to pass an empty Value object to the service for that field.
I'll add a note that clarifies this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Warning: This pull request is touching the following templated files:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should encourage usage of resource name helpers when available - they are more friendly than building the strings from scratch.
BatchPredictionJob batchPredictionJob = | ||
BatchPredictionJob.newBuilder() | ||
.setDisplayName(displayName) | ||
// Format: 'projects/{project}/locations/{location}/models/{model_id}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For any resource name, we should probably demonstrate the resource name helpers:
String model = com.google.cloud.aiplatform.v1beta1.ModelName.of("project", "location", "model").toString();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
DataLabelingJob dataLabelingJob = | ||
DataLabelingJob.newBuilder() | ||
.setDisplayName(displayName) | ||
// Full resource name: projects/{project}/locations/{location}/datasets/{dataset_id} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DatasetName.of("project", "location", "dataset");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
"aiplatform.googleapis.com/annotation_set_name", | ||
"data_labeling_job_specialist_pool") | ||
// Full resource name: | ||
// projects/{project}/locations/{location}/specialistPools/{specialist_pool_id} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SpecialistPoolName.of("project", "location", "specialistPool");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
PipelineServiceSettings settings = | ||
PipelineServiceSettings.newBuilder() | ||
.setEndpoint("us-central1-aiplatform.googleapis.com:443") | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to this effect.
Adds new code snippets.