https://nf-co.re logo
Join Slack
Powered by
Would it be a bad idea to change the default outpu...
# modules
p
Would it be a bad idea to change the default output of the bwa/index module to 
${meta.id}/*.{amb,ann,bwt,pac,sa}
instead of 
bwa/*.{amb,ann,bwt,pac,sa}
? I noticed two issues: 1. While working on fastqscreen - where index files for several references are processed simultaneously - I noticed that nextflow had problems with staging similarly named 
bwa
directories. This could be fixed by using 
stageAs
in the input section of the process though. 2. When publishing the output of bwa/index when it was run on multiple input reference genomes, only one of the references gets published, unless the modules.config is modified (e.g. 
path: { "${params.outdir}/reference/${meta.id}" }
, but then you do end up with nested 
genome/bwa/*
folders). I couldn't figure out a way to do it using 
saveAs
). Or would this change break things/guidelines I'm not considering?
m
go for ${prefix}/...
p
Prefix is already used though
m
confused dog
p
Copy code
bwa \\
        index \\
        $args \\
        -p bwa/${prefix} \\
        $fasta
m
oooh
let me check what I did
p
and prefix is being set to 
fasta.baseName
(which seems like a good idea imo), but 
meta.id
is not used.
m
yeah, then my recommendation
use fasta.basename where we had prefix
and replace bwa by prefix
p
I'm just worried there might be workflows or modules that rely on the hardcoded bwa folder name.
But that's a great idea!
extreme teamwork 1
m
yeah, but it's easy enough to deal with
it's your idea you know
I'm happy to share the blame
rubber duck 1
s
There is an open PR for this I think
Which points out for publishing it would be helpful, when you are indexing lots of different genomes in one pipeline
p
Oh! Why didn't that PR end up being accepted though?
s
Uhh, nobody pushed for it and made sure it went through?
it is marked as draft with failing tests 🤷
The repository is littered with similar PRs 😉
That haven't been followed up at all
p
Aw, that's a shame... Well, in that case, I've created a new PR implementing the fix we discussed above. I've also linked back to the previous PRs, just in case that approach might be favoured after all. Everything seems to work with this approach though, although I dislike the new output emit path. https://github.com/nf-core/modules/pull/9947
👀 1
m
looking good
you need to check out the depending subworkflows
p
(forgot to update meta.yml, hang on)
m
I made a suggestion on the prefix usage
p
Cheers!
m
How about: task.ext.prefix ?: meta.id ? "${meta.id}" : "${fasta.baseName}"
👍 1
p
Another quick question: according to https://nf-co.re/docs/guidelines/components/modules?utm_source=chatgpt.com#input-and-output-channel-types the only valid input/output types in meta.yml are ``map`, 
file
, 
directory
, 
string
, 
boolean
, 
integer
, 
float
, 
boolean
, 
list
, but I could've sworn I've seen path being used before as well. Am I dreaming?
m
isn't directory what you want to use here?
but yeah, path does ring a bell
p
I admit directory makes more sense, but I suddenly became very aware I have no idea if I've been using path/file/dir consistently elsewhere 🥲
🤷 1
s
My approach is if it passes linting it is fine, frankly I try not to spend any more time on the meta.yml than I have to
👍 2
😆 1
p
Aw. Linter is indeed a bit unhappy. But not about path vs directory. I think it's the output named ${prefix} that's setting it off.
Copy code
│ Module name  │ File path                          │ Test message                                                                                                                                                                                 │
│╶─────────────┼────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╴│
│ bwa/index    │ modules/nf-core/bwa/index/meta.yml │ correct_meta_outputs: Module meta.yml does not match main.nf. Outputs should contain: {'index': [['meta', '${prefix}/']], 'versions_bwa': [['${task.process}', 'bwa', 'bwa 2>&1 | sed -n     │
│              │                                    │ "s/^Version: //p']]} Run nf-core modules lint --fix to update the meta.yml file.                                                                                                             │
│ bwa/index    │ modules/nf-core/bwa/index/meta.yml │ correct_meta_topics: Module meta.yml does not match main.nf. Topics should contain: {'versions': [['${task.process}', 'bwa', 'bwa 2>&1 | sed -n "s/^Version: //p']]} Run nf-core modules     │
│              │                                    │ lint --fix to update the meta.yml file.
I didn't touch the meta_topics part though ¯\_(ツ)_/¯ ( 
lint --fix
just empties the respective blocks so that's no good)
m
oooh
I had that before
👀 1
check unzip or bcftools/isect
👍 1
or even gatk4/genomicsdbimport
sorry, github appears to be down to me
is it a hackathon already???
😮 1
p
Hm. I think I've implemented it the same as in those examples now, but the linter still complains. However, it's also failing on the master branch without any changes for me.
m
facepalm #C07KND2JP54 LINTING
shakefist
🥲 2
p
Added a brief description to the PR. Let me know if there's anything else I can try, but for now I'm stuck otherwise I think :)
Thanks for all the assistance so far though!
m
I'll have a look later today, thanks
WE WILL CARRY IT THROUGH
It's my today's #C0749PAEU9F
p
Hah, good luck, but no rush from my side 😉
shared a GIF
m
What was the problem?
I haven't done a thing, but it works
🤣 1
shared a GIF
s
Linting the module might be working, but a bunch of the downstream modules that use bwa/index in their tests are failing...
m
yeah, I'm on it
and then I'll do bwamem2/index
p
Still failing locally for me though?
m
update tools to the latest dev
I'll prepare a PR for all gridss related modules, so no need to fix that
gridss fixed upstream, so we're good
I committed and pushed a small change to your implementation to allow for a finer control of filename using suffix as svaba needs the index files to be named in a certain way
yeah I know, DUMB TOOLS
@Simon Pearce care to review, I feel like I have contributed too much to review now: https://github.com/nf-core/modules/pull/9947
@Pieter Moris I told you it was my #C0749PAEU9F 🚀
party parrot 1
p
Oof. I haven't gotten around to checking out your last changes yet, Maxime. Will let you know when I do!
s
The only contentious part is the use of "suffix" now right, @Maxime Garcia?
m
Meh
For me it's not contentious
But I can see other people saying that
🤣
2 Views
Open in Slack
PreviousNext
Powered by