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

AWSBatch Generic Configuration Profile #42

Merged
merged 9 commits into from Aug 2, 2018
Merged

AWSBatch Generic Configuration Profile #42

merged 9 commits into from Aug 2, 2018

Conversation

apeltzer
Copy link
Member

@apeltzer apeltzer commented Aug 2, 2018

This adds a generic AWSBatch configuration profile that allows the users to specify required parameters

  • Job Queue
  • AWS Region

Additionally, I implemented a small check next to the UPPMAX sanity checking to check for the existence of these mandatory parameters when running on the "awsbatch" profile, so users get a warning to set these params on AWSBatch properly.

Let me know if that makes sense - happy to polish this further if someone has suggestions!

@apeltzer apeltzer requested a review from a team August 2, 2018 13:29
fixOwnership = true
runOptions = "-u \$(id -u):\$(id -g)"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Docker options are ignored by he AWS Batch executor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @pditommaso ! I adjusted that :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also remove docker.enabled :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

main.nf Outdated
@@ -58,11 +58,16 @@ def helpMessage() {

Other options:
--outdir The output directory where the results will be saved
--workDir The temporary directory where intermediate data will be saved
Copy link
Member

Choose a reason for hiding this comment

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

Nextflow already has a core function for this:

    -w, -work-dir
       Directory where intermediate result files are stored

Copy link
Member Author

Choose a reason for hiding this comment

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

Took care of using that one in 70d4bb0

main.nf Outdated
@@ -166,6 +171,11 @@ if( workflow.profile == 'uppmax' || workflow.profile == 'uppmax-modules' || work
if ( !params.project ) exit 1, "No UPPMAX project ID found! Use --project"
}

//AWSBatch sanity checking
if(workflow.profile == 'awsbatch'){
if (!params.awsqueue || !params.awsregion || !params.workDir.startsWith('s3') || !params.outdir-startsWith('s3')) exit 1, "Specify AWS Batch required parameters!"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe split this up a bit with different error messages - I could imagine getting super frustrated if I did specify all parameters but used a http address for the outdir or something ;)

Also, minor typo: hyphen instead of a dot: !params.outdir-startsWith('s3')

Copy link
Member Author

Choose a reason for hiding this comment

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

Found the typo - already fixed, but I can break it up yes!

@ewels
Copy link
Member

ewels commented Aug 2, 2018

Really nice stuff 😁 🎉

}

batch {
workDir = params.workDir
Copy link
Member

Choose a reason for hiding this comment

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

workflow.workDir

*/

docker {
enabled = true
Copy link
Member

@ewels ewels Aug 2, 2018

Choose a reason for hiding this comment

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

@pditommaso suggested we remove this 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Done :-)

@ewels
Copy link
Member

ewels commented Aug 2, 2018

Looking great! Now it just needs some documentation 😉

Fine if you make an issue for that and do it separately though.

@ewels ewels added this to Tasks for all pipelines in hackathon-scilifelab-2018 Aug 2, 2018
@apeltzer
Copy link
Member Author

apeltzer commented Aug 2, 2018

I added some notes on the command lines at least - will publish a proper blog post on detailed steps to get AWSBatch running (including all steps from zero - running) in the next days with my master student. Plus, I guess we can document the general approach in cookiecutter and also make sure that all pipelines benefit from that documentation?

@apeltzer
Copy link
Member Author

apeltzer commented Aug 2, 2018

Will furthermore add a PR to cookiecutter later today...

@ewels ewels merged commit ed695d2 into nf-core:master Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
hackathon-scilifelab-2018
  
Tasks for all pipelines
Development

Successfully merging this pull request may close these issues.

None yet

3 participants