Skip to content

Conversation

@sanderegg
Copy link
Member

@sanderegg sanderegg added this to the Engage milestone Jul 29, 2025
@sanderegg sanderegg self-assigned this Jul 29, 2025
@sanderegg sanderegg requested review from GitHK and pcrespov as code owners July 29, 2025 11:56
@sanderegg sanderegg added the a:director-v2 issue related with the director-v2 service label Jul 29, 2025
@sanderegg sanderegg added the 🤖-automerge marks PR as ready to be merged for Mergify label Jul 29, 2025
@sanderegg
Copy link
Member Author

@mergify queue

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 fixes a bug where missing computational tasks in a pipeline would cause issues when computing pipeline details. The fix adds proper null checking and error handling to gracefully handle scenarios where not all nodes in the pipeline DAG have corresponding computational tasks.

  • Adds null safety checks in pipeline computation functions to handle missing tasks
  • Includes a new test case to verify the fix works for missing tasks
  • Refactors some code for consistency and clarity

Reviewed Changes

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

File Description
services/director-v2/src/simcore_service_director_v2/utils/dags.py Main fix adding null checks for missing tasks in pipeline computation functions
services/director-v2/tests/unit/test_utils_dags.py New test case validating behavior with missing tasks and commented out old assertions
services/director-v2/src/simcore_service_director_v2/api/routes/computations.py Minor cleanup removing unnecessary type annotations
Comments suppressed due to low confidence (1)

services/director-v2/tests/unit/test_utils_dags.py:394

  • The commented out assertions were likely validating important test preconditions. Consider either removing these comments entirely or replacing them with appropriate assertions that account for the new behavior where missing tasks are allowed.
    # assert len(set(dag_adjacency)) == len(node_keys) == len(list_comp_tasks)

@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 28845dd

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!

@sanderegg
Copy link
Member Author

@mergify requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 29, 2025

requeue

☑️ This pull request is already queued

@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Jul 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.67%. Comparing base (ef83d12) to head (1f95a9d).
⚠️ Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (ef83d12) and HEAD (1f95a9d). Click for more details.

HEAD has 31 uploads less than BASE
Flag BASE (ef83d12) HEAD (1f95a9d)
unittests 32 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8173       +/-   ##
===========================================
- Coverage   88.05%   69.67%   -18.38%     
===========================================
  Files        1900      734     -1166     
  Lines       73086    33752    -39334     
  Branches     1280      176     -1104     
===========================================
- Hits        64355    23518    -40837     
- Misses       8351    10176     +1825     
+ Partials      380       58      -322     
Flag Coverage Δ
integrationtests 64.12% <100.00%> (-0.02%) ⬇️
unittests 84.66% <81.81%> (-2.03%) ⬇️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_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.16% <ø> (-7.89%) ⬇️
agent ∅ <ø> (∅)
api_server ∅ <ø> (∅)
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 91.04% <100.00%> (-0.03%) ⬇️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 87.31% <ø> (-2.77%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 59.09% <ø> (-28.97%) ⬇️

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 ef83d12...1f95a9d. 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.

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.

👍

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

👍

@mergify mergify bot merged commit 28845dd into ITISFoundation:master Jul 29, 2025
149 of 154 checks passed
@sanderegg sanderegg deleted the bugfix/8172/check-input-nodes branch July 29, 2025 13:32
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:director-v2 issue related with the director-v2 service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Unhandled exception for invalid key in node_id_to_comp_task[node_id].progress while computing dag

4 participants