jrmccluskey commented on code in PR #39738:
URL: https://github.com/apache/beam/pull/39738#discussion_r3823478010
##########
sdks/python/apache_beam/ml/inference/vertex_ai_model_monitoring_v2.py:
##########
@@ -193,7 +193,38 @@ def __init__(
def setup(self):
self.manager = self.create_model_monitor()
+ def _schedule_already_exists(self) -> bool:
+ """Checks if an identical schedule already exists on the model monitor."""
+ try:
+ existing_schedules = self.manager.list_schedules()
+ if not existing_schedules:
+ return False
+ for schedule in existing_schedules:
+ sched_display_name = getattr(schedule, 'display_name', None)
+ sched_cron = getattr(schedule, 'cron', None)
+ if isinstance(schedule, dict):
+ sched_display_name = schedule.get('display_name', sched_display_name)
+ sched_cron = schedule.get('cron', sched_cron)
+ if (sched_display_name == self.schedule_display_name and
+ sched_cron == self.cron):
Review Comment:
Ah I misread the comment around the cron arg (it should be non-optional IFF
we can't check for schedule existence.) I guess that comes down to behavior
here:
If cron provided -> check that the provided cron and existing cron are equal
If cron not provided -> ignore the cron equality check and just check
display name
This behavior is fine. I would assume that the former pattern is going to be
more common, with all of the args for the model monitoring being defined as
consts in a pipeline definition, but having some flexibility is not bad. The
only real additional consideration would be if we allowed users to update
existing schedules by passing alternate crons + some sort of boolean, but I
think that's out of scope for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
