feat!: replace all bbtool wrappers with one bbtool wrapper for all subcommands

[SilasK]

Description

I add a generic bbtool wrapper.
It is based on #2

BBtools hase many bash shcripts. This wrapper should allow to run them all.

QC

• I confirm that:

For all wrappers added by this PR,

• there is a test case which covers any introduced changes,
• input: and output: file paths in the resulting rule can be changed arbitrarily,
• either the wrapper can only use a single core, or the example rule contains a threads: x statement with x being a reasonable default,
• rule names in the test case are in snake_case and somehow tell what the rule is about or match the tools purpose or name (e.g., map_reads for a step that maps reads),
• all environment.yaml specifications follow the respective best practices,
• wherever possible, command line arguments are inferred and set automatically (e.g. based on file extensions in input: or output:),
• all fields of the example rules in the Snakefiles and their entries are explained via comments (input:/output:/params: etc.),
• stderr and/or stdout are logged correctly (log:), depending on the wrapped tool,
• temporary files are either written to a unique hidden folder in the working directory, or (better) stored where the Python function tempfile.gettempdir() points to (see here; this also means that using any Python tempfile default behavior works),
• the meta.yaml contains a link to the documentation of the respective tool or command,
• Snakefiles pass the linting (snakemake --lint),
• Snakefiles are formatted with snakefmt,
• Python wrapper scripts are formatted with black.
• Conda environments use a minimal amount of channels, in recommended ordering. E.g. for bioconda, use (conda-forge, bioconda, nodefaults, as conda-forge should have highest priority and defaults channels are usually not needed because most packages are in conda-forge nowadays).

[SilasK]

I now extract the mem using a regex.

I also made a major update to the Snakefile test. It now represents a good usage of the wrapper and the bbtools functions.
e.g. previously all parameters were input as a comma-separated string.
With the same rules, it works for paired or single-end reads.

What do you think of my updated workflow?
@fgvieira @dlaehnemann

[fgvieira]

I think it looks good but, since it will introduce breaking changes, not sure if @johanneskoester want to take a look first.

[dlaehnemann]

This generally looks good to me. But I haven't looked at all the wrapper.py code in detail -- @johanneskoester has requested changes anyway, and will thus have to have another look. I'll ping him...

[SilasK]

Now we are almost done!

I had two problems with the linter:

1. It didn't want me to have functions in the main Snakemake, therefore the import. Even if I think it's less ideal.

2. It interpreted a ktrim="r" as a path, and didn't accept it. I found a solution, which is less optimal.

[SilasK]

In the wrapper I put the links to other PRs in snakemake that will incorporate these functions. But I guess we should not wait until then to release the next version of this wrapper.

Do you have any final feedback @fgvieira and @dlaehnemann
Thank you in advance.

[dlaehnemann]

Now we are almost done!
Yes, definitely!

I had two problems with the linter:
1. It didn't want me to have functions in the main Snakemake, therefore the import. Even if I think it's less ideal.
Then I guess we'll leave it as is. Maybe add a comment as suggested, if you think this is useful.

2. It interpreted a `ktrim="r"` as a path, and didn't accept it. I found a solution, which is less optimal.

Is that under params? Because I would find that weird, and possibly a linter bug. But if it enforces this for input and output, this makes sense. Then, I would suggest simply specifying this as a param. And if that's what you did, did you try something like either of the following, to make it work with the current linter:

params:
    ktrim="'r'",
    ktrim="'r' ",
    ktrim="r ",

As for the links to PRs, I think it is perfectly fine to simply leave those as TODOs. Maybe make it a bit clearer still, that once these are merged, the respective functions can simply go away and use snakemake internal functionality.

[SilasK]

It looks like a linter bug:
Code
Error

[dlaehnemann]

Ah, thanks for the pointers to the linter. Regarding the (input) function, you could indeed make this an inline lambda expression like this:

rule loglog:
    input:
        expand(
            "reads/raw/{{sample}}_{fraction}.fastq.gz",
            fraction=lambda wc: ["R1", "R2"] if config.get("reads_are_paired", True) else ["se"]
        ),

It has the advantage of being right there in the input of the rule. It has the disadvantage of being slightly less readable than the actual function with newlines. And you'll have to repeat that code wherever you provide such a read fraction specification. It seems like there's no perfect solution with the current linter.

And from what I understand about the ktrim="r", this recognizes that r is a prefix of the input/output path, which starts with reads/.... That's a very short prefix, but it is correct. The linter could probably be amended to ignore such single letter cases or make it more of a suggestion, then a must (not sure if that exists, though). And I think you should get around this by including some random whitespace at the end, so:
ktrim="r "
Then, it shouldn't be a prefix any more...

[SilasK]

The whitespace fixed the linter.

I find the lambda version less clear.

[dlaehnemann]

Agreed, I'll merge! 🎉

[SilasK]

Thank you very much for your review and your suggestions!

[dlaehnemann]

Thanks for keeping at it! Getting things right for reproducible and reusable data analyses can be exhausting, especially when you're battling with multiple levels of the workflow / analysis stack and several workflows at the same time.

So take a moment and celebrate! 🚀

[fgvieira]

Nice work!!