search

broadinstitute / cromwell-tools

A collection of Python clients and accessory scripts for interacting with the Cromwell
https://cromwell-tools.readthedocs.io
BSD 3-Clause "New" or "Revised" License
22 stars 9 forks source link

Enable Cromwell-tools to add labels at submission time #9

Closed rexwangcc closed 6 years ago

rexwangcc commented 6 years ago

This PR enables Cromwell-tools to add the label(s) at submission time.

Besides, it implements a Cromwell label validator to check the validity of the label(s) before hitting the Cromwell endpoint.

The implementation is based on the Cromwell doc: https://cromwell.readthedocs.io/en/develop/Labels/ and the scala code base of Cromwell: https://github.com/broadinstitute/cromwell/blob/develop/core/src/main/scala/cromwell/core/labels/Label.scala

ambrosejcarr commented 6 years ago

@rexwangcc Why did you end up removing the label validator? I liked this option.

ambrosejcarr commented 6 years ago

From the conversation during stand-up, it sounds like we might be able to keep the tool around somewhere that is outside of the Lira execution path. This would make me happy. :)

rexwangcc commented 6 years ago

Per an offline discussion with @dshiga , since currently Lira is the main consumer of Cromwell-tools, it is reasonable to remove the label validator for simplicity, especially when updating the version of Cromwell-tools will end up with redeploying Lira, this could reduce the maintenance pain of us. Besides, I think https://github.com/ambrosejcarr/cromwell-manager should be a better place to move the code to, will make a PR for it.

rexwangcc commented 6 years ago

Per another discussion during standup, I just realized that https://github.com/ambrosejcarr/cromwell-manager will eventually get merged into this cromwell-tools. If that is the case, I would strongly recommend that adding back the code here, from the perspective of the completeness of a "Cromwell Python API"/development task. I will make the label validator optional for start_workflow, so that the changes won't affect Lira that much. @ambrosejcarr

ambrosejcarr commented 6 years ago

That solution sounds optimal for my use cases. @dshiga does this satisfy your desire for simplicity, or do we need further iteration?

dshiga commented 6 years ago

That works for me!

ambrosejcarr commented 6 years ago

Still looks good. Thanks for adding the doc strings :)

rexwangcc commented 6 years ago

Thanks for all the comments and discussions, going to merge this PR now.