[Schedule] Synchronize labels on upgrade #6389
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
quaark
requested changes
Sep 23, 2024
server/api/initial_data.py
Outdated
Comment on lines
906
to
931
| def _align_schedule_labels(db, db_session): | ||
| db_schedules: list[mlrun.common.schemas.ScheduleRecord] = db.list_schedules( | ||
| session=db_session | ||
| ) | ||
| schedules_update = [] | ||
| for db_schedule in db_schedules: | ||
| # convert list[LabelRecord] to dict | ||
| db_schedule_labels = {label.name: label.value for label in db_schedule.labels} | ||
| # merging labels | ||
| merged_labels = server.api.utils.scheduler.Scheduler()._merge_schedule_and_schedule_object_labels( | ||
| labels=db_schedule_labels, | ||
| scheduled_object=db_schedule.scheduled_object, | ||
| ) | ||
|
|
||
| # get a Schedule object (not a ScheduleRecord) and update it | ||
| schedule = db._get_schedule_record( | ||
| db_session, db_schedule.project, db_schedule.name | ||
| ) | ||
| db._update_schedule_body( | ||
| schedule=schedule, | ||
| scheduled_object=db_schedule.scheduled_object, | ||
| labels=merged_labels, | ||
| ) | ||
| schedules_update.append(schedule) | ||
|
|
||
| db._upsert(db_session, schedules_update) |
There was a problem hiding this comment.
- This method is very expensive. It is going to preform
O(n)queries for each schedule in the DB. I suggest adding aas_records: bool = False,param tolist_schedules(similar tolist_artifacts). You can still extract the schedule struct with_transform_schedule_record_to_schemebut then you don't need to re-query for each record after getting them all. - This method is using loads of the
db's private methods, I don't think we should make them public, but maybe this method should be part of thesqldbinterface?
alonmr
reviewed
Sep 23, 2024
server/api/initial_data.py
Outdated
| # convert list[LabelRecord] to dict | ||
| db_schedule_labels = {label.name: label.value for label in db_schedule.labels} | ||
| # merging labels | ||
| merged_labels = server.api.utils.scheduler.Scheduler()._merge_schedule_and_schedule_object_labels( |
There was a problem hiding this comment.
Perhaps use ensure_scheduler and get_scheduler just for consistency
server/api/initial_data.py
Outdated
Comment on lines
907
to
911
| db_schedules: list[mlrun.common.schemas.ScheduleRecord] = db.list_schedules( | ||
| session=db_session | ||
| ) | ||
| schedules_update = [] | ||
| for db_schedule in db_schedules: |
There was a problem hiding this comment.
I'm thinking that we need to optimize this for scale systems. I would take the following steps:
- Add
as_recordsflag similar tolist_artifacts. - Use sqlalchemy lazy load - return the query and then for loop on the query instead of using query.all() and loading all schedules in memory.
- Transform the DB schedule to schema schedule then you have both and you don't need to get the DB one (saves us O(n) queries to the DB/cache).
Suggested change
| db_schedules: list[mlrun.common.schemas.ScheduleRecord] = db.list_schedules( | |
| session=db_session | |
| ) | |
| schedules_update = [] | |
| for db_schedule in db_schedules: | |
| schedules_update = [] | |
| for db_schedule in db.list_schedules(session=db_session, as_records=True): | |
| schedule_record = db._transform_schedule_record_to_scheme(db_schedule) |
tests/api/test_initial_data.py
Outdated
| @@ -228,6 +237,39 @@ def test_create_project_summaries(): | |||
| assert migrated_project_summary.name == project.metadata.name | |||
|
|
|||
|
|
|||
| def test_align_schedule_labels(): | |||
There was a problem hiding this comment.
please add some more schedules with edge cases. we need to make sure this will not break in field
alonmr
approved these changes
Sep 23, 2024
liranbg
approved these changes
Sep 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After implementing ML-7349, which aligns the labels in schedules and schedule objects, we need to account for objects created prior to this change. To address this, a new data migration (version 8) has been added. This migration retrieves all schedules from the database and aligns their labels accordingly.
Jira - https://iguazio.atlassian.net/browse/ML-7914