♻️ preparing TasksManager's interface to be extracted into a common interface
#7884
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7884 +/- ##
==========================================
+ Coverage 88.14% 88.27% +0.13%
==========================================
Files 1869 1677 -192
Lines 72052 67620 -4432
Branches 1279 947 -332
==========================================
- Hits 63507 59692 -3815
+ Misses 8165 7638 -527
+ Partials 380 290 -90
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
GitHK
changed the title
TasksManager's interface to be extracted into a common interface
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the TasksManager interface to simplify unique task handling and prepares it to be extracted into a common interface. Key changes include simplifying progress update calls by removing explicit ProgressPercent wrappers, replacing direct start_task calls with lrt_api.start_task using function names, and updating tests and API endpoints to align with these changes.
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py | Replaced ProgressPercent wrapped values with numeric literals and added TaskRegistry import. |
| services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/api/rest/containers_long_running_tasks.py | Updated start_task calls to await lrt_api.start_task and pass task function names. |
| services/director-v2/src/simcore_service_director_v2/api/routes/dynamic_scheduler.py | Refactored task starting logic with lrt_api and adjusted TaskRegistry registration/unregistration. |
| packages/service-library/* | Refactored multiple tests and tasks to use lrt_api, updated HTTP status expectations, and removed deprecated task_name usage in models. |
| packages/service-library/src/servicelib/long_running_tasks/task.py | Updated start_task implementation and unique task ID logic using TaskRegistry. |
| packages/models-library/src/models_library/api_schemas_long_running_tasks/tasks.py | Removed task_name from TaskBase to align with the refactored interface. |
Comments suppressed due to low confidence (2)
services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py:152
- Since the ProgressPercent wrapper has been removed in favor of passing numeric literals directly, consider updating the function's docstring and any related documentation to clearly indicate that the 'percent' parameter now expects a float value.
progress.update(message="started pulling user services", percent=0)
packages/models-library/src/models_library/api_schemas_long_running_tasks/tasks.py:21
- The removal of the 'task_name' field from TaskBase could affect clients that expect this information. Please ensure that the API documentation and any client integrations are updated accordingly to reflect this change.
class TaskBase(BaseModel):
packages/models-library/src/models_library/api_schemas_long_running_tasks/tasks.py
Outdated
Show resolved
Hide resolved
packages/models-library/tests/test_api_schemas_long_running_tasks_tasks.py
Outdated
Show resolved
Hide resolved
packages/service-library/tests/fastapi/long_running_tasks/test_long_running_tasks.py
Show resolved
Hide resolved
packages/models-library/tests/test_api_schemas_long_running_tasks_tasks.py
Outdated
Show resolved
Hide resolved
packages/models-library/src/models_library/api_schemas_long_running_tasks/tasks.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/long_running_tasks/errors.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/long_running_tasks/task.py
Outdated
Show resolved
Hide resolved
|
|
packages/models-library/src/models_library/api_schemas_long_running_tasks/tasks.py
Show resolved
Hide resolved
...ynamic-sidecar/src/simcore_service_dynamic_sidecar/api/rest/containers_long_running_tasks.py
Show resolved
Hide resolved
GitHK
added
🤖-automerge
|
@Mergifyio queue |
🛑 The pull request has been removed from the queue
|
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks:You may have to fix your CI before adding the pull request to the queue again. |
|
@Mergifyio requeue |
✅ This pull request will be re-embarked automaticallyThe followup |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at ae5f894 |
…osparc-simcore-forked into pr-osparc-long-running-refactor-4
|
What do these changes do?
Completely separated the interface of the
TaskManagerform the "public interface" which the user facing API expose placed inpackages/service-library/src/servicelib/long_running_tasks/lrt_api.py. No more internals exposed form theTaskManagerto the outside world.uniquetasks are handled in a simpler manner, they are simple determined by their task_id and are no longer kept track via complex data structuresstart_taskis now part of the "public interface" and no longer a utility bundled with theTaskManagerTasksRegistrybefore starting them via theTaskManagerIn future developments, these changes will allow, without altering the current "public interface" to:
long_running_taskswith the celery workersRelated issue/s
How to test
Dev-ops