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.

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

Stand alone schema #584

Merged
merged 27 commits into from
Mar 17, 2020
Merged

Stand alone schema #584

merged 27 commits into from
Mar 17, 2020

Conversation

ewels
Copy link
Member

@ewels ewels commented Mar 6, 2020

Started building a stand-alone JSON Schema for #574

Schema added for the template, also refactoring code for nf-core launch to work with this new schema structure.

@ewels ewels added the WIP Work in progress label Mar 6, 2020
@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #584 into dev will decrease coverage by 2.54%.
The diff coverage is 35.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #584      +/-   ##
==========================================
- Coverage   64.92%   62.37%   -2.55%     
==========================================
  Files          12       13       +1     
  Lines        1796     2076     +280     
==========================================
+ Hits         1166     1295     +129     
- Misses        630      781     +151
Impacted Files Coverage Δ
nf_core/launch.py 10.37% <0%> (+10.37%) ⬆️
nf_core/schema.py 31.49% <31.49%> (ø)
nf_core/lint.py 91.73% <75.86%> (-1.04%) ⬇️
nf_core/workflow/workflow.py 55.55% <0%> (+55.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97ab1cb...cb5fafb. Read the comment docs.

@ewels ewels force-pushed the stand-alone-schema branch from 931b80f to 7340d2c Compare March 17, 2020 09:35
@ewels ewels requested a review from a team March 17, 2020 10:58
@ewels ewels added command line tools Anything to do with the cli interfaces template nf-core pipeline/component template and removed WIP Work in progress labels Mar 17, 2020
@ewels ewels marked this pull request as ready for review March 17, 2020 11:00
@ewels
Copy link
Member Author

ewels commented Mar 17, 2020

Ok, so this is getting there now. Still to do:

  • Rewrite nf-core launch to work with the new schema
  • Write some tests!

However, the PR is already pretty massive, so I'm thinking that perhaps I should do these two in a separate PR so that it's not overwhelming.

New stuff done in this PR:

  • Three new commands: nf-core schema build, nf-core schema lint and nf-core schema validate
  • New tests in nf-core lint for nextflow schema
  • Update to the pipeline template schema file

Note that I have made having a nextflow_schema.json file a hard requirement - nf-core lint will fail if the file is not present. And I have also added a hard fail if any parameters from nextflow config are not described in the schema. Feedback welcome!

This PR has to be reviewed in conjunction with nf-core/website#329, which provides the graphical tools to be used with nf-core schema build.

}
},
"required": [
"reads"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆 Would like your --input here!

@drpatelh
Copy link
Member

Looks great! I was just waiting for this to add .json files to atacseq, chipseq and imcyto before I release.

Couple of questions:

  • Can we check that parameters are in snake_case format?
  • Will the nf-core schema build factor in params that are defined in main.nf and not nextflow.config?

@drpatelh
Copy link
Member

I think I will have to play with this to get a proper idea as to how everything is working as with nf-core/website#329

Maybe worth merging these if you are happy with them and Ill give it a test run.

@ewels
Copy link
Member Author

ewels commented Mar 17, 2020

Can we check that parameters are in snake_case format?

This is something for nf-core lint really, it's not to do with the schema as such.

Will the nf-core schema build factor in params that are defined in main.nf and not nextflow.config?

No, see nextflow-io/nextflow#876

This may be a reason to make the linting warn instead of fail. But at the same time, is there a reason not to define stuff in nextflow.config and main.nf if it's needed in the latter?

Maybe worth merging these if you are happy with them and I'll give it a test run.

I'm happy, especially the website one which shouldn't affect anything else. This one does affect quite a lot of behaviour, but I think it's probably ok to merge assuming that we want to push it out in the next release.

@drpatelh
Copy link
Member

Ok 👍 Are we ignoring the codecov tests?

@ewels
Copy link
Member Author

ewels commented Mar 17, 2020

Codecov is just complaining because I wrote a load of new code and haven't written any tests. I've been working on this solidly for three and a half days now though and need to catch up on some other work, so figured it is better to get it out and usable ASAP instead of leaving it sitting on the shelf. It also makes the PR smaller and easier to review.

But - nf-core launch is now broken with these changes, so I do need to more. I'll try to do these together.

@drpatelh
Copy link
Member

I feel you 👍

@drpatelh drpatelh merged commit a84c619 into nf-core:dev Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command line tools Anything to do with the cli interfaces template nf-core pipeline/component template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants