Added support for Cleanifier tool: cleanifier/download, cleanifier/index and cleanifier/filter#12360
Conversation
LeonHornich
added
Ready for Review
new module
|
I would suggest to split this up into one PR by subtool / command :) regarding the md5sum instability I will add a comment |
There was a problem hiding this comment.
We want the ext.args to be present in the main.nf.test file. That makes everything more readable in one go. See the module specifications for more info.
There was a problem hiding this comment.
Please remove this file :)
| @@ -0,0 +1,5 @@ | |||
| process { | |||
| withName: CLEANIFIER_INDEX { | |||
| ext.args = "-k 31" | |||
There was a problem hiding this comment.
We want the ext.args to be present in the main.nf.test file. That makes everything more readable in one go. See the module specifications for more info.
| """ | ||
| } | ||
| } | ||
|
|
||
| then { | ||
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot(sanitizeOutput(process.out)).match() } | ||
| { assert snapshot(process.out).match() } |
There was a problem hiding this comment.
I would just first assert the txt content of the filter output and then we can probably see why the md5sum is instable
| { assert snapshot(process.out).match() } | |
| { assert snapshot( | |
| file(process.out.filter[0][1]).name, | |
| process.out.info, | |
| process.out.findAll { key, val -> key.startsWith("versions") }, | |
| ).match() } |
Edit: I updated to just use the name
There was a problem hiding this comment.
I played around with the index tests. From my understanding the core issue is that cleanifier/index generates a index file using a probabilistic cuckoo filter. These are not deterministic and therefore differ between snapshots.
Since this doesn't seem avoidable i am now only assessing the files existence (via file path) in the most recent commit.
While this seems to improve my snapshots locally, I now get an error while linting:
nf-core modules lint cleanifier (nextflow_env)
,--./,-.
___ __ __ __ ___ /,-._.--~\
|\ | |__ __ / ` / \ |__) |__ } {
| \| | \__, \__/ | \ |___ \`-._,-`-,
`._,._,'
nf-core/tools version 4.0.2 - https://nf-co.re
INFO Linting modules repo: '.'
INFO Linting module: 'cleanifier'
╭─ [✗] 1 Module Test Failed ───────────────────────────────────────────────────╮
│ ╷ ╷ │
│ Module name │ File path │ Test message │
│╶────────────────────┼───────────────────────────┼───────────────────────────╴│
│ cleanifier/index │ modules/nf-core/cleanifi… │ test_snap_versions: │
│ │ │ versions not found in │
│ │ │ snapshot file │
│ ╵ ╵ │
╰──────────────────────────────────────────────────────────────────────────────╯
╭───────────────────────╮
│ LINT RESULTS SUMMARY │
├───────────────────────┤
│ [✔] 166 Tests Passed │
│ [!] 0 Test Warnings │
│ [✗] 1 Test Failed │
╰───────────────────────╯
How would you usually handle this according to nf-core guidelines? Thank you :)
There was a problem hiding this comment.
You need to add this line:
process.out.findAll { key, val -> key.startsWith("versions") },
because otherwise the versions are not asserted
| setup { | ||
| run("CLEANIFIER_INDEX") { | ||
| script "../../index/main.nf" | ||
| config "./nextflow.config" |
There was a problem hiding this comment.
| config "./nextflow.config" |
duplicated to above
famosab
left a comment
famosab
left a comment
There was a problem hiding this comment.
Nice work, I think these can be merged :)
2 participants
With this PR I want to add support for the Cleanifier tool for species contamination removal. There are 3 added modules: download, index and filter.
Unfortunately I am still experiencing some issues with nf-test snapshot stability. If anyone has an idea on how to fix them i'd very much appreciate that. Of course other feedback is also highly appreciated.
PR checklist
Closes #XXX
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile conda