- 
                Notifications
    
You must be signed in to change notification settings  - Fork 32
 
♻️✨🐛Dask-Sidecar: add RabbitMQ dependency and remove usage of deprecated Pub/Sub for logs 🚨🚨🚨 #7621
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
Conversation
          Codecov ReportAttention: Patch coverage is  
 Additional details and impacted files@@            Coverage Diff             @@
##           master    #7621      +/-   ##
==========================================
+ Coverage   84.00%   88.43%   +4.42%     
==========================================
  Files        1788     1766      -22     
  Lines       68985    67869    -1116     
  Branches     1134     1134              
==========================================
+ Hits        57951    60020    +2069     
+ Misses      10722     7538    -3184     
+ Partials      312      311       -1     
 
 Continue to review full report in Codecov by Sentry. 
 🚀 New features to boost your workflow:
  | 
    
63ff44a    to
    68d3802      
    Compare
  
    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 replaces the deprecated Dask Pub/Sub logging mechanism with a RabbitMQ-based implementation and refactors related settings, logging, and error handling. Key changes include updating TaskPublisher to use asynchronous RabbitMQ messaging for logs, revising settings import and initialization, and modifying test and deployment configurations accordingly.
Reviewed Changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description | 
|---|---|
| services/dask-sidecar/src/simcore_service_dask_sidecar/utils/dask.py | Updated TaskPublisher to send logs directly via RabbitMQ instead of using TaskLogEvent. | 
| services/dask-sidecar/src/simcore_service_dask_sidecar/settings.py | Refactored settings to use ApplicationSettings and added RabbitMQ settings. | 
| services/dask-sidecar/src/simcore_service_dask_sidecar/scheduler.py | Adjusted scheduler logging and settings retrieval. | 
| services/dask-sidecar/src/simcore_service_dask_sidecar/rabbitmq_plugin.py | Introduced RabbitMQPlugin for managing RabbitMQ connectivity in a dask-worker. | 
| Other files | Minor updates for asynchronous usage and configuration consistency across the codebase. | 
Files not reviewed (1)
- services/dask-sidecar/docker/boot.sh: Language not supported
 
Comments suppressed due to low confidence (1)
services/dask-sidecar/src/simcore_service_dask_sidecar/utils/dask.py:110
- When publishing parent logs, the wrong message object is passed; it should use 'parent_message' instead of 'base_message'. Consider replacing 'base_message' with 'parent_message' in the call.
 
await rabbitmq_client.publish_message_from_any_thread(parent_message.channel_name, base_message)
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.
Thanks 👍
Q:
- Is the worker the one who talks directly to Rabbit? (Is there a use case where the manager also needs to communicate?)
 - What happens if this is an external cluster? Will we still have access to Rabbit? Will it only work if Rabbit is exposed? So if we run Rabbit as part of our Simcore stack, it won’t work?
 
          
 @matusdrobuliak66 as stated in the description: 
  | 
    
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.
Read the description, thanks a lot. Already approving to unblock, ping me in case you'd like a thorough code review no problem:)
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.
👍
          
 | 
    



What do these changes do?
Dask deprecated their Pub/Sub mechanism and retired it from their code recently. We are using this mechanism for transfering logs and progress events from the dask-sidecar to the director-v2. Also logs are only forwarded to RabbitMQ from the director-v2, thus loading director-v2 for no added value.
Next step: removing the progress Pub/Sub mechanism
This PR aims at replacing this mechanism by:
Details
dask-task-models-library, 'dask-sidecar', 'director-v2'Bonus Fixes:
startedcolumn not filled correctly (now filled when job starts instead of when the computation is created)Related issue/s
How to test
Dev-ops
RABBIT_ENVs variables to access the 🐰computational_services_subnetdocker network such that dask-sidecar can access it