-
Notifications
You must be signed in to change notification settings - Fork 462
Training Configurations Add/Update implementation #4873
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
Docker Image Sizes
|
📊 Test coverage report
|
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 implements GET and POST operations for training configurations, adding a dedicated service layer and endpoints. The implementation replaces placeholder endpoints with full functionality that supports retrieving and updating training configurations for projects and specific model architectures.
- Added complete service layer with
TrainingConfigurationService
for managing training configurations - Implemented database repository and schema for persisting training configurations
- Created dedicated API endpoints with proper error handling and validation
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
test_training_configurations.py |
Unit tests for training configuration endpoints |
test_training_configuration_service.py |
Integration tests for the training configuration service |
test_initial_schema_migration.py |
Updated test to verify new training_configurations table |
training_configuration_service.py |
Core service implementing configuration retrieval and update logic |
training_configuration.py |
Enhanced schema with new factory methods and detailed examples |
training_configuration_repo.py |
Repository for database operations on training configurations |
schema.py |
Added TrainingConfigurationDB model with unique constraints |
training_configurations.py |
New dedicated API endpoints for training configuration operations |
projects.py |
Removed placeholder training configuration endpoints |
dependencies.py |
Added dependency injection for training configuration service |
da385d690aae_schema.py |
Database migration to create training_configurations table |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
application/backend/app/alembic/versions/da385d690aae_schema.py
Outdated
Show resolved
Hide resolved
application/backend/app/repositories/training_configuration_repo.py
Outdated
Show resolved
Hide resolved
application/backend/tests/integration/services/test_training_configuration_service.py
Outdated
Show resolved
Hide resolved
application/backend/tests/integration/services/test_training_configuration_service.py
Outdated
Show resolved
Hide resolved
""" | ||
existing = self.get_by_project_and_model_architecture(project_id, model_architecture_id) | ||
|
||
if existing: |
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.
Do we need existing
check - will it not be able to detect whether to generate update/insert
based on provided ids by default?
application/backend/app/services/training_configuration_service.py
Outdated
Show resolved
Hide resolved
"evaluation": {"config": {"metrics": ["accuracy", "precision", "recall"], "validation_split": 0.2}}, | ||
"dataset_preparation": { | ||
"augmentation": { | ||
"topdown_affine": None, |
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.
Null-valued parameters should be filtered out from the response, because in this context None
represents a parameter that's not applicable to the algorithm.
I believe Geti already implements a similar feature. This is an example of response, as you can see there are some empty sections (-> this algorithm doesn't support any augmentation)

"gaussian_noise": {"enable": False, "mean": 0, "sigma": 0.1, "probability": 0.5}, | ||
"tiling": {"enable": False, "adaptive_tiling": True, "tile_size": 400, "tile_overlap": 0.2}, | ||
} | ||
}, |
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.
I think the subset_split
category is missing?
} | ||
}, | ||
"training": { | ||
"max_epochs": 200, |
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.
The REST endpoint should return a richer representation of each parameter, including the name, description, type, min/max and default values, etc... These details are necessary to the UI to correctly display the parameter.
For reference, here is how the max_epochs
parameter looks like in Geti response:

…nfiguration retrieval, and use model_copy for model updating
Summary
Add the implementation for GET and POST of the training configuration
How to test
http://geti-tune.localhost/api/projects/165f79f9-fba5-415b-adda-7e6548ed2c40/training_configuration
Checklist