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.

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

feat: Added tabular forecasting samples #128

Merged
merged 16 commits into from Dec 22, 2020

Conversation

ivanmkc
Copy link
Contributor

@ivanmkc ivanmkc commented Dec 10, 2020

Added samples and tests for AutoML forecasting

  • list_model_evaluations_tabular_forecasting_sample
  • create_training_pipeline_tabular_forecasting_sample
  • get_model_evaluation_tabular_forecasting
  • predict_tabular_forecasting <- Forecasting doesn't support predict so will do in a subsequent PR.

@ivanmkc ivanmkc requested review from dizcology and a team as code owners December 10, 2020 16:49
@ivanmkc ivanmkc requested review from kurtisvg and removed request for a team December 10, 2020 16:49
@snippet-bot
Copy link

snippet-bot bot commented Dec 10, 2020

Here is the summary of changes.

You added 1 region tag.

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 Dec 10, 2020
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Dec 10, 2020

training_task_inputs_dict = {
# required inputs
"targetColumn": target_column,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious as to if we should show comments for each of these values in the sample.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ivanmkc do you mean comments like # display_name: YOUR_DISPLAY_NAME?

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 for reviewing, I was wondering if we should add comments for each param in the samples.

Seems like Yuhan is suggesting to just tell them to read the docstrings.

@@ -0,0 +1,37 @@
# Copyright 2020 Google LLC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as corresponding classification sample.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don’t really need to have sample variants for get and list methods. I see that we have them for some other kinds of AutoML services and perhaps those need to be removed (in a separate PR).

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!

@@ -0,0 +1,44 @@
# Copyright 2020 Google LLC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as corresponding classification sample.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that all the predict tabular samples would be identical? Is so we should consolidate those too perhaps. On the other hand if the sample tests would be very different (Eg you might be looking for different specific fields in the predictions), then let’s keep this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Predict cannot be used with 'tabular'. They only support batch_predict, I will think about consolidation when doing batch_predict next week.

@ivanmkc ivanmkc changed the title feat: Added tabular forecasting samples feat: Added tabular forecasting samples [WIP] Dec 10, 2020
@ivanmkc ivanmkc force-pushed the imkc--tabular-forecasting-samples branch from a74d6c3 to e75b880 Compare December 11, 2020 11:52
Ivan Cheung added 3 commits December 15, 2020 01:36
…t, fixed get_model_evaluation_tabular_forecasting_sample, and fixed create_training_pipeline_tabular_forecasting_sample
@ivanmkc ivanmkc changed the title feat: Added tabular forecasting samples [WIP] feat: Added tabular forecasting samples Dec 15, 2020

import predict_tabular_classification_sample

ENDPOINT_ID = "TODO"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ivanmkc
Copy link
Contributor Author

ivanmkc commented Dec 15, 2020

Fixing CI errors.

"transformations": transformations,
"period": period,

# Objective function the model is to be optimized towards.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to point to the doc pages instead of including the information below 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.

Do you mean include a link in the comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

right - if the information here is available on a particular documentation page.

@@ -175,7 +177,7 @@ list_hyperparameter_tuning_jobs:
list_model_evaluation_slices:
- ''
list_model_evaluations:
- ''
- tabular_forecasting
Copy link
Contributor

Choose a reason for hiding this comment

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

this would replace the list_model_evaluation sample. Please keep the "default" variant keyed with the empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also - perhaps we don't need a special list_model_evaluations variant unless the call to list_model_evaluations is different in the tabular_forecasting use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. Good catch.

@@ -182,6 +183,7 @@ get_model_evaluation_sample:
- model_explanation
get_model_evaluation_slice_sample: {}
get_model_evaluation_tabular_classification_sample: {}
get_model_evaluation_tabular_forecasting_sample: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

If the tabular_forcasting variant for get_model_evaluation ends up identical with any get_model_evaluation, please skip it. (and we should separately clean up the others - but perhaps i missed something 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.

I got it, we want to consolidate if possible. Let me do another pass.

@ivanmkc ivanmkc force-pushed the imkc--tabular-forecasting-samples branch from 9f113a4 to 087ff39 Compare December 21, 2020 20:26
@ivanmkc
Copy link
Contributor Author

ivanmkc commented Dec 22, 2020

@dizcology can I get another look?

@ivanmkc ivanmkc merged commit 69fc7fd into googleapis:master Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

3 participants