feat: support model monitoring for batch prediction in Vertex SDK #1570
Conversation
product-auto-label
bot
added
size: l
| Optional. The objective config for model monitoring. Passing this parameter enables | ||
| monitoring on the model associated with this batch prediction job. | ||
| model_monitoring_alert_config (aiplatform.model_monitoring.EmailAlertConfig): | ||
| Optional. Configures how model monitoring alerts are sent to the user. Right now |
There was a problem hiding this comment.
Are these docstrings copied from the source at https://github.com/googleapis/googleapis/tree/master/google/cloud/aiplatform? Will we be able to remember to update this when/if alerts other than email alert become supported?
There was a problem hiding this comment.
They're not directly copied from GAPIC. But Jing's team also confirmed that there's no plans for additional alert configs.
google/cloud/aiplatform/jobs.py
Outdated
| @@ -702,6 +759,11 @@ def create( | |||
| sync=sync, | |||
| create_request_timeout=create_request_timeout, | |||
| ) | |||
| # TODO: remove temporary re-import statements once model monitoring for batch prediction is GA | |||
There was a problem hiding this comment.
Since there are several of these throughout the PR, perhaps add a tracking bug number so the TODO comments look like # TODO(b/.....): remove... with a common bug number.
There was a problem hiding this comment.
buganizer component created & assigned
| @@ -17,9 +17,16 @@ | |||
|
|
|||
| from typing import Optional, List | |||
| from google.cloud.aiplatform_v1.types import ( | |||
| model_monitoring as gca_model_monitoring, | |||
| model_monitoring as gca_model_monitoring_v1, | |||
There was a problem hiding this comment.
Should we use compat for these imports?
There was a problem hiding this comment.
compat by default imports from GA version of GAPIC, unless if DEFAULT_VERSION is set to v1beta1. I tried to see if the import aliases will index into the correct version by simply setting DEFAULT_VERSION = 'v1beta1' and then switching it back to v1 on an ad-hoc basis, but it doesn't dynamically index in the way I was hoping for. I think it's because the symbol table isn't automatically re-written unless if we explicitly re-import. So that's why I imported both v1 and v1beta1 versions explicitly.
| def as_proto(self): | ||
| """Returns EmailAlertConfig as a proto message.""" | ||
| # TODO: remove config_for_bp parameter when model monitoring for batch prediction is GA | ||
| def as_proto(self, config_for_bp: Optional[bool] = False): |
There was a problem hiding this comment.
This typing annotation should be just config_for_bp: bool = False.
| def as_proto(self): | ||
| """Returns _ObjectiveConfig as a proto message.""" | ||
| # TODO: remove config_for_bp parameter when model monitoring for batch prediction is feature complete and in GA | ||
| def as_proto(self, config_for_bp: Optional[bool] = False): |
There was a problem hiding this comment.
Same comment as above about typing annotation here.
| io as gca_io, | ||
| model_monitoring as gca_model_monitoring, | ||
| ) | ||
|
|
||
| # constants used for testing | ||
| USER_EMAIL = "" | ||
| MODEL_NAME = "churn" |
There was a problem hiding this comment.
Is this used as the model's display_name? if so maybe better call it MODEL_DISPLAY_NAME, since "model name" could be understood as its resource full name.
If this is indeed intended as a display name of a resource used only for testing, please prefix it with "temp".
There was a problem hiding this comment.
I updated the variable names to make it more specific. But the extra prefixing is unnecessary because the actual display names are being created by _make_display_name implemented in e2e_base.py, which does append a prefix set by the class (in this case the prefix is 'temp_e2e_model_monitoring_test_')
There was a problem hiding this comment.
Thanks. In that case can we rename this further and call it something like MODEL_DISPLAYNAME_KEY to prevent the next engineer reading this to misinterpret as the actual model displayname? More generally, what’s the meaning of “churn” in the context of testing?
There was a problem hiding this comment.
I've updated the variable name. Regarding the actual value string, "churn" is just a shorthand used to reference the BQ dataset. The same dataset and pre-trained model was used in the example notebook on the Cloud SDK documentation page and a separate example for BQML.
rosiezou
added
kokoro:force-run
yoshi-kokoro
removed
kokoro:force-run
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕