Implement Vertex AI Model Monitoring v2 - #39738
Conversation
|
Assigning reviewers: R: @shunping for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
Thanks! Overall this looks good to me, had a few specific questions/comments
| _ = ( | ||
| results | ||
| | 'BeamML_RunInference_MonitoringOutlet' >> | ||
| self._monitoring_transform) |
There was a problem hiding this comment.
Does the monitoring track error rates as well? If yes, we should probably find a way to route the bad inferences here as well.
There was a problem hiding this comment.
No error rates at the moment. If that ever changes we could probably add similar optional branching to the DLQ.
| explanation_spec: Optional[Any] = None, | ||
| output_spec: Optional[Any] = None, | ||
| notification_spec: Optional[Any] = None, | ||
| credentials: Optional[Any] = None, |
There was a problem hiding this comment.
Should we accept arbitrary kwargs? That way if any arguments are added in the future, it provides a workaround even if we don't add it to the base beam implementation
There was a problem hiding this comment.
Reasonable change, I've plumbed those through.
| location=self.location, | ||
| credentials=self.credentials, | ||
| ) | ||
| monitors = ml_monitoring.model_monitors.ModelMonitor.list( |
There was a problem hiding this comment.
Do we need any sort of backoff on this if it isn't found? I'm not sure how long monitors take to create, but what happens if you have 2 DoFns, A and B with setup called at the same time. Could you end up with:
A -> create monitor (starts)
B -> create monitor (fails, conflict)
B -> list monitors (finds nothing, throws)
A -> create monitor (finishes)
There was a problem hiding this comment.
In my experience the creation happens pretty quickly, but adding a little backoff in the Conflict case is a reasonable way to mitigate potential race conditions. Added.
There was a problem hiding this comment.
LGTM. The failure case isn't that bad since setup would be retried, but it would be nice to avoid an error in this case
|
|
||
| if is_streaming: | ||
| if not self.cron: | ||
| raise ValueError( |
There was a problem hiding this comment.
Within the current approach, could a user have an existing monitoring job and just point this to populate the correct BQ table? Or is there a reason we couldn't support this?
The cron piece made me think of this since a cron schedule wouldn't be needed if you configured the job already.
This would potentially allow multiple streaming pipelines to feed the same monitoring job (e.g. if you have the same model deployed in multiple regions but wanted a single monitoring job)
There was a problem hiding this comment.
In this form yes, but with the caveat that the entire configuration would need to be completely identical. I took a quick run at allowing the omission of a cron schedule to effectively skip the monitoring transform altogether (and log a warning for the user that this happened.) We still route the inferences to the BQ table, but don't worry about anything else.
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
Just one more follow up, thanks
| location=self.location, | ||
| credentials=self.credentials, | ||
| ) | ||
| monitors = ml_monitoring.model_monitors.ModelMonitor.list( |
There was a problem hiding this comment.
LGTM. The failure case isn't that bad since setup would be retried, but it would be nice to avoid an error in this case
| 'A cron schedule was not provided, so a monitoring job will not be' | ||
| ' created. Inferences will still be written to the BigQuery table' | ||
| ' %s', | ||
| self.bigquery_table) |
There was a problem hiding this comment.
Sorry, should've mentioned this before - can we do a lookup in this case to determine if a job already exists for this model name/version with a cron schedule? If not, then we should at least force a user to specify a value in this case (e.g. cron is not optional, but pass a constant in to skip creation) - otherwise I'd worry about users accidentally falling into this case
2 participants
Implementation of https://s.apache.org/beam-python-vertex-model-monitoring for model monitoring v2.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.