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.

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

Jump to bottom

feat: Added create_training_pipeline_custom_job_sample and create_training_pipeline_custom_training_managed_dataset_sample and fixed create_training_pipeline_image_classification_sample #343

Merged
merged 10 commits into from
Apr 27, 2021
Merged

feat: Added create_training_pipeline_custom_job_sample and create_training_pipeline_custom_training_managed_dataset_sample and fixed create_training_pipeline_image_classification_sample #343

merged 10 commits into from
Apr 27, 2021

Conversation

ivanmkc
Copy link
Contributor

@ivanmkc ivanmkc commented Apr 20, 2021 •
edited
Loading

Added 2 samples with tests:

  • create_training_pipeline_custom_job_sample
  • create_training_pipeline_custom_training_managed_dataset_sample.py

Fixed:

  • create_training_pipeline_image_classification_sample.py

@ivanmkc ivanmkc requested review from dizcology and a team as code owners April 20, 2021 22:59
@ivanmkc ivanmkc requested review from dinagraves and removed request for a team April 20, 2021 22:59
@product-auto-label product-auto-label bot added api: aiplatform Issues related to the AI Platform API. samples Issues that are directly related to samples. labels Apr 20, 2021
@snippet-bot
Copy link

snippet-bot bot commented Apr 20, 2021 •
edited
Loading

Here is the summary of changes.

You are about to add 2 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

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 20, 2021
@ivanmkc ivanmkc changed the title Imkc mbsdk milestone2 samples Added create_and_import_dataset_tabular_bigquery_sample and create_and_import_dataset_tabular_gcs_sample Apr 20, 2021
@ivanmkc ivanmkc changed the title Added create_and_import_dataset_tabular_bigquery_sample and create_and_import_dataset_tabular_gcs_sample feat: Added create_and_import_dataset_tabular_bigquery_sample and create_and_import_dataset_tabular_gcs_sample Apr 20, 2021
@ivanmkc ivanmkc force-pushed the imkc--mbsdk-milestone2-samples branch from 85fff2d to 0ca06d5 Compare April 20, 2021 23:25
@ivanmkc ivanmkc changed the title feat: Added create_and_import_dataset_tabular_bigquery_sample and create_and_import_dataset_tabular_gcs_sample feat: Added create_and_import_dataset_tabular_bigquery_sample, create_and_import_dataset_tabular_gcs_sample and create_training_pipeline_custom_job_sample Apr 21, 2021
@ivanmkc ivanmkc force-pushed the imkc--mbsdk-milestone2-samples branch from 4ea56bf to 68eff56 Compare April 21, 2021 00:27
sasha-gitg
sasha-gitg reviewed Apr 21, 2021
View reviewed changes
samples/model-builder/create_and_import_dataset_tabular_bigquery_sample.py Outdated


# [START aiplatform_sdk_create_and_import_dataset_tabular_bigquery_sample]
def create_and_import_dataset_tabular_bigquery_sample(
Copy link
Member

Choose a reason for hiding this comment

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

Is this a dupe of this: https://github.com/googleapis/python-aiplatform/pull/338/files#diff-1efd7e79aab7a269e6ed5cb0ce9d4a9c3016782762156e3869a4d77d9d4824daR20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but it's under my name. Let me check with @aribray.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assignments might have changed between her writing the sample and me starting. I will remove these and we can use Ari's.

sasha-gitg
sasha-gitg requested changes Apr 21, 2021
View reviewed changes
samples/model-builder/create_training_pipeline_custom_job_sample.py Show resolved Hide resolved
samples/model-builder/create_training_pipeline_custom_job_sample.py Outdated
def create_training_pipeline_custom_job_sample(
project: str,
display_name: str,
args: List[str],
Copy link
Member

Choose a reason for hiding this comment

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

args are optional and should default to None.

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.

samples/model-builder/create_training_pipeline_custom_job_sample.py Outdated
script_path: str,
container_uri: str,
location: str = "us-central1",
model_display_name: str = None,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
model_display_name: str = None,
model_display_name: Optional[str] = None,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do it, but I followed the example of the existing create_training_pipeline_image_classification_sample. I will change it there too.

samples/model-builder/create_training_pipeline_custom_job_sample.py Outdated
script_path: str,
container_uri: str,
location: str = "us-central1",
model_display_name: str = None,
Copy link
Member

Choose a reason for hiding this comment

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

We should also include the model_serving_container_image_uri as required since we are returning the Model in the sample.

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.

samples/model-builder/create_training_pipeline_custom_job_sample.py Show resolved Hide resolved
samples/model-builder/create_training_pipeline_custom_training_managed_dataset_sample.py Outdated
container_uri: str,
dataset_id: int,
location: str = "us-central1",
model_display_name: str = None,
Copy link
Member

Choose a reason for hiding this comment

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

Same comments here as above.

@ivanmkc
Copy link
Contributor Author

ivanmkc commented Apr 21, 2021 •
edited
Loading

@sasha-gitg I fixed a couple other things along the same lines, such as order of args.

@ivanmkc ivanmkc changed the title feat: Added create_and_import_dataset_tabular_bigquery_sample, create_and_import_dataset_tabular_gcs_sample and create_training_pipeline_custom_job_sample feat: Added create_training_pipeline_custom_job_sample and create_training_pipeline_custom_training_managed_dataset_sample and fixed create_training_pipeline_image_classification_sample Apr 21, 2021
@ivanmkc ivanmkc force-pushed the imkc--mbsdk-milestone2-samples branch from 22abd8c to fec3e6c Compare April 21, 2021 18:46
dinagraves
dinagraves previously requested changes Apr 23, 2021
View reviewed changes
Copy link

@dinagraves dinagraves left a comment

Choose a reason for hiding this comment

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

Please fix linting errors!

@ivanmkc ivanmkc mentioned this pull request Apr 23, 2021
Merged
@ivanmkc ivanmkc added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 26, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 26, 2021
@ivanmkc ivanmkc added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 27, 2021
@ivanmkc
Copy link
Contributor Author

ivanmkc commented Apr 27, 2021 •
edited
Loading

@dinagraves Linting fixed. Can I get another look?

I usually run the linter after all the other checks/approvals pass so I only run it once.

@vinnysenthil vinnysenthil requested a review from dinagraves April 27, 2021 17:29
@ivanmkc ivanmkc dismissed dinagraves’s stale review April 27, 2021 21:41

Linter passed.

@ivanmkc ivanmkc merged commit 1c6b998 into googleapis:master Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sasha-gitg sasha-gitg sasha-gitg approved these changes

@dizcology dizcology Awaiting requested review from dizcology

@vinnysenthil vinnysenthil Awaiting requested review from vinnysenthil

@dinagraves dinagraves Awaiting requested review from dinagraves

Assignees
No one assigned
Labels
api: aiplatform Issues related to the AI Platform API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ivanmkc @sasha-gitg @dinagraves @yoshi-kokoro