Skip to content
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

feat(workflow): support for workflow definition files #3176

Merged
merged 5 commits into from Dec 19, 2022

Conversation

m-alisafaee
Copy link
Contributor

@m-alisafaee m-alisafaee commented Oct 18, 2022

Description

Workflow definition file pitch: renku run workflow-file.yml and renku workflow show workflow-file.yml.
Every existing file that has a .yml or .yaml extension and is not executable is considered to be a workflow file. Also, valid YAML files that contain a dictionary are considered to be a workflow file. We can also force running a workflow file by passing the --file flag.

Some Decisions

  • Plan names for workflow file steps are prefixed with the workflow file name (e.g. workflow-file.pre-processing). It is called qualified name. This is done to make sure that workflow file steps will have a unique name which is required for the Renku workflow machinery to function correctly.
  • Workflow files are added as a dependency to every plan that they contain. This is done to make sure that renku status works as intended. renku status shows if a workflow file has changed and its generation. Note that renku update doesn't re-run workflow files. Users need to run the workflow file to bring its generations up to date.
  • When running a selected subset of steps, we don't create a new version of the WorkflowFileCompositePlan; We stored the plan with all steps. We can later find out if a subset of steps were executed by looking at the number of activities that are stored in the WorkflowFileActivityCollection.

Demo Project

A demo project is available at https://gitlab.dev.renku.ch/mohammad.alisafaee/workflow-file-demo.git.

@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 4 times, most recently from 1b17950 to 423aa0c Compare October 23, 2022 00:10
@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 9 times, most recently from dd1a34e to 0074aef Compare November 7, 2022 21:33
@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 3 times, most recently from bc8c416 to 85264a6 Compare November 8, 2022 17:57
@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 2 times, most recently from c7522db to 79daadd Compare November 15, 2022 23:51
@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 5 times, most recently from a43d41e to 4ef78fe Compare November 30, 2022 16:37
@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 3 times, most recently from 51e6d1d to 2e11444 Compare December 4, 2022 21:40
@m-alisafaee m-alisafaee marked this pull request as ready for review December 5, 2022 08:28
@m-alisafaee m-alisafaee requested a review from a team as a code owner December 5, 2022 08:28
renku/command/schema/workflow_file.py Outdated Show resolved Hide resolved
renku/command/schema/plan.py Outdated Show resolved Hide resolved
renku/core/errors.py Outdated Show resolved Hide resolved
renku/core/plugin/workflow_file_parser.py Outdated Show resolved Hide resolved
@@ -234,7 +241,7 @@ def hash_file(path: Union[Path, str], hash_type: str = "sha256") -> Optional[str
return hash_file_descriptor(f, hash_type)


def hash_str(content: str, hash_type: str = "sha256") -> str:
def hash_string(content: str, hash_type: str = "md5") -> str:
Copy link
Member

Choose a reason for hiding this comment

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

This isn't actually used anywhere? The function above it uses sha256 as well, and the docstring of this function still says sha256.

Was there a reason to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this function since we have it in the git history.

md5 is a bit faster than sha256 and since this is used just for hashing (and not cryptography) and we might use it a lot, I've replaced the hash function.

Copy link
Member

Choose a reason for hiding this comment

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

but wouldn't that cause issues with existing hashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I've used md5 for new hash functions and this is the only old one that I've replaced. It's not used anywhere so no harm done.

renku/core/workflow/converters/renku.py Outdated Show resolved Hide resolved
renku/core/workflow/converters/renku.py Show resolved Hide resolved
renku/core/workflow/parser/renku.py Outdated Show resolved Hide resolved
renku/core/workflow/parser/renku.py Show resolved Hide resolved
renku/core/workflow/parser/renku.py Show resolved Hide resolved
renku/core/workflow/plan.py Outdated Show resolved Hide resolved
renku/core/workflow/providers/local.py Outdated Show resolved Hide resolved
renku/core/workflow/workflow_file.py Outdated Show resolved Hide resolved
renku/core/workflow/workflow_file.py Outdated Show resolved Hide resolved
renku/core/workflow/workflow_file.py Outdated Show resolved Hide resolved
renku/ui/cli/run.py Show resolved Hide resolved
renku/ui/cli/run.py Show resolved Hide resolved
renku/ui/cli/utils/terminal.py Outdated Show resolved Hide resolved
tests/cli/test_rerun.py Outdated Show resolved Hide resolved
tests/cli/test_rerun.py Show resolved Hide resolved
@m-alisafaee m-alisafaee marked this pull request as draft December 9, 2022 02:29
@m-alisafaee m-alisafaee marked this pull request as ready for review December 14, 2022 09:48
Copy link
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all the changes, looks great now! Only two minor suggestions plus the one mistake I made that you found 🙈

renku/command/view_model/plan.py Outdated Show resolved Hide resolved
renku/core/workflow/activity.py Outdated Show resolved Hide resolved
renku/core/workflow/activity.py Outdated Show resolved Hide resolved
renku/core/workflow/parser/renku.py Outdated Show resolved Hide resolved
renku/core/workflow/parser/renku.py Outdated Show resolved Hide resolved
@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 2 times, most recently from 92341b1 to e6f572f Compare December 16, 2022 16:17
@m-alisafaee m-alisafaee force-pushed the pitch/workflow-definition-file branch 2 times, most recently from bafb439 to fbe852a Compare December 16, 2022 16:57
@m-alisafaee m-alisafaee merged commit b7b2395 into develop Dec 19, 2022
@m-alisafaee m-alisafaee deleted the pitch/workflow-definition-file branch December 19, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants