Skip to content

Conversation

@sanderegg
Copy link
Member

@sanderegg sanderegg commented May 12, 2025

What do these changes do?

This PR introduces several updates to the Dask-sidecar and Director-v2 and their associated tests.
The changes focus on transitioning from Dask's deprecated Pub/Sub mechanism to their recommended event-based communication. Additionally, minor refactoring and cleanup have been performed to streamline the codebase.

Key Changes:

Transition from Pub/Sub to Event-Based (using log_event) Communication:

  • Replaced Dask's Pub/Sub mechanism with event-based communication using worker.log_event for publishing task progress and logs. ([services/dask-sidecar/src/simcore_service_dask_sidecar/utils/dask.pyL172-R181](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-27830c4191d3861064437bb2474d4cbd8e1d90a2d3b0e8217c028e14fb674a2bL172-R181))
  • Removed all references to distributed.Pub and distributed.Sub throughout the codebase and tests. ([[1]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-27830c4191d3861064437bb2474d4cbd8e1d90a2d3b0e8217c028e14fb674a2bL66-L83), [[2]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-c66b25f1d5f48cb131389bf7b17fbe391d0bd704a54c10715d617d8684851a14L109-L116), [[3]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-c66b25f1d5f48cb131389bf7b17fbe391d0bd704a54c10715d617d8684851a14L141))

Test Suite Updates:

  • Refactored tests to use event handlers instead of Sub for verifying task progress and event publishing. Added helper functions like _assert_parse_progresses_from_progress_event_handler to validate progress ordering and completeness. ([[1]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-29344dd3643e46b1bd55fb9f0d6cda0bec4a8131a71154f17ab0cb833ac41880L40-R121), [[2]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-c66b25f1d5f48cb131389bf7b17fbe391d0bd704a54c10715d617d8684851a14L644-R650), [[3]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-c66b25f1d5f48cb131389bf7b17fbe391d0bd704a54c10715d617d8684851a14L760-R757))
  • Consolidated sync and async Dask client tests into a single parameterized test fixture (dask_client_multi) to reduce redundancy. ([services/dask-sidecar/tests/unit/test_utils_dask.pyL40-R121](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-29344dd3643e46b1bd55fb9f0d6cda0bec4a8131a71154f17ab0cb833ac41880L40-R121))
  • Updated test utilities like _wait_for_task_to_start and _notify_task_is_started_and_ready to use the Dask client explicitly. ([[1]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-29344dd3643e46b1bd55fb9f0d6cda0bec4a8131a71154f17ab0cb833ac41880L179-R139), [[2]](https://github.com/ITISFoundation/osparc-simcore/pull/7660/files#diff-29344dd3643e46b1bd55fb9f0d6cda0bec4a8131a71154f17ab0cb833ac41880L189-R149))

Next PR:

Related issue/s

How to test

Dev-ops

@sanderegg sanderegg added this to the Bazinga! milestone May 12, 2025
@sanderegg sanderegg self-assigned this May 12, 2025
@sanderegg sanderegg added the a:dask-service Any of the dask services: dask-scheduler/sidecar or worker label May 12, 2025
@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.00%. Comparing base (50a4875) to head (355cf27).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (50a4875) and HEAD (355cf27). Click for more details.

HEAD has 32 uploads less than BASE
Flag BASE (50a4875) HEAD (355cf27)
unittests 34 2
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #7660       +/-   ##
===========================================
- Coverage   87.55%   70.00%   -17.55%     
===========================================
  Files        1802      711     -1091     
  Lines       70140    33809    -36331     
  Branches     1137      170      -967     
===========================================
- Hits        61409    23669    -37740     
- Misses       8421    10082     +1661     
+ Partials      310       58      -252     
Flag Coverage Δ
integrationtests 64.51% <100.00%> (+0.05%) ⬆️
unittests 85.18% <100.00%> (-1.60%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 77.53% <ø> (-8.13%) ⬇️
agent ∅ <ø> (∅)
api_server ∅ <ø> (∅)
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar 89.86% <100.00%> (-0.03%) ⬇️
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 91.13% <100.00%> (-0.04%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 89.06% <ø> (-1.10%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 58.61% <ø> (-27.20%) ⬇️

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 50a4875...355cf27. 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.

@sanderegg sanderegg force-pushed the dask-sidecar/remove-deprecated-pub-sub branch from f9ae6c7 to 33b4faa Compare May 15, 2025 12:19
@sanderegg sanderegg force-pushed the dask-sidecar/remove-deprecated-pub-sub branch from c810815 to 355cf27 Compare May 15, 2025 13:26
@sanderegg sanderegg added the a:director-v2 issue related with the director-v2 service label May 15, 2025
@sonarqubecloud
Copy link

@sanderegg sanderegg marked this pull request as ready for review May 15, 2025 13:31
@sanderegg sanderegg requested review from GitHK and pcrespov as code owners May 15, 2025 13:31
@sanderegg sanderegg requested a review from Copilot May 15, 2025 13:42
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 removes Dask’s legacy Pub/Sub mechanism in favor of an event-based workflow using worker.log_event. Key changes include updating handlers and tests to work with the new event tuples, removing deprecated Pub/Sub usages, and refactoring related utilities and client registrations.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
services/director-v2/tests/unit/with_dbs/comp_scheduler/test_scheduler_dask.py Updated progress event handling to use tuples with arrow timestamps
services/director-v2/tests/unit/test_modules_dask_client.py Removed assertions on deprecated _subscribed_tasks and adjusted type annotations
services/director-v2/src/simcore_service_director_v2/utils/dask_client_utils.py Modified TaskHandlers to accept progress events as tuples
services/director-v2/src/simcore_service_director_v2/utils/dask.py Removed legacy Pub/Sub consumer logic
services/director-v2/src/simcore_service_director_v2/modules/dask_client.py Replaced Pub/Sub-based registration with subscribe_topic calls
services/director-v2/src/simcore_service_director_v2/modules/comp_scheduler/_scheduler_dask.py Adapted task progress change handler for tuple events
services/dask-sidecar/tests/unit/test_worker.py Refactored tests to use subscribe_topic instead of Pub/Sub mocks
services/dask-sidecar/tests/unit/test_utils_dask.py Updated test utilities to align with the new event-based progress reporting
services/dask-sidecar/tests/unit/conftest.py Updated environment variables for logging level
services/dask-sidecar/src/simcore_service_dask_sidecar/utils/dask.py Modified publish_event to call worker.log_event directly

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@sanderegg sanderegg merged commit 17a8abf into ITISFoundation:master May 15, 2025
93 of 95 checks passed
@sanderegg sanderegg deleted the dask-sidecar/remove-deprecated-pub-sub branch May 15, 2025 15:03
@sanderegg sanderegg changed the title ♻️Dask-sidecar: remove dask Pub/Sub ♻️Dask-sidecar: remove dask Pub/Sub (🚨🚨🚨 computational services must be switched off prior to deploy) May 19, 2025
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jun 6, 2025
92 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:dask-service Any of the dask services: dask-scheduler/sidecar or worker a:director-v2 issue related with the director-v2 service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants