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

global: allow operational options in reana.yaml #400

Merged
merged 5 commits into from
Apr 8, 2020

Conversation

mvidalgarcia
Copy link
Member

@mvidalgarcia mvidalgarcia commented Mar 23, 2020

closes #356

reana.yaml could be like:

version: 0.6.0
inputs:
  options:
    toplevel: './example'
workflow:
  type: yadage
  file: workflow.yaml
[...]
TODO:
  • Validation of reana.yaml operational options on creation.
  • Execute a CWL workflow with -o TARGET ... successfully.

@mvidalgarcia mvidalgarcia marked this pull request as ready for review March 23, 2020 15:50
reana_client/utils.py Outdated Show resolved Hide resolved
@diegodelemos
Copy link
Member

Needs rebase ⏭

@mvidalgarcia mvidalgarcia force-pushed the 356-toplevel-configurable branch from 698fe43 to d4604b4 Compare March 25, 2020 15:46
@mvidalgarcia mvidalgarcia changed the title global: allow passing options to workflow loader global: allow operational options in reana.yaml Mar 31, 2020
@mvidalgarcia mvidalgarcia force-pushed the 356-toplevel-configurable branch 3 times, most recently from 3a5d2a4 to 3c1bb65 Compare April 2, 2020 17:16
reana_client/cli/utils.py Outdated Show resolved Hide resolved
@@ -38,7 +40,7 @@ def workflow_uuid_or_name(ctx, param, value):
return value


def yadage_load(workflow_file, toplevel='.'):
def yadage_load(workflow_file, toplevel='.', **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Who is consuming these **kwargs? We already unpack toplevel which is the only one we are interested in.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we set other inputs.options such as initdir, its value will get here and this function would fail. With **kwargs we can filter out these options that are not needed on validation time. I agree it's a bit hacky but it's cleanest and safest way I could come up with. If you have any other idea please let me know 🙂


except ValidationError as e:
e.message = str(e)
raise e


def cwl_load(workflow_file):
def cwl_load(workflow_file, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Also here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same here. For CWL workflows, when passing TARGET: foo for example, it will end up here.

@mvidalgarcia mvidalgarcia force-pushed the 356-toplevel-configurable branch 2 times, most recently from 17ac56c to 25125d9 Compare April 3, 2020 16:29
@mvidalgarcia mvidalgarcia force-pushed the 356-toplevel-configurable branch 5 times, most recently from c27c364 to b7c546c Compare April 7, 2020 15:12
In favor of `check_output` which is compatible with Python 2.7
@mvidalgarcia mvidalgarcia force-pushed the 356-toplevel-configurable branch from 773a770 to e2eb481 Compare April 7, 2020 17:39
Copy link
Member

@tiborsimko tiborsimko left a comment

Choose a reason for hiding this comment

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

Works well 👍

WRT second item (run CWL with target), it does not seem to work for our demo workflows in a pure cwltool environment... So we need first to investigate that. Hence this is good to go 🚀

@tiborsimko tiborsimko merged commit e2eb481 into reanahub:master Apr 8, 2020
@mvidalgarcia mvidalgarcia deleted the 356-toplevel-configurable branch April 8, 2020 12:28
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.

yadage: make toplevel configurable
3 participants