Skip to content

Conversation

@GitHK
Copy link
Contributor

@GitHK GitHK commented Aug 7, 2025

What do these changes do?

Unfortunately this is a very noisy pull request and I've tried my best to summarise here the most imporntat parts which changed, I would request to read this full description.
The amount of changes is mostly due to adjusting tests after these changes.


The change per se, is staring forward.
The code inside servicelib/long_running_tasks/lrt_api.py was split across an RPC interface:

  • added _rabbit/lrt_client.py part which is called by the lrt_api.py
  • added _rabbit/lrt_server.py which replies to the requests from _rabbit/lrt_client.py

BEFORE

  • direct access only to HTTP interface which calls internally lrt_api
  • lrt_api calls required an instance of TasksManager (part of the long running tasks internals)
flowchart LR
    subgraph HTTP Interface
        A[HTTP Request]
    end
    subgraph LRT Interface
        B[lrt_api]
    end
    subgraph TasksManager
        C[Task Processor]
    end
    A <--> B
    B <--> C

Loading

AFTER

  • direct access to HTTP interface which calls internally lrt_api
  • direct access to RPC interface (via lrt_api)
  • lrt_api calls require a generic RabbitMQRPCClient enabling them to also be called from other services
flowchart LR
    subgraph HTTP Interface
        A[HTTP Request]
    end
    subgraph LRT Interface
        E[lrt_api]
        B[lrt_client]
        C[lrt_server]
    end
    subgraph TasksManager
        D[Task Processor]
    end
    A <--> E
    E <--> B
    B <--> C
    C <--> D

Loading

Highlights of most important changes:

  • 🎨 made task cancellation more reliable
  • 🎨 TaskRegistry.register now registers a partial, since some objects cannot be serialised and sent over via RabbitMQ
  • ♻️ refactored packages/service-library/src/servicelib/long_running_tasks/_store/redis.py internals to deal with concurrency
  • ♻️ replaced cancel_and_delete_task with remove_task in aiohttp, FastAPI path names and OpenAPI specs (triggered regeneration of openapi-specs)
  • ♻️ replaced TasksManager with RabbitMQRPCClient inside lrt_api interface
  • ♻️ webserver's long_running_tasks module now has settings and is now a full module

Most important files changed (🙏 extra attention here):

  • packages/service-library/src/servicelib/long_running_tasks/lrt_api.py
  • packages/service-library/src/servicelib/long_running_tasks/_rabbit/ all files in module
  • packages/service-library/src/servicelib/long_running_tasks/task.py this is where the cancellation was addressed
  • services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py
  • services/web/server/src/simcore_service_webserver/long_running_tasks/ all files in module
  • services/docker-compose.yml since the long_running_tasks in the webserver have to use different namespaces, because the image is used for 4 different services

Related issue/s

How to test

Dev-ops ⚠️🚨

  • 🚨 remove long_running_tasks database form redis-commander when deploying (due to models changing)

@GitHK GitHK self-assigned this Aug 7, 2025
@GitHK GitHK added a:storage issue related to storage service a:webserver webserver's codebase. Assigning the area is particularly useful for bugs a:services-library issues on packages/service-libs a:director-v2 issue related with the director-v2 service a:dynamic-sidecar dynamic-sidecar service labels Aug 7, 2025
@GitHK GitHK added this to the Voyager milestone Aug 7, 2025
@codecov
Copy link

codecov bot commented Aug 7, 2025

Codecov Report

❌ Patch coverage is 94.60154% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.03%. Comparing base (8f96782) to head (9fc652a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8198      +/-   ##
==========================================
+ Coverage   87.99%   88.03%   +0.04%     
==========================================
  Files        1917     1919       +2     
  Lines       74169    74311     +142     
  Branches     1301     1305       +4     
==========================================
+ Hits        65264    65423     +159     
+ Misses       8514     8495      -19     
- Partials      391      393       +2     
Flag Coverage Δ
integrationtests 63.90% <72.83%> (-0.02%) ⬇️
unittests 86.68% <94.60%> (+0.04%) ⬆️
Components Coverage Δ
pkg_aws_library 93.93% <ø> (ø)
pkg_celery_library 87.37% <ø> (ø)
pkg_dask_task_models_library 79.62% <ø> (ø)
pkg_models_library 93.05% <ø> (ø)
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.02% <ø> (ø)
pkg_service_integration 70.19% <ø> (ø)
pkg_service_library 72.35% <93.50%> (+0.53%) ⬆️
pkg_settings_library 90.17% <ø> (ø)
pkg_simcore_sdk 84.97% <ø> (-0.06%) ⬇️
agent 93.90% <ø> (ø)
api_server 92.84% <ø> (ø)
autoscaling 95.89% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 92.37% <ø> (+0.56%) ⬆️
datcore_adapter 97.94% <ø> (ø)
director 75.81% <ø> (ø)
director_v2 90.87% <100.00%> (-0.12%) ⬇️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.10% <100.00%> (+0.04%) ⬆️
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.61% <ø> (ø)
resource_usage_tracker 92.13% <ø> (+0.05%) ⬆️
storage 86.61% <ø> (+0.27%) ⬆️
webclient ∅ <ø> (∅)
webserver 88.10% <98.11%> (+0.04%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f96782...9fc652a. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GitHK GitHK added a:dynamic-scheduler and removed a:storage issue related to storage service a:director-v2 issue related with the director-v2 service labels Aug 7, 2025
Copy link
Contributor

Copilot AI left a 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 refactors the long-running tasks framework to add RabbitMQ RPC interface support while maintaining the existing HTTP interface. The main purpose is to enable calling long-running tasks from other services via RabbitMQ RPC instead of just through HTTP.

  • Splits the long-running tasks API into client and server RPC components communicating via RabbitMQ
  • Improves task cancellation reliability and error handling
  • Refactors task registration to use partials for serialization compatibility
  • Renames API endpoints from cancel_and_delete_task to remove_task

Reviewed Changes

Copilot reviewed 95 out of 96 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
services/web/server/src/simcore_service_webserver/long_running_tasks/ New modular structure with settings and plugin setup for webserver LRT integration
services/web/server/src/simcore_service_webserver/projects/ Updates project creation and node operations to use new RPC client interface
services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py Refactored to register tasks with context and use new RPC-based setup
services/storage/src/simcore_service_storage/ Removed unused long-running tasks module
packages/service-library/src/servicelib/long_running_tasks/ Core refactoring with new RPC client/server, improved Redis store, and enhanced task management
Comments suppressed due to low confidence (1)

packages/service-library/src/servicelib/long_running_tasks/task.py:59

  • Class name has a typo: _TetingError should be _TestingError.
    ) -> Any: ...

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some last suggestions, let's align the result in the client with our other frameworks so they are used the same way.
also did you check what happens in storage as we discussed yesterday?

@GitHK GitHK requested a review from sanderegg August 22, 2025 08:22
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx

@GitHK GitHK added the 🤖-automerge marks PR as ready to be merged for Mergify label Aug 22, 2025
@GitHK
Copy link
Contributor Author

GitHK commented Aug 22, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Aug 22, 2025

queue

🛑 Configuration not compatible with a branch protection setting

The branch protection setting Require branches to be up to date before merging is not compatible with max_parallel_checks>1, queue_conditions != merge_conditions and must be unset.

@sonarqubecloud
Copy link

@GitHK GitHK merged commit 0bbcd00 into ITISFoundation:master Aug 22, 2025
95 checks passed
@GitHK GitHK deleted the pr-osparc-long-running-rabbitmq-client branch August 22, 2025 12:16
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Sep 2, 2025
61 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:dynamic-scheduler a:dynamic-sidecar dynamic-sidecar service a:services-library issues on packages/service-libs a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants