Hi all! @Felix Krueger has been working on a rewrite of TrimGalore! recently and reckons he has now gotten to a place where it's equivalent or better than the original, and also substantially faster. #CE8SSJV3N is the biggest nf-core pipeline that uses it (although there are several), does anyone have suggestions about a validation strategy he could follow before releasing this v2 monster? 🦀

is it a 1-1 rewrite?

Identical outputs I believe (to the byte). 1-1 rewrite, albeit with some new bells and whistles

Thanks Phil. Yes, the idea was to have an identical drop in replacement, more or less just noticeable by a version bump. The brach is currently still in beta test phase, and I would value if people could give it test and give feedback. It lives on this branch: https://github.com/FelixKrueger/TrimGalore/tree/optimus_prime, and can be installed with

cargo install trim-galore

The benefit is really multi-layered, starting from ease-of use, general speed up (I managed to trim a 55M paired-end BS-seq dataset down to ~ 40-50 seconds), but is saves ~70% of compute time and this saves a tonne of CO2 along the way. I also used the opportunity to remove a nice outdated option, and added automatic (and tunable) PolyA handling, as well as auto-detection of 2-colour quemistry and (tunable) PolyG handling. From all I can tell it’s blazingly fast, has the exact same Trim Galore feel and scope - but I need some additional testing to be sure. The page https://github.com/FelixKrueger/TrimGalore/blob/optimus_prime/docs/SUMMARY.md has a nice summary of benefits, benchmarks, and more. Feedback welcome!e

I would run both

test

and

test_full

profiles, on a branch with the tools switched out. When I did that for RustQC I did find a number of differences and found a few gremlins Phil hadn't noticed, in silly things like output file naming but also in some real logic differences. I would expect to do a bit of work to resolve those. Fortunately you can have Claude look at both the source implementation and the new one to check any differences you may find.

that sounds great, thanks! I have already found (and fixed) three little things - e.g, going over many SE or PE files in a folder, or Multi-member gzip (GzDecoder → MultiGzDecoder) — many real-world FASTQ files are concatenated gzip streams; GzDecoder stopped at the first member boundary mid-record, causing “Truncated FASTQ” errors which one only sees on a special kind of FastQ files…

OK, I'll have a play (probably tomorrow)

In fact, I would probably benefit of understanding the procedure so I can potentially have a tinker myself. Maybe we could have a joint session? I could do 9:30-11, or 13-16:30 tomorrow (we wouldn’t have to connect for the whole time 😛 )

All I'm really proposing to do is: • Make a container with your new branch active • Make a draft PR to rnaseq switching out the container declaration and applying any other tweaks necessary to make the module run • Compare workflow runs to dev branch reference • Have Claude investigate any unexplained differences You're welcome to watch me work through those, though it's unlikely to be very enthralling!

I talked with Phil about a strategy of something similar to how RustQC does it, upon commit goes ahead to build a container on the github container registry (but not the full release build), and then run nf-test. I would be quite keen to learn and understand to the procedure - would you mind guiding me through it? I wouldn’t expect an initial session to take more than, say 30 mins?

Just copy and paste this file 😅

OK, can do that tonight 😛

oh and if you're pure Rust, which I think you are, you can ask Claude to add in some additional binary builds - such as Windows

I am indeed

Nice! I added a comment / suggestion on the issue

And the tests are running! https://github.com/nf-core/rnaseq/pull/1789

That was always a bit fiddly to manage logic-wise, it would be more consistent to run it outside

but that means it won’t run tests successfully that use

fastqc

15 -            def preset_args_map = ["--fastqc_args": "'-t ${task.cpus}'"]

15 +            def preset_args_map = [:]

Yeah, it complicates things for this specific round of testing.

as in remove FastQC post-trim from entirely, and add in another FastQC step? Note that

--fastqc

still runs FastQC if it is installed locally (and in the PATH), but it is not part of the container as such

Ahh, gotcha, thought you were saying it was removed. Up to you then- you could just tweak your container building fanciness to include it for testing purposes

OK I have now bundled FastQC as well so it doesn’t interfere with the nf-core tests

Great, that will simplify things

Looking good!

yep, 'cutadapt_filtered_reads_plot.txt ' looks like the main diff

added a MultiQC native .json report now.

Need anything from me Felix? Does anything need fixing to keep the plot.txt consistent, or is that changing for good reason?

only seeing this now. Where can I see the plot.txt? But I have added some additional features, namely 2-colour chemistry detection and automatic PolyG removal - if it’s about that then, yes, it’s gonna stay

If you actually go into the CI you'll see the failures and the actual snapshot diffs. e.g. this job:

I am not sure how far off we are with making a full release, but it looks like it would already be a functional replacement and only need cosmetic changes? I suppose adding the

trimming.json

to MultiQC might be super nice…

I'd definitely get a diff to understand what's going on

MultiQC now extracts something from the Trimming report of R1 (which was previously only reported for R2), which leads a change in the MultiQC report. The files are byte identical

Yeah MultiQC is a bit of a hassle here. would be best to remove that cutadapt shim completely and only use the new files, but that'll slow things down a bit as it needs working into multiqc and releasing

The shim works perfectly now, and it already outputs a perfect json for integration whenever you are ready

cough https://github.com/MultiQC/MultiQC/pull/3465

yeah the shim is just ugly. Saying it's cutadapt when it's not

I don’t have a merged button….

Yeah, that was directed at Phil 😄

👀 that’s over 6 weeks!!! 🙂

Yeah people keep distracting me with other projects and stuff 🙄

there are so many fascinating projects out there - all are waiting for you!

Phil has a clone army, it's fine

Yeah people keep distracting me with other projects and stuff 🙄

And you're one of these people, right?

@Jonathan Manning: The latest-everything shards use the dev Docker tag, which was rebuilt when we pushed to optimus_prime. That means shard 7/20 picked up the report fix and the snapshot now matches.

optimus_prime?

that was supposed to be a work in progress name, nothing for public consumption

if that file I flagged is identical we're probably fine from the rnaseq point of view? Ultimately as the tool author you're master of your own destiny Felix and if you're content that everything is behaving correctly, in line with how you wrote the original, it's your call!

It currently runs 99 internal CI tests, and has had lots of validation with identical results. On top of that it works within RNA-seq…. I am quite tempted to move to the next step, and try to be responsive it issues arise?

Sure, do it!