-
Notifications
You must be signed in to change notification settings - Fork 458
Fix inconsistent key names in PATCH pipeline endpoint #5089
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR renames the model_revision_id field to model_id in the Pipeline model to improve API consistency, while maintaining backward compatibility by accepting both field names during validation. The change also includes adding support for detecting model changes in running pipelines through a new event type.
- Renamed
model_revision_idtomodel_idwith backward-compatible validation usingAliasChoices - Added
MODEL_CHANGEDevent type and corresponding event handling logic - Updated all references throughout the codebase to use the new
model_idfield name
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| application/docs/models.md | Fixed grammatical error ("an parent-child" → "a parent-child") |
| application/backend/app/models/pipeline.py | Renamed field from model_revision_id to model_id with alias support for backward compatibility |
| application/backend/app/services/pipeline_service.py | Updated to use model_id and added MODEL_CHANGED event emission logic |
| application/backend/app/services/pipeline_metrics_service.py | Updated references from model_revision_id to model_id |
| application/backend/app/services/event/event_bus.py | Added MODEL_CHANGED event type and notification logic |
| application/backend/tests/unit/services/event/test_event_bus.py | Added test coverage for MODEL_CHANGED event |
| application/backend/tests/integration/services/test_pipeline_service.py | Updated test assertions and added new test for model switching with both field name aliases |
| application/backend/tests/integration/services/test_dataset_service.py | Updated test references from model_revision_id to model_id |
| application/backend/tests/conftest.py | Updated fixture to use model_id instead of model_revision_id |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Test coverage report
|
Docker Image SizesCPU
GPU
XPU
|
| pipeline = self.get_pipeline_by_id(project_id) | ||
| to_update = type(pipeline).model_validate(pipeline.model_copy(update=partial_config)) | ||
| base = pipeline.model_dump() | ||
| to_update = type(pipeline).model_validate({**base, **partial_config}) |
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.
Pipeline is already imported in this file, why not use it again?
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.
Both are valid. But type(pipeline).model_validate(...) validates with the concrete runtime model returned by get_pipeline_by_id(), which makes this code more resilient to refactors (e.g., subclasses) than hard-coding Pipeline.model_validate(...).
Summary
model_revision_idtomodel_idand allow both aliases when validating modelChecklist