feat(core): add workflow execute subcommand #2273

vigsterkr · 2021-08-18T05:40:53Z

Description

Fixes #2130

this PR only includes the following features:

add subcommand workflow execute based on the specs
use pluggy to implement a plugin based workflow provider/executor framework
create a default cwltool based workflow provider
fix cwl exporter to use parameter's actual_value property instead of default_value this way user-defined parameter overrides will be reflected in the exported cwl file

renku/core/commands/workflow.py

renku/core/management/workflow/providers/cwltool_provider.py

renku/core/models/workflow/provider.py

renku/core/plugins/provider.py

tests/cli/test_workflow.py

renku/core/management/workflow/value_resolution.py

renku/core/commands/workflow.py

renku/core/management/workflow/value_resolution.py

vigsterkr · 2021-09-03T06:01:20Z

@Panaetius ready for (re)review. thanks!

Panaetius · 2021-09-03T07:31:24Z

You should enjoy your vacation 😉

Don't worry, we can take care of this PR while you're gone. And thanks for the changes

vigsterkr · 2021-09-21T06:33:30Z

renku/core/commands/update.py

@@ -143,7 +146,9 @@ def execute_workflow(

    started_at_time = local_now()

-    modified_outputs = _execute_workflow_helper(plans=plans, client=client)
+    # NOTE: Create a ``CompositePlan`` because ``workflow_covert`` expects it
+    workflow = CompositePlan(id=CompositePlan.generate_id(), plans=plans, name=f"plan-collection-{uuid.uuid4().hex}")


i dont like this as there's a CompositePlan -> List[Plan] -> CompositePlan cycle going on here. but for the sake of having the execute feature in master i'd merge this and the toil PR in the meanwhile i'll finish the refactor of handling list of plans in the workflow execute/converter plugins

Me and Mohammad discussed this in the past, and it is unfortunate but a bit of a necessary feature to allow the same executor to be used with workflow execute, rerun and update. In the rerun and update case, there is only a list of plans as the source, not a CompositePlan, and we didn't want to modify execute_workflow as you're still working on that part.

But we should consider not building a CompositePlan in the execute_workflow method as it's not really needed.

m-alisafaee

Thanks Viktor! This looks very good. I've made a few comments; feel free to address them in a coming refactoring PR if you are doing one atm.

renku/core/models/workflow/provider.py

renku/core/management/workflow/value_resolution.py

m-alisafaee · 2021-09-21T12:03:47Z

renku/core/commands/workflow.py

+
+        return jsonld.read_yaml(file)
+    except Exception as e:
+        raise errors.ParameterError(e)


It's probably good to write a message here to say that something was wrong with the values file.

renku/core/commands/workflow.py

m-alisafaee · 2021-09-21T12:06:25Z

renku/cli/workflow.py

+    "-s",
+    "--set",
+    multiple=True,
+    metavar="<parameter>=<value>",


How does this work with CompositePlanValueResolver? In there we look for a parameters or a steps keys. It's also probably good to add a test which uses this flag.

make sure that cwl export uses the actual_value of a parameter

refactor workflow_execute plugin interface

Co-authored-by: Ralf Grubenmann <ralf.grubenmann@sdsc.ethz.ch>

transform exception from jsonld.read_yaml to RenkuException

this class does a values resolution for a given AbstractPlan using user supplied values as well as holds information regarding none effective settings.

Co-authored-by: Ralf Grubenmann <ralf.grubenmann@sdsc.ethz.ch>

…eResolver

add documentation for cwltool provider implementation

m-alisafaee

Thank you!

github-actions bot added the documentation:pending label Aug 18, 2021

vigsterkr force-pushed the 2130-workflow-execute branch 2 times, most recently from da4f321 to b21264c Compare August 22, 2021 15:51

vigsterkr mentioned this pull request Aug 23, 2021

feat(core): use toil-cwl-runner for running workflows #2263

Closed

vigsterkr force-pushed the 2130-workflow-execute branch from 82739c3 to 36bdb1d Compare August 23, 2021 13:38

vigsterkr removed the documentation:pending label Aug 23, 2021

vigsterkr marked this pull request as ready for review August 23, 2021 13:38

vigsterkr requested a review from a team as a code owner August 23, 2021 13:38

vigsterkr force-pushed the 2130-workflow-execute branch 2 times, most recently from 4db047d to 9eef65c Compare August 23, 2021 16:16

Panaetius requested changes Aug 27, 2021

View reviewed changes

vigsterkr commented Aug 27, 2021

View reviewed changes

tests/cli/test_workflow.py Show resolved Hide resolved

vigsterkr commented Aug 27, 2021

View reviewed changes

tests/cli/test_workflow.py Outdated Show resolved Hide resolved

vigsterkr force-pushed the 2130-workflow-execute branch 2 times, most recently from f5f4b06 to 0b63e15 Compare August 30, 2021 13:18

vigsterkr commented Aug 30, 2021

View reviewed changes

renku/core/management/workflow/value_resolution.py Outdated Show resolved Hide resolved

vigsterkr commented Aug 30, 2021

View reviewed changes

renku/core/commands/workflow.py Show resolved Hide resolved

Panaetius requested changes Aug 30, 2021

View reviewed changes

vigsterkr force-pushed the 2130-workflow-execute branch 2 times, most recently from dcc6570 to 0b46f54 Compare September 2, 2021 21:52

m-alisafaee mentioned this pull request Sep 10, 2021

Allow changing parameters/inputs/outputs in the rerun command #2323

Closed

vigsterkr force-pushed the 2130-workflow-execute branch from 34d6f02 to d2d7feb Compare September 13, 2021 07:01

vigsterkr force-pushed the 2130-workflow-execute branch 2 times, most recently from 64d7714 to c4e7d92 Compare September 21, 2021 06:25

vigsterkr commented Sep 21, 2021

View reviewed changes

vigsterkr force-pushed the 2130-workflow-execute branch from c4e7d92 to b453f3c Compare September 21, 2021 06:35

m-alisafaee reviewed Sep 21, 2021

View reviewed changes

add workflow execute support

c44bbf8

Viktor Gal and others added 14 commits September 22, 2021 15:28

fix execute --set command line arg

ddbcf9d

make sure that cwl export uses the actual_value of a parameter

fix compositeplan exporting and workflow_graph generation

e26776b

refactor workflow_execute plugin interface

Apply suggestions from code review

fcf915e

Co-authored-by: Ralf Grubenmann <ralf.grubenmann@sdsc.ethz.ch>

safe-guard yaml parser

4461126

transform exception from jsonld.read_yaml to RenkuException

parameterize test_workflow_execute_command to cover more cases

efdf885

fix docstring for IWorkflowProvider interface

a36020d

Add RunValues class for value resolution

6b34f77

this class does a values resolution for a given AbstractPlan using user supplied values as well as holds information regarding none effective settings.

Apply suggestions from code review

716000e

Co-authored-by: Ralf Grubenmann <ralf.grubenmann@sdsc.ethz.ch>

add communicator to export and execute commands

30d0ae2

refactor RunValues class into PlanValueResolver and CompositePlanValu…

1af0071

…eResolver

remove encodingFormat for output

7726a39

use the specific ValueResolver when the type of workflow is known

a0986da

add documentation for cwltool provider implementation

minor refactor _execute_workflow command to align with update and rerun

74aceb1

address review comments

846a7de

vigsterkr force-pushed the 2130-workflow-execute branch from b453f3c to 846a7de Compare September 22, 2021 13:29

vigsterkr enabled auto-merge (squash) September 22, 2021 17:28

Merge branch 'master' into 2130-workflow-execute

03a8608

m-alisafaee self-requested a review September 23, 2021 10:43

m-alisafaee approved these changes Sep 23, 2021

View reviewed changes

Panaetius approved these changes Sep 23, 2021

View reviewed changes

vigsterkr merged commit 34297be into master Sep 23, 2021

vigsterkr deleted the 2130-workflow-execute branch September 23, 2021 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): add workflow execute subcommand #2273

feat(core): add workflow execute subcommand #2273

vigsterkr commented Aug 18, 2021 •

edited

vigsterkr commented Sep 3, 2021

Panaetius commented Sep 3, 2021

vigsterkr Sep 21, 2021

Panaetius Sep 21, 2021

m-alisafaee left a comment

m-alisafaee Sep 21, 2021

m-alisafaee Sep 21, 2021

m-alisafaee left a comment

feat(core): add workflow execute subcommand #2273

feat(core): add workflow execute subcommand #2273

Conversation

vigsterkr commented Aug 18, 2021 • edited

Description

vigsterkr commented Sep 3, 2021

Panaetius commented Sep 3, 2021

vigsterkr Sep 21, 2021

Choose a reason for hiding this comment

Panaetius Sep 21, 2021

Choose a reason for hiding this comment

m-alisafaee left a comment

Choose a reason for hiding this comment

m-alisafaee Sep 21, 2021

Choose a reason for hiding this comment

m-alisafaee Sep 21, 2021

Choose a reason for hiding this comment

m-alisafaee left a comment

Choose a reason for hiding this comment

vigsterkr commented Aug 18, 2021 •

edited