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

Document required cores argument for snakemake and nextstrain build #207

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

huddlej
Copy link
Contributor

@huddlej huddlej commented Aug 7, 2020

Adds the required --cores 1 argument to all calls to snakemake and nextstrain build in documentation. This is the first step toward removing Snakemake as an augur dependency.

Adds the required --cores 1 argument to all calls to `snakemake` and `nextstrain
build` in documentation. This is the first step toward removing Snakemake as an
augur dependency [1].

[1] nextstrain/augur#557
@tsibley tsibley temporarily deployed to nextstrain-s-document-s-roitil August 7, 2020 22:06 Inactive
@tsibley
Copy link
Member

tsibley commented Aug 7, 2020

Why limit to a single core with --cores 1 instead of defaulting to all cores with --cores?

We might want to alternatively advise nextstrain build --cpus 1 zika-tutorial in the CLI examples. This will automatically pass --cores 1 to Snakemake, plus it has the benefit of working across Nextstrain CLI runners. Currently this correctly hints both Docker and AWS Batch. It also leaves open the future ability for nextstrain build to automatically hint other workflow engines than Snakemake (like Nextflow, perhaps). It seems like it might be worth it to try to setup future-proof habits in our doc? Thoughts?

Separately, I've also thought about extending Nextstrain CLI to automatically provide a --cores option to Snakemake if none is specified, thus insulating from this (IMO) mis-step by Snakemake to require it. I'd love your input on that.

@emmahodcroft
Copy link
Member

Does all cores literally mean all cores? That might be a shock for users running locally, if it totally takes over their computer. Or will it limit so that it doesn't completely overtake the cores?
If it does use all cores, we could maybe just add more explaining that one can specify core number to suit their machine.

@huddlej
Copy link
Contributor Author

huddlej commented Aug 11, 2020

Why limit to a single core with --cores 1 instead of defaulting to all cores with --cores?

(Cross-posting this comment from the related augur PR.) One core is a safer default for most environments (especially a shared environment like a rhino node on the Hutch cluster) and I prefer to have an explicit integer argument that hints to new users that there is a value they can change as they wish. I'm also not sure how --cores without any argument interacts with --cluster and I don't want to find out the hard way. :)

I've also thought about extending Nextstrain CLI to automatically provide a --cores option to Snakemake if none is specified, thus insulating from this (IMO) mis-step by Snakemake to require it.

I would love this. At first glance, I thought this was what the CLI was already doing. I would assume that defining a default value for the cpus argument would essentially accomplish what you're describing here?

I find Snakemake's requirement of --cores really frustrating from a usability perspective (especially since they allow users to not specify how many cores they want, which seems to defeat the purpose of requiring an argument). It would be great if the CLI had reasonable/safe defaults (e.g., cores=1) that hide Snakemake's interface and also allow us to switch easily between other workflow tools.

We might want to alternatively advise nextstrain build --cpus 1 zika-tutorial in the CLI examples.

This seems like a good direction. For this PR, I want to focus on minimal steps to getting rid of Snakemake as an augur dependency though. Nevermind, I see what you're saying now. I'll update this PR to use the --cpus argument.

Using the `--cpus` argument should make our documentation more compatible with
future support for other workflow tools.
@tsibley tsibley temporarily deployed to nextstrain-s-document-s-roitil August 11, 2020 18:03 Inactive
@huddlej huddlej merged commit a82956d into master Aug 11, 2020
@huddlej huddlej deleted the document-snakemake-cores branch August 11, 2020 18:42
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.

3 participants