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

call_api task and update sarscov2_illumina_full with task #206

Merged
merged 20 commits into from
Feb 8, 2021
Merged

Conversation

schaluva
Copy link
Contributor

@schaluva schaluva commented Feb 5, 2021

created task to call apis to make data tables
added api task into full illumina workflow -- assemblies and reads tables created as last task in full workflow

Copy link
Member

@dpark01 dpark01 left a comment

Choose a reason for hiding this comment

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

Thanks @schaluva !

task looks good and thanks for adding both a standalone workflow as well as adding on to the full one.

Does it really need 10GB RAM? Seems unusually high.

Main thing I’d like to request is to alter the sarscov2_illumina_full workflow to make the Terra project and workspace name inputs optional inputs, and only call the new task if both inputs are defined. The new task is very Terra-centric and is our only non-portable WDL in this repository so far. That’s fine for the standalone new workflow: no one would ever want to run it outside of Terra (though maybe the new workflow should have “terra” somewhere in its name so users perusing dockstore don’t try to run it elsewhere). But the illumina full workflow should certainly remain cross platform if possible, and simply making the two new inputs optional should do the trick.

@dpark01
Copy link
Member

dpark01 commented Feb 8, 2021

Oh also, can you merge in the latest master branch so this is mergable?

Copy link
Member

@dpark01 dpark01 left a comment

Choose a reason for hiding this comment

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

Thanks!

Later on when you get a chance, lets move that docker somewhere more institutional (e.g. broadinstitute) but lets merge this for now once the tests pass

@dpark01 dpark01 merged commit b040558 into master Feb 8, 2021
@dpark01 dpark01 deleted the api_wdl branch February 8, 2021 18:59
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.

2 participants