Remove .fromSamplesheet, but create an equivalent function samplesheetToList
#3
Conversation
|
TODO change target branch to |
|
Seems like the right idea 👍 |
|
This PR is done now! Waiting for #1 to be merged before setting this ready to review |
|
@mirpedrol can you also take a look at this please? (since it's a pretty big change) |
| !!! warning | ||
|
|
||
| The `.fromSamplesheet` channel operator and `samplesheetToList` also validate the files before converting them. If you convert the samplesheet, you should not add a schema to the parameter corresponding to the samplesheet to keep your pipeline as efficient as possible. | ||
|
|
There was a problem hiding this comment.
I wouldn't add this warning, it's better to validate the sample sheet before starting the pipeline execution, even if it is then validated twice.
There was a problem hiding this comment.
Is it okay if I change it to a note instead, I think it's useful information
| stdout.contains("[[string1:extraField, string2:extraField, integer1:10, integer2:10, boolean1:true, boolean2:true], string1, 25, false, ${getRootString()}/src/testResources/test.txt, ${getRootString()}/src/testResources/testDir, ${getRootString()}/src/testResources/testDir, unique3, 1, itDoesExist]" as String) | ||
| } | ||
|
|
||
| def 'samplesheetToList - String, Path' () { |
There was a problem hiding this comment.
should we add the same test for "path, path"?
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
…hema into rework-fromSamplesheet
plugins/nf-schema/src/main/nextflow/validation/SchemaValidator.groovy
Outdated
Show resolved
Hide resolved
|
Hi all, I'm having some weird error when trying out the this branch on the nf-core test pipeline: You can see the adjusted code for the test pipeline here: https://github.com/nvnieuwk/testpipeline/tree/nf-schema-plugin I've no idea left on what to try (I've deleted all cached data I know of, I've updated the nextflow version to the latest edge version...) |
I think the edge version might be the issue! I can reproduce the same error with nextflow version |
I've also tried it with |
|
It seems these lines in the Nextflow source code are causing the issue. If I printed out @bentsherman do you have any ideas on what could be going wrong here? Is it an issue on the plugin side or could this be a bug in Nextflow? |
|
After some more testing I noticed that |
Ah, I was not using the properly adjusted testpipeline.. I also get the same error on In my edit I had manually swapped nf-validation for nf-schema in the |
|
Yeah some weird stuff is going on 😁 This needs to be fixed first though! Thanks for also going through this and confirming it's not just me |
|
I thought it might have something to do with loading an operator in a module, but I couldn't reproduce any error. If you can show me a minimal reproducible example, I can look into it |
|
I have the error in this branch of the nf-core template test pipeline: https://github.com/nvnieuwk/testpipeline/tree/nf-schema-plugin. If you try it out with this branch, you should get the error (at least @human9 and I got it by testing it this way) |
|
I've so far been unable to recreate the error in any other way. |
|
How are you providing the plugin? I see it in the |
|
I was editing I believe I can reproduce a minimal example with the nf-hello plugin. Here's a diff from mainline testpipeline that reproduces for me on nextflow diff --git a/subworkflows/local/utils_nfcore_testpipeline_pipeline/main.nf b/subworkflows/local/utils_nfcore_testpipeline_pipeline/main.nf
index dc90ad9..70b775e 100644
--- a/subworkflows/local/utils_nfcore_testpipeline_pipeline/main.nf
+++ b/subworkflows/local/utils_nfcore_testpipeline_pipeline/main.nf
@@ -11,6 +11,7 @@
include { UTILS_NFVALIDATION_PLUGIN } from '../../nf-core/utils_nfvalidation_plugin'
include { paramsSummaryMap } from 'plugin/nf-validation'
include { fromSamplesheet } from 'plugin/nf-validation'
+include { goodbye } from 'plugin/nf-hello'
include { UTILS_NEXTFLOW_PIPELINE } from '../../nf-core/utils_nextflow_pipeline'
include { completionEmail } from '../../nf-core/utils_nfcore_pipeline'
include { completionSummary } from '../../nf-core/utils_nfcore_pipeline'
@@ -41,6 +42,8 @@ workflow PIPELINE_INITIALISATION {
ch_versions = Channel.empty()
+ channel.of('uh oh').goodbye()
+
//
// Print version and exit if required and dump pipeline parameters to JSON file
//
diff --git a/workflows/testpipeline.nf b/workflows/testpipeline.nf
index 5a1bf8e..5f88a38 100644
--- a/workflows/testpipeline.nf
+++ b/workflows/testpipeline.nf
@@ -7,6 +7,7 @@
include { FASTQC } from '../modules/nf-core/fastqc/main'
include { MULTIQC } from '../modules/nf-core/multiqc/main'
include { paramsSummaryMap } from 'plugin/nf-validation'
+include { goodbye } from 'plugin/nf-hello'
include { paramsSummaryMultiqc } from '../subworkflows/nf-core/utils_nfcore_pipeline'
include { softwareVersionsToYAML } from '../subworkflows/nf-core/utils_nfcore_pipeline'
include { methodsDescriptionText } from '../subworkflows/local/utils_nfcore_testpipeline_pipeline'
@@ -33,9 +34,1N E X T F L O W ~ version 23.10.1
Launching `./main.nf` [agitated_boltzmann] DSL2 - revision: d17b498066
ERROR ~ Operator 'goodbye' conflict - it's defined by plugin nf-hello and nf-hello
-- Check script './workflows/../subworkflows/local/utils_nfcore_testpipeline_pipeline/main.nf' at line: 14 or see '.nextflow.log' file for more details
3 @@ workflow TESTPIPELINE {
FASTQC (
ch_samplesheet
)
+
ch_multiqc_files = ch_multiqc_files.mix(FASTQC.out.zip.collect{it[1]})
+output: |
|
as far as I can tell the key to triggering it is importing the plugin in more than one workflow |
I forgot to push the latest changes sorry for that! I installed the plugin like you showed in the podcast with |
|
Even more minimal! https://github.com/human9/nextflow-import-issue All it requires is the include in two different files. |
|
Thank you so much @human9 🥳 |
|
Thanks @human9 , looks like you found a Nextflow bug. Interesting because nf-core/rnaseq already includes from nf-validation in multiple modules without any issue. Makes me think the bug happens only with operators @nvnieuwk while I do intend to squash the bug in Nextflow, I am also wondering if you really need both the schema = file('schema_input.json')
// equivalent to channel factory
Channel.fromList( fromSamplesheet(path, schema) )
// equivalent to operator
Channel.fromPath('samplesheet.csv') | flatMap { path -> fromSamplesheet(path, schema) }I don't mean to dodge the operator issue, but it does seem like that would simplify things for you. I'm also trying to discourage people from creating operators that could just be a map/flatMap + regular function. |
|
That's actually a pretty good idea! Maybe I'll tweak the naming a bit so it's clear in the migration that it's no longer a factory/operator but a function ( I'll repost this in the nf-validation slack channel first though to see if everyone is on board with this. Thanks for the help @bentsherman and @human9. I'll implement the changes now! |
nvnieuwk
changed the title
.fromSamplesheet to a channel operator + create an equivalent function samplesheetToList.fromSamplesheet, but create an equivalent function samplesheetToList
|
Okay I'll just merge this for now. We can still change some things if stuff comes up later |
As suggested on Slack, the
.fromSamplesheetchannel factory has now been converted to an operator to make it possible to use files created by other pipelines in a meta pipeline. A function has also been added that returns a list of the samplesheet contents.