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 forecasting snippets and fixed bugs with existing snippets #1210

Merged
merged 10 commits into from
May 31, 2022
Merged

feat: Added forecasting snippets and fixed bugs with existing snippets #1210

merged 10 commits into from
May 31, 2022

Conversation

ivanmkc
Copy link
Contributor

@ivanmkc ivanmkc commented May 9, 2022 •
edited
Loading

Issues with existing snippets fixed

  • dataset_id should be a str and not an int
  • create_training_pipeline_tabular_classification_sample and create_training_pipeline_tabular_regression_sample are missing parameters
  • Renamed samples/model-builder/explain_tabular_sample.py to samples/model-builder/explain_sample.py since it is universally applicable.

Added snippets and accompanying tests

  • samples/model-builder/create_and_import_dataset_time_series_bigquery_sample.py
  • samples/model-builder/create_and_import_dataset_time_series_gcs_sample.py
  • samples/model-builder/create_training_pipeline_forecasting_sample.py
  • samples/model-builder/create_batch_prediction_job_dedicated_resources_bigquery_sample.py
  • samples/model-builder/create_batch_prediction_job_sample_bigquery.py

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label May 9, 2022
@snippet-bot
Copy link

snippet-bot bot commented May 9, 2022 •
edited
Loading

Here is the summary of changes.

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

@product-auto-label product-auto-label bot added api: vertex-ai Issues related to the googleapis/python-aiplatform API. size: l Pull request size is large. and removed size: m Pull request size is medium. labels May 9, 2022
@ivanmkc ivanmkc changed the title [WIP] Added dataset snippets [WIP] Added forecasting snippets May 9, 2022
@ivanmkc ivanmkc changed the title [WIP] Added forecasting snippets [WIP] Added forecasting snippets and fixed bugs with existing snippets May 9, 2022
@ivanmkc ivanmkc changed the title [WIP] Added forecasting snippets and fixed bugs with existing snippets Added forecasting snippets and fixed bugs with existing snippets May 9, 2022
@ivanmkc ivanmkc force-pushed the imkc--forecasting-snippets branch from 8f89496 to d0c2ca6 Compare May 9, 2022 19:02
@ivanmkc ivanmkc changed the title Added forecasting snippets and fixed bugs with existing snippets feat: Added forecasting snippets and fixed bugs with existing snippets May 9, 2022
@ivanmkc ivanmkc marked this pull request as ready for review May 9, 2022 19:03
@ivanmkc ivanmkc requested review from a team as code owners May 9, 2022 19:03
@ivanmkc ivanmkc requested a review from m-strzelczyk May 9, 2022 19:03
@sararob sararob added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 10, 2022
TheMichaelHu
TheMichaelHu reviewed May 10, 2022
View reviewed changes
samples/model-builder/conftest.py Outdated Show resolved Hide resolved
samples/model-builder/create_training_pipeline_forecasting_sample_test.py Show resolved Hide resolved
samples/model-builder/create_and_import_dataset_time_series_bigquery_sample.py Show resolved Hide resolved
samples/model-builder/create_training_pipeline_forecasting_sample.py
sync=sync,
)

model.wait()
Copy link
Contributor

@TheMichaelHu TheMichaelHu May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we're always going to call wait() right after, why not just always have sync=True and remove sync as an arg? I see some run args not present (e.g. predefined_split_column_name, weight_column, context_window, export_evaluated_data_times...), so I imagine it's fine to omit sync.

Optional: I think context_window and the args associated with exporting evaluations on the test set are pretty important/commonly used. Would be nice to see them 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.

Thanks Michael, I will add the other args that are important.

Let me double-check about sync, as I'm following the common convention for other samples.

Copy link
Contributor Author

@ivanmkc ivanmkc May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The sync is to show users that they can call sync if they use the async API.

@ivanmkc ivanmkc force-pushed the imkc--forecasting-snippets branch 2 times, most recently from dfe08e3 to b0e4f5e Compare May 17, 2022 16:57
@ivanmkc ivanmkc force-pushed the imkc--forecasting-snippets branch from 1d7a6cd to 9d4d699 Compare May 24, 2022 18:56
@andrewferlitsch andrewferlitsch merged commit 4e4bff5 into googleapis:main May 31, 2022
@release-please release-please bot mentioned this pull request May 31, 2022
Merged
rosiezou pushed a commit that referenced this pull request Jun 16, 2022
@ivanmkc @rosiezou
feat: Added forecasting snippets and fixed bugs with existing snippets (
e6c8547 
#1210)

* Added dataset snippets

* Fixed typehint and missing parameter bugs as well as added new samples

* Fixed lint issues

* Added bq batch_prediction bq snippets

* Removed unneeded fixture

* Renamed bq_source to bigquery_source

* Added back explain_tabular_sample.py for now

* Fixed tests

* Fixed lint issues
@release-please release-please bot mentioned this pull request Jun 8, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheMichaelHu TheMichaelHu TheMichaelHu left review comments

@reznitskii reznitskii reznitskii approved these changes

@andrewferlitsch andrewferlitsch andrewferlitsch approved these changes

@m-strzelczyk m-strzelczyk Awaiting requested review from m-strzelczyk m-strzelczyk was automatically assigned from googleapis/python-samples-owners

Assignees
No one assigned
Labels
api: vertex-ai Issues related to the googleapis/python-aiplatform API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ivanmkc @TheMichaelHu @andrewferlitsch @reznitskii @sararob