Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove "None" definition from schema #851

Merged
merged 1 commit into from
Nov 17, 2022
Merged

Remove "None" definition from schema #851

merged 1 commit into from
Nov 17, 2022

Conversation

drpatelh
Copy link
Member

Specifying "None" here in the schema breaks the config resolution on Tower meaning we are unable to use the pipeline from the Launchpad. The error that is generated is below:

If you use nf-core/sarek for your analysis please cite:
* The pipeline
  https://doi.org/10.12688/f1000research.16665.2
  https://doi.org/10.5281/zenodo.4468605
* The nf-core framework
  https://doi.org/10.1038/s41587-020-0439-x
* Software dependencies
  https://github.com/nf-core/sarek/blob/master/CITATIONS.md
-�[2m----------------------------------------------------�[0m-
Unable to read script: '/.nextflow/assets/nf-core/sarek/./workflows/sarek.nf' -- cause: /None

This was added in 6fd00e1 by @FriederikeHanssen ! Figuring this out has consumed my entire day 😅 You're welcome!

@drpatelh drpatelh requested a review from maxulysse as a code owner November 17, 2022 14:23
@github-actions
Copy link

github-actions bot commented Nov 17, 2022 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit ba359ba

+| ✅ 151 tests passed       |+
#| ❔   8 tests were ignored |#
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline
  • schema_description - No description provided in schema for parameter: cnvkit_reference

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.6
  • Run at 2022-11-17 14:24:48

@maxulysse
Copy link
Member

I think @asp8200 actually encountered some schema building issue when adding a params.
@asp8200 can you remind me what it was?

@FriederikeHanssen
Copy link
Contributor

I am sorry

@drpatelh
Copy link
Member Author

That's ok! All of the full-sized tests are running out-of-the-box now so I owed you one!

@asp8200
Copy link
Contributor

asp8200 commented Nov 17, 2022 •
edited
Loading

I think @asp8200 actually encountered some schema building issue when adding a params. @asp8200 can you remind me what it was?

Yeah, I ran nf-core schema build and that changed a "default": null was also changed to "default": "None"

image

(But I didn't commit that change to dev or master)

@maxulysse
Copy link
Member

ok, so given what @asp8200 just said, I think we need to fix tools and the schema so that no "None" definition are there.
So not your fault @FriederikeHanssen

@maxulysse maxulysse mentioned this pull request Nov 17, 2022
Closed
@maxulysse maxulysse merged commit c22bd34 into dev Nov 17, 2022
@maxulysse maxulysse deleted the rm_none branch November 17, 2022 14:37
@pditommaso
Copy link

Holy crap, why it was happening only on Tower

@ewels
Copy link
Member

ewels commented Nov 17, 2022

Holy crap, why it was happening only on Tower

I guess because the launchpad sends through all params from the form - including the default values supplied by the schema.

This is different behaviour to running the pipeline alone (quick launch or local). It's also different to the nf-core launch tools (web, cli), which only pass on values from the schema which have been changed from the default.

@ewels
Copy link
Member

ewels commented Nov 17, 2022

Does this make sense? Makes perfect sense in my head but I'm not sure that I explained it very well 😆 Can try again if anyone is confused ☝🏻

@pditommaso
Copy link

Likely. It would be nice to have a snippet to replicate the error so that the error message can be better handled by nextflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@drpatelh @maxulysse @FriederikeHanssen @asp8200 @pditommaso @ewels