-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add option to output a new config file #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some suggestions
omics/cli/run_analyzer/__main__.py
Outdated
@@ -24,6 +25,7 @@ | |||
-o, --out=<path> Write output to file | |||
-P, --plot=<directory> Plot a run timeline to a directory | |||
-H, --headroom=<float> Adds a fractional buffer to the size of recommended memory and CPU. Values must be between 0.0 and 1.0. | |||
-c, --config=<path> Output a config file with recommended resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specify that it is a Nextflow style config file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or Nextflow / CWL
omics/cli/run_analyzer/__main__.py
Outdated
""") | ||
out.write(task_string) | ||
elif engine == 'WDL': | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raise an error saying we don't support WDL
omics/cli/run_analyzer/__main__.py
Outdated
wfid = res['workflow'].split('/')[-1] | ||
engine = omics.get_workflow(id=wfid)['engine'] | ||
if res['type'] == 'task': | ||
task_name = res['name'].split(" ")[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this always work? Do we know that our engines can't produce a task name like "[my workflow task]" with spaces?
It might make this more future proof to have a constant for the task name split string because if we need to change it due to new engines, this line of code doesn't advertise it's intent. Alternatively, perhaps have a function to split the name, perhaps with an engine name as an argument if different engines do different things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add a function that can parse the base task name and enforce the format of it.
Updates instructions for building from source
caveats on price estimation
@markjschreiber I made this Nextflow only for now. I will add in CWL and WDL once I have spent more time working out how to handle scattered tasks for those. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Before merging, can you update the README with information about the --config
option.
Other suggestions for improvements:
docopt
has a nice mechanism for separating mutually exclusive tags (see my most recent PR and docopt documentation). If --config
shouldn't be logically used with some other options then you can include a new usage line such as omics-run-analyzer <runId> --config=<path>
. This can also simplify the logic for the main method as conflicting options can be rejected by the arg parser before they get to the main logic.
--write-config
might be a better long name? --config
might suggest you can read a config file to change the behavior of the run analyzer. Not sure?
Can you see if you can split the config logic into a separate python file. I have been trying to do this for new features to declutter the __main__.py
which has gotten large and confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Issue #, if available:
Description of changes:
DRAFT: Need to add CWL and WDL formatting still.
Adds
--config=<path>
which if specified will create a configuration file formatted correctly for the workflow's engine. The below for example would be correctly formatted for NF. This would enable users to directly utilize recommended settings from the runanalyzer.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.