https://nf-co.re logo
Join Slack
Powered by
Can we please update samtools/sort to emit just on...
# team-maintainers
s
Can we please update samtools/sort to emit just one index channel, rather than splitting out bai/csi/crai.
m
didn’t we have this kind of discussion a while back? 😅
s
yes
but no actual consensus was reached
because people were concerned about having bam and cram in the same channel
m
I can live with an 
index
output channel, but I’d like to keep the bam/cram separate
👍 1
s
I'll take that small victory
👍🏻 1
m
I’ll make the change in a separate PR once the version update is merged
m
I feel a hackathon is the perfect moment to merge a PR while no one is looking
#C0853MQJJHW
That is the bury a body moment
s
Started on the samtools index change: https://github.com/nf-core/modules/pull/10614
m
2 birds and all that
s
I think samtools/index is ready: https://github.com/nf-core/modules/pull/10614
(also has the 1.23 bump)
Passed all the tests apart from some no space left on device errors, so ready to go I think. If someone could review that'd be great.
✅ 1
m
We should have done this ages ago
Let do this EVERYWHERE
m
approved, but hesitantly
m
Why hesitantly?
m
Because I’m going to get merge conflicts and a whole mess in my PR 😛
🤣 2
m
Yeah your PR was too big
Sorry
m
I’m also sneaking in some channel changes in my PR, so yeah, it’s gotten a bit out of hand...
🤣 1
m
Ok should we do a bigger big post over the changes in both samtools and multiqc?
m
In hindsight, I regret the change because of the amount of work, but it’ll be worthwhile when it’s done I think
Possibly, explaining the rationale behind the changes
party this 1
m
Ok, I'll ping you later about this then
m
but... as long as we’re explaing things, why don’t we explain/apologise a bit more while giving samtools the “one tuple” treatment? 😉
you know, show the rest how it’s done
m
Yeah
When is the next maintainers meeting
m
Or is that a bit much? 😛
m
We should check if the rest of the team agree
m
@Simon Pearce is the boss of #C043UU89KKQ
While we’re changing things, I really don’t see the point of “subworkflows” with just an aligner and then `bam_stats_samtools`behind it. Just seems like a bunch of code duplication to me
🤣 2
100 rainbow 1
m
One day at a time please
m
There’s a lot of them, they have little value IMO and they just add maintenance burden whenever we want to do stuff
One day at a time please
You said you wanted to break everything 🤷🏻
m
Yeah but one day at a time
m
So we can break it twice for double the fun?
s
I agree, it is a PITA to update things like this
We should check if the rest of the team agree
Ssh, you'll wake them from their slumber!
The hackathon is really not the best time to be updating modules like samtools 😉
m
Jon is on vacation, let's wait for him, he's one of my favorite people to argue with
m
I’ve added the subworkflow thing to the agenda points for the maintainer meeting
Why hesitantly?
This is why 😰
🤣 2
m
this is where the fun begins
m
I wonder who’s going to check all the fun I’m having 😉
homer disappear 1
btw, I’ve also merged all the index outputs from 
samtools_sort
for you @Simon Pearce
❤️ 1
s
well I guess that means I can review your PR then
m
Please don’t be too harsh 🙏🏻
I’d like to merge as soon as I can coax the tests to pass
🤣 1
and I stop expanding the scope of this PR....
s
Did you ever think about a) not doing this all in one PR, and b) not doing this during the hackathon?
m
I asked @mashehu if I could do it during the hackathon 🤣
You’re right about not doing all of this in one PR though. That’s an issue I’m having everywhere...
s
about 90% of these modules aren't used in any other tests, that'd be fine to do
but samtools sort 😐
(and index, but I did that one 😉 )
m
Yeah.... samtools is everywhere in some form, so I knew there was going to be quite a fallout
somebody's gotta do it
s
samtools/merge is next on the block for getting index as a channel output, but not here
shared a GIF
m
Stop giving me ideas then...
I’m a “get all the groceries inside in one trip” kind of guy
s
Now to refresh and see how many things have actually changed
m
Godspeed good man
s
that's half an hour of my life I won't get back
m
Yeah, sorry ’bout that. you found some good stuff though.
s
Only a few bits
And some of them aren't even your 
null
the flagstat channels seem to disappear, but other than that mostly there
Fixed the fasta_index_methylseq subworkflow that was failing due to trying to access a version channel that had been removed, hopefully it should merge in now
m
Great! Thanks. Let's get this merged !
Aaand the samtools mega bump is merged! Great work everyone extreme teamwork
🙌 4
m
NICE JOB and amazing timing
s
Surprised it actually finished the tests in that time
well that's my hackathon contribution
m
Next stop, find every module that uses samtools/htslib as a dependency and update those too 😄
😱 1
s
ehj
if you really like
next job is to make samtools merge make index output
🎉 1
m
updating samtools in #C06PHJM0Y3D https://github.com/nf-core/seqinspector/pull/215
✅ 1
m
samtools/merge index update
✅ 1
❤️ 1
Thanks @Matthieu Muffato
2 Views
Open in Slack
PreviousNext
Powered by