-
Notifications
You must be signed in to change notification settings - Fork 348
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: enabling AutoML Forecasting training response to include BigQuery location of exported evaluated examples #657
Conversation
…recasting Training Job in training output
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.
Thanks Mansi! Added one comment. Could you also run nox -s lint
in the repo root? Looks like you need to run black
on training_jobs.py
try: | ||
meta = getattr((self._gca_resource), "training_task_metadata") | ||
except ValueError: | ||
raise ValueError("BigQuery uri for evaluated data items does not exist. Must export evaluated data items during training.") | ||
|
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 can be removed as the try/except below already covers the logic.
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.
Removed try/except block.
Removed the unnecessary try/except block from the evaluated bigquery uri property method in Automl Forecasting
nox -s lint has been successfully run in root of repo |
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.
Please update the unit tests.
@@ -4138,6 +4146,20 @@ def _model_upload_fail_string(self) -> str: | |||
"Model." | |||
) | |||
|
|||
@property | |||
def evaluated_data_items_bigquery_uri(self) -> Optional[str]: |
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.
What are the scenarios where this method:
- returns
str
- returns
None
- raises ValueError
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.
Updated the method doc string.
try: | ||
metadata = self._gca_resource.training_task_metadata | ||
return metadata["evaluatedDataItemsBigqueryUri"] | ||
except (AttributeError, KeyError): |
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 both AttributeError
and KeyError
?
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.
Corrected.
@@ -4138,6 +4146,20 @@ def _model_upload_fail_string(self) -> str: | |||
"Model." | |||
) | |||
|
|||
@property | |||
def evaluated_data_items_bigquery_uri(self) -> Optional[str]: | |||
"""BigQuery location of exported evaluated examples from the Training 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.
Raises section.
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.
No longer needed.
@mansiachuthan Any updates on this? |
Hello @ivanmkc @sasha-gitg! I just wanted to let you know that my internship ended a few weeks ago and I am currently not working on this PR. I would like to request my mentor @thehardikv to answer any future comments/questions for this pull request, as I would love to see that these changes get merged. Thank you! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
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.
One requested change. @thehardikv It seems like this PR may need to closed and opened as a new PR as the CLA bot may be complaining about
mansiachuthan.
self._assert_gca_resource_is_available() | ||
|
||
metadata = self._gca_resource.training_task_metadata | ||
if "evaluatedDataItemsBigqueryUri" in metadata: |
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.
Safer to do this: if metadata and "evaluatedDataItemsBigqueryUri" in metadata:
See this issue b/192601601. training_task_metadata
may not be populated by the service.
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.
@sasha-gitg Before I jump to copying over your changes to a fresh PR (and eliminating the commit history and credit attribution), we're going to try vinnysenthil@'s suggestion: mansiachuthan@ is going to update all the commits to use a personal gmail account (doing something like this: https://stackoverflow.com/a/750182) and to sign the individual Google CLA with that email address here: https://cla.developers.google.com/clas. It's currently using the default no-reply email. Looks like go/github used to associate the @google.com email to mansiachuthan@'s github username, but no longer does. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
We currently do not support the output of a BigQuery destination uri for the exported evaluated examples from our training step when creating an AutoML Forecast model. Adding this feature would allow users to easily access the evaluated examples and use them for data visualization.