Added samshee #6749

nschcolnicov · 2024-10-07T18:12:02Z

PR checklist

Closes #XXX

grst · 2024-10-08T11:12:30Z

modules/nf-core/samshee/main.nf

+
+    input:
+    tuple val(meta), path(samplesheet)
+    val(validator_schema)              //optional


It's tricky to fully support samshee's range of inputs here.
Samshee can accept either

a schema name e.g. urn:samshee:illuminav2/v1

a json string, e.g. {"required": ["BCLConvert_Data"]}

a URI pointing to a local file, e.g. {"$ref": "file:./test.schema.json"}

a URI pointing to a remote file, e.g. '{"$ref": "https://dataportal.lit.eu/schemas/litngscoresamplesheet/v0.1/litngscoresamplesheet.schema.json"}'

plus it can accept multiple schemas. Especially when referencing a local file, value channels are not ideal, as when working with a cloud-based solution, the file needs to be provisioned by nextflow. And even when it's a remote URL, it might be preferable to have the download handled by nextflow.

Maybe it could work to have two input channels, one accepting value, and one accepting files, such that a call to the samshee module looks like

SAMSHEE( [['id': 'samplesheet01'], path("samplesheet.csv")], ['{"required": ["Data"]}', '{"$ref": "urn:samshee:illuminav2/v1"}'], [path("file:./test.schema.json"), path('https://dataportal.lit.eu/schemas/litngscoresamplesheet/v0.1/litngscoresamplesheet.schema.json')] )

WDYT?

)

Hi @grst I think this makes sense, I'll work on it

grst · 2024-10-08T11:14:54Z

Here's a testcase. Note that this is a V1 samplesheet, see also my comment here: nf-core/demultiplex#268 (comment)

[Header],,,,,,,,
IEMFileVersion,4,,,,,,,
Experiment Name,241003_RNASeq_FFPE,,,,,,,
Date,03.10.2024,,,,,,,
Workflow,GenerateFASTQ,,,,,,,
Application,FASTQ Only,,,,,,,
Assay,TruSeq HT,,,,,,,
Chemistry,Amplicon,,,,,,,
,,,,,,,,
[Reads],,,,,,,,
Read1Cycles,102,,,,,,,
Read2Cycles,102,,,,,,,
Index1Cycles,8,,,,,,,
Index2Cycles,8,,,,,,,
,,,,,,,,
[Settings],,,,,,,,
,,,,,,,,
[Data],,,,,,,,
Sample_ID,Sample_Name,Sample_Plate,Sample_Well,I7_Index_ID,index,I5_Index_ID,index2,Sample_Project
101_xxxx,,,A1,S762,TTACCGAC,S512,CGTATTCG,
102_xxxx,,,B1,S713,TCGTCTGA,S586,TCAAGGAC,
103_xxxx,,,C1,S736,TTCCAGGT,S543,AAGCACTG,
104_xxxx,,,D1,S709,TACGGTCT,S575,GCAATGGA,

modules/nf-core/samshee/environment.yml

modules/nf-core/samshee/main.nf

modules/nf-core/samshee/tests/main.nf.test

modules/nf-core/samshee/tests/nextflow.config

modules/nf-core/samshee/tests/main.nf.test

nschcolnicov · 2024-10-10T16:22:30Z

@grst Some updates on this:
Using --output-format sectioned on the samplesheet doesn't work on all v1 samplesheets. For example, it didn't work on the one you provided or the one from the test profile. However, it did work on the one from the test_full profile: https://raw.githubusercontent.com/nf-core/test-datasets/demultiplex/testdata/miseq_35147139/miseq_35147139_samplesheet.csv

Regarding having a different channel for local and remote json files, it shouldn't be needed since, as long as we stage the remote file, it will always be local

nschcolnicov · 2024-10-10T20:02:45Z

@grst I added the additional features you requested, let me know what you think! @atrigila Also implemented your suggestions, I'm waiting on the bioconda package https://github.com/bioconda/bioconda-recipes/pull/51315/checks?check_run_id=31375653510

grst · 2024-10-11T08:19:19Z

modules/nf-core/samshee/main.nf

+    def arg_json_schema_validator = json_schema_validator ? "--schema '${json_schema_validator}'"                         : ""
+    def arg_name_schema_validator = name_schema_validator ? "--schema '${name_schema_validator}'"                         : ""


I think we could either merge these two into one channel, then the user would need to specify '{"$ref": "urn:samshee:illuminav2/v1"} or I'd ajust the second line as follows:

Suggested change

def arg_json_schema_validator = json_schema_validator ? "--schema '${json_schema_validator}'" : ""

def arg_name_schema_validator = name_schema_validator ? "--schema '${name_schema_validator}'" : ""

def arg_json_schema_validator = json_schema_validator ? "--schema '${json_schema_validator}'" : ""

def arg_name_schema_validator = name_schema_validator ? "--schema '{\"\$ref\": \"${name_schema_validator}\"'" : ""

I have a slight preference for the single channel version as it keeps the module simpler.

I agree with single channel version. I would suggest (untested):

def arg_schema = json_schema_validator ? "--schema '${json_schema_validator}'" : (task.ext.args?.contains("--schema") ? "" : "")

@grst I also thought of having the user just specify just the name, but I thought it could add a layer of confusion. I did it this way so that they can just use exactly the same strings that are displayed in the samshee readme. Doing it this way we would need to add an explanation saying that they need to extract the string from the readme that comes after the "$ref:", do you think it would be easier for them to understand?

I went with the approach of not having the channels combined into a tuple to make it easier to avoid issues regarding optional inputs.

fair enough, let's go with a single channel for schemas specified as string then (+ the second one for paths).

grst · 2024-10-11T08:20:52Z

modules/nf-core/samshee/main.nf

+    def arg_json_schema_validator = json_schema_validator ? "--schema '${json_schema_validator}'"                         : ""
+    def arg_name_schema_validator = name_schema_validator ? "--schema '${name_schema_validator}'"                         : ""
+    def arg_file_schema_validator = file_schema_validator ? "--schema '{\"\$ref\": \"file:${file_schema_validator}\"}'" : ""
+    def arg_v1_schema             = params.v1_schema      ? "--output-format sectioned"                                   : ""


I don't think a module should ever (?) access a global param. So this should either be task.ext.v1_schema, or just rely on the user to provide the "sectioned" flag via task.ext.args when necessary.

I agree. I would supply it via task.ext.args

modules/nf-core/samshee/main.nf

atrigila · 2024-10-14T19:14:45Z

modules/nf-core/samshee/main.nf

+    container "${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ?
+        'docker://community.wave.seqera.io/library/pip_samshee:733e11f3377fc2e3' :
+        'community.wave.seqera.io/library/pip_samshee:733e11f3377fc2e3' }"


Here is a comment with the latest guidelines on incorporating seqera containers to modules:

https://nfcore.slack.com/archives/CJRH30T6V/p1728901074405529

Check the container settings for singularity (steps 8-11), I think they require https.

atrigila · 2024-10-14T19:18:03Z

modules/nf-core/samshee/main.nf

+    val(json_schema_validator)  // optional
+    val(name_schema_validator)  // optional


All non-mandatory command-line tool non-file arguments MUST be provided as a string via the $task.ext.args variable. (https://nf-co.re/docs/guidelines/components/modules#optional-command-arguments)

atrigila · 2024-10-14T19:56:20Z

modules/nf-core/samshee/main.nf

+    def arg_json_schema_validator = json_schema_validator ? "--schema '${json_schema_validator}'"                         : ""
+    def arg_name_schema_validator = name_schema_validator ? "--schema '${name_schema_validator}'"                         : ""


I agree with single channel version. I would suggest (untested):

def arg_schema = json_schema_validator ? "--schema '${json_schema_validator}'" : (task.ext.args?.contains("--schema") ? "" : "")

atrigila · 2024-10-14T19:57:26Z

modules/nf-core/samshee/main.nf

+    def arg_json_schema_validator = json_schema_validator ? "--schema '${json_schema_validator}'"                         : ""
+    def arg_name_schema_validator = name_schema_validator ? "--schema '${name_schema_validator}'"                         : ""
+    def arg_file_schema_validator = file_schema_validator ? "--schema '{\"\$ref\": \"file:${file_schema_validator}\"}'" : ""
+    def arg_v1_schema             = params.v1_schema      ? "--output-format sectioned"                                   : ""


I agree. I would supply it via task.ext.args

atrigila · 2024-10-14T19:58:39Z

modules/nf-core/samshee/main.nf

+    $arg_json_schema_validator \
+    $arg_name_schema_validator \
+    $arg_file_schema_validator \


Replace for a single arg_schema that will be populated depending on whether there is an input json file OR the ext.args contain --schema

I don't think I follow, so users can use many of arg_json_schema_validator and arg_name_schema_validator and arg_file_schema_validator. Aren't they mutually exclusive?

Aren't they mutually exclusive?

No, they are not. The user could for instance specify the '{"$ref": "urn:samshee:illuminav2/v1"} schema, and additionally validate against a custom schema provided as json file.

modules/nf-core/samshee/main.nf

…te_samshee

nschcolnicov · 2024-10-15T14:49:53Z

@grst @atrigila We will be passing all string values through the ext.args, so there is no longer a need to combine the channels. I also removed the custom ext.args since this is also not good practices, and we can organize all of these parameters through the modules.config.
Linting for the module fails due to the already reported issue about having pip in the environment.yml

Note:
I already started a PR on demultiplex, so I could test these changes, you can look here how this module would fit into the pipeline, as well as how we can set it up: https://github.com/nf-core/demultiplex/pull/275/files#diff-dfadb23810ec7ce446f121e68f79a112e361d6a833cce1701224a89295c7b8d6
I still need to merge this PR first and then install it into demultiplex using nf-core tooling, but just so that you get a peak.

Added samshee

1dc138b

nschcolnicov marked this pull request as ready for review October 7, 2024 18:12

nschcolnicov mentioned this pull request Oct 7, 2024

Simplify samshee module nf-core/demultiplex#268

Open

nschcolnicov requested review from grst and atrigila October 7, 2024 18:13

nschcolnicov added 2 commits October 7, 2024 18:39

Fix pre-commit

3eba10c

Fix linting

ef328fc

grst reviewed Oct 8, 2024

View reviewed changes

atrigila reviewed Oct 10, 2024

View reviewed changes

nschcolnicov added 2 commits October 10, 2024 19:21

Add more options

6c539a1

Remove nextflow.config

31b03e1

grst requested changes Oct 11, 2024

View reviewed changes

atrigila requested changes Oct 14, 2024

View reviewed changes

nschcolnicov added 3 commits October 15, 2024 13:28

Addressing PR comments

8c80296

Merge branch 'master' of https://github.com/nf-core/modules into upda…

435bccd

…te_samshee

Add python version

dc9d42e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added samshee #6749

Added samshee #6749

nschcolnicov commented Oct 7, 2024

grst Oct 8, 2024 •

edited

Loading

nschcolnicov Oct 9, 2024

grst commented Oct 8, 2024 •

edited

Loading

nschcolnicov commented Oct 10, 2024 •

edited

Loading

nschcolnicov commented Oct 10, 2024 •

edited

Loading

grst Oct 11, 2024

atrigila Oct 14, 2024

nschcolnicov Oct 15, 2024 •

edited

Loading

grst Oct 15, 2024

grst Oct 11, 2024

atrigila Oct 14, 2024

atrigila Oct 14, 2024

atrigila Oct 14, 2024

atrigila Oct 14, 2024

atrigila Oct 14, 2024

atrigila Oct 14, 2024

atrigila Oct 15, 2024

grst Oct 15, 2024

nschcolnicov commented Oct 15, 2024 •

edited

Loading

		def arg_json_schema_validator = json_schema_validator ? "--schema '${json_schema_validator}'" : ""
		def arg_name_schema_validator = name_schema_validator ? "--schema '${name_schema_validator}'" : ""

		val(json_schema_validator) // optional
		val(name_schema_validator) // optional

Added samshee #6749

Are you sure you want to change the base?

Added samshee #6749

Conversation

nschcolnicov commented Oct 7, 2024

PR checklist

grst Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grst commented Oct 8, 2024 • edited Loading

nschcolnicov commented Oct 10, 2024 • edited Loading

nschcolnicov commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nschcolnicov Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nschcolnicov commented Oct 15, 2024 • edited Loading

grst Oct 8, 2024 •

edited

Loading

grst commented Oct 8, 2024 •

edited

Loading

nschcolnicov commented Oct 10, 2024 •

edited

Loading

nschcolnicov commented Oct 10, 2024 •

edited

Loading

nschcolnicov Oct 15, 2024 •

edited

Loading

nschcolnicov commented Oct 15, 2024 •

edited

Loading