CDCgov / phoenix

🔥🐦🔥PHoeNIx: A short-read pipeline for healthcare-associated and antimicrobial resistant pathogens
Apache License 2.0
52 stars 19 forks source link

[New Feature Request] - Move all params out of config files into nf files #81

Closed erinyoung closed 1 year ago

erinyoung commented 1 year ago

Is your feature request related to a problem? Please describe. We are getting PHOENIX up and running on Illumina ICA. Illumina ICA's use of nextflow, however, doesn't allow the use of config files... which is where all of your params are defined.

Describe the solution you'd like Define all your params in the main.nf file

Describe alternatives you've considered Right now we have a custom fork of this repo that we're using, which is going to be a pain to keep in sync with this repo.

jvhagey commented 1 year ago

Hi @erinyoung, follow up question on this. Is ICA going to be mad if the nextflow.config is still in the repo? Looking at your converted scripts I don't see one there. Also, looks like the base.config is still where the memory/cpus resources are defined. So its ok to have the configs in the conf folder?

erinyoung commented 1 year ago

Here's a comparison of what we did to mycosnp:

CDC's sparkly version: https://github.com/CDCgov/mycosnp-nf/blob/master/main.nf#L1-L30

UPHL's ugly added param version : https://github.com/UPHL-BioNGS/ICA_Modified_Nextflow_Scripts/blob/main/CDC_mycosnp_V1_3/main.nf#L1-L30

ICA doesn't read the nextflow.config file, so information like profiles or any of the directives aren't read in unless they are actually part of a process. They're in the ICA_Modified_Nextflow_Scripts repos because of how we cloned files in, and didn't bother removing unneeded files.

jvhagey commented 1 year ago

@erinyoung does this mean ICA also doesn't read in the modules.config ? I noticed the task resource requests are also in the process in ICA version of mycosnp...

erinyoung commented 1 year ago

thanks for creating a bundle on ICA! This is now a non-issue