Skip to content

Conversation

@sanderegg
Copy link
Member

@sanderegg sanderegg commented Dec 9, 2020

What do these changes do?

allow running partial pipelines as required by the guided mode

Changes:
Director-v2:

  • POST /computations/{project_uuid}:start API now allow to path a subgraph field which is a list of nodes to be run
    • a minimal dependency graph is computed by checking if the dependent nodes have an output. in case they don't they will be run as well (NOTE: currently outdated outputs are not checked so it will not automatically re-run if some input number is changed)
    • a selected node is automatically set as dirty and will re-trigger any node that depend on it
  • GET /computations/{project_uuid} now returns the state of the entire or sub-pipeline depending how it was started

Webserver

  • /computations/{project_uuid}:start API now allow to path a subgraph field that is forwarded to director-v2

NOTE: The frontend will come in a next PR by @odeimaiz

Related issue number

How to test

  • not really testable without hacking in the frontend...

but the idea is:
now the UI just need to say:

  • hey I want to run that node
  • the backend will run this node and before that all the dependent ones if needed (with some restrictions, currently the only check is to see if there is an output or not)

the UI can also say:

  • I want to run this node, this one and this other one
  • the backend will automatically run them with the dependent ones if needed

Checklist

  • Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@sanderegg sanderegg added this to the Alfred_Büchi milestone Dec 9, 2020
@sanderegg sanderegg self-assigned this Dec 9, 2020
@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #2040 (4e93ff2) into master (03ade6c) will increase coverage by 1.4%.
The diff coverage is 77.7%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2040     +/-   ##
========================================
+ Coverage    71.7%   73.2%   +1.4%     
========================================
  Files         413     414      +1     
  Lines       15097   15146     +49     
  Branches     1526    1540     +14     
========================================
+ Hits        10839   11100    +261     
+ Misses       3889    3650    -239     
- Partials      369     396     +27     
Flag Coverage Δ
integrationtests 64.3% <74.1%> (-0.3%) ⬇️
unittests 67.6% <65.5%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...es/service-library/src/servicelib/logging_utils.py 0.0% <0.0%> (ø)
...ices/sidecar/src/simcore_service_sidecar/celery.py 0.0% <0.0%> (ø)
...ar/src/simcore_service_sidecar/celery_log_setup.py 0.0% <0.0%> (-84.7%) ⬇️
...r/src/simcore_service_webserver/computation_api.py 80.0% <ø> (-2.4%) ⬇️
...ervices/sidecar/src/simcore_service_sidecar/cli.py 66.6% <30.0%> (+20.3%) ⬆️
...rvices/sidecar/src/simcore_service_sidecar/core.py 71.4% <66.6%> (+46.6%) ⬆️
...webserver/computation_comp_tasks_listening_task.py 89.0% <66.6%> (-1.0%) ⬇️
...r-v2/src/simcore_service_director_v2/utils/dags.py 62.2% <67.5%> (-7.8%) ⬇️
...core-sdk/src/simcore_sdk/node_ports/filemanager.py 80.4% <100.0%> (+0.1%) ⬆️
...ore_service_director_v2/api/routes/computations.py 88.9% <100.0%> (+0.5%) ⬆️
... and 27 more

@sanderegg sanderegg force-pushed the guided_mode/partial_pipelines branch 6 times, most recently from 0771d58 to 34a8a11 Compare December 14, 2020 14:43
@sanderegg sanderegg changed the title WIP: Guided mode/partial pipelines Guided mode/partial pipelines Dec 14, 2020
@sanderegg sanderegg changed the title Guided mode/partial pipelines Guided mode/partial pipelines (backend only) Dec 14, 2020
@sanderegg sanderegg marked this pull request as ready for review December 14, 2020 15:32
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.

I have one very very late suggestion.

Comment on lines +68 to +70
# FIXME: here we should check if the current inputs are the ones used to generate the current outputs
# this could be done by saving the inputs as metadata together with the outputs (see blockchain)
# we should compare the current inputs with the inputs used for generating the current outputs!!!
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, you'd like to add a check where if the inputs did not change, there is no need to compute the results again?

Copy link
Member Author

Choose a reason for hiding this comment

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

right.

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.

Fine for me.

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.

some minors. very nice work!

else RunningState.NOT_STARTED,
pipeline=nx.to_dict_of_lists(dag_graph),
url=f"{request.url}/{job.project_id}",
stop_url=f"{request.url}/{job.project_id}:stop"
Copy link
Member

Choose a reason for hiding this comment

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

FYI: I know this is not from this PR but for reverse urls lookups, it is safer to use https://www.starlette.io/routing/#reverse-url-lookups

Copy link
Member Author

Choose a reason for hiding this comment

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

looks good. will think about it next time cause I'm not too sure how to use it here.

@sanderegg sanderegg force-pushed the guided_mode/partial_pipelines branch from c799107 to 4e93ff2 Compare December 15, 2020 11:19
@sanderegg sanderegg merged commit 60e8c85 into ITISFoundation:master Dec 15, 2020
@sanderegg sanderegg deleted the guided_mode/partial_pipelines branch December 15, 2020 13:28
@odeimaiz odeimaiz mentioned this pull request Dec 16, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants