-
Notifications
You must be signed in to change notification settings - Fork 31
Migrate to topic channels for versions #431
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
base: dev
Are you sure you want to change the base?
Conversation
|
Updated CHANGELOG for version 3.2.0 with changes and dependency updates.
|
@jonasscheid I've opened a new pull request, #433, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: jonasscheid <[email protected]>
Remove cache buster artifacts from topic channel migration
Joon-Klaps
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% certain if everything went ok with template merge.
Think you might to fix of couple stuff first before the linter will be happy.
| tuple val(meta), path("*.psmpkl") , emit: psmpkl | ||
| tuple val(meta), path("*.peakpkl"), emit: peakpkl | ||
| path "versions.yml" , emit: versions | ||
| tuple val("${task.process}"), val('easypqp'), eval("easypqp --version 2>&1 | grep -oP '(?<=easypqp, version )\\d+\\.\\d+\\.\\d+'"), emit: versions, topic: versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want to create a topic and emit variable with the same name. I think it's the first time I see this, typically it's convert_versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for all other files migrations
| script: | ||
| def prefix = task.ext.prefix ?: "${meta.id}_resolved" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| """ | ||
| IDConflictResolver \\ | ||
| -in $consensus \\ | ||
| IDConflictResolver -in $consensus \\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intentional?
| def args = task.ext.args ?: '' | ||
| def out_names = idxmls.collect { it.baseName.replace('_fdr_filtered','')+'.trafoXML' }.join(' ') | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did those pass linting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have them in quite a few files, double check if this is inetentional that you add an empty line with just spaces in them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you also updated a lot of nf-core modules. Think it would be best if you patched them as well in nf-core/modules repo themselves.
Did it pass linting given that all md5sums will be different for these modules
| // Raw file conversion | ||
| THERMORAWFILEPARSER(branched_ms_files.raw) | ||
| ch_versions = ch_versions.mix(THERMORAWFILEPARSER.out.versions) | ||
| // ch_versions = ch_versions.mix(THERMORAWFILEPARSER.out.versions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think u forgot this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forgot to delete them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
| PREPARE_SPECTRA(ch_samplesheet) | ||
| ch_versions = ch_versions.mix(PREPARE_SPECTRA.out.versions) | ||
|
|
||
| // Decoy Database creation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woops duplicate line, also i see some commented out lines that can be removed
| // Decoy Database creation |
Merge after #429
Nf-core module topic channel merge should be the next step. I did migrate to topic channels in pipeline nf-core modules for consistency reasons.
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).