nf-core / tools

Python package with helper tools for the nf-core community.
https://nf-co.re
MIT License
232 stars 187 forks source link

Remove try-catch blocks around institutional configs #3132

Open bentsherman opened 3 weeks ago

bentsherman commented 3 weeks ago

The nf-core pipeline template has these try-catch blocks to load institutional configs:

https://github.com/nf-core/tools/blob/7785b4cd8d3ce1321832e1a7ae4f2874509e31f4/nf_core/pipeline-template/nextflow.config#L76-L88

But try-catch blocks won't be supported by the new config parser, so it will need to be removed here.

If this safeguard is still important, I recommend removing these includes entirely and instead make it easy to load them into your environment, independently of any pipeline.

For example, you could use the nf-core CLI to download an institutional config:

nf-core config <profile> > $HOME/.nextflow/config

Similarly for a pipeline-specific config:

# environment setup
nf-core config <pipeline> <profile> > extra.config

# on each pipeline run
nextflow run -c extra.config <pipeline> # ...

I think we could implement some similar conveniences in Seqera Platform, if people take interest in this approach. It's up to you guys, but either way, the try-catch blocks are gonna have to go.

mashehu commented 3 weeks ago

Cc @jfy133 🤔

jfy133 commented 3 weeks ago

Following also the slack thread this issue came out of, this is indeed a very old thing, and 'loathe to touch the way things work' as Phil said there.

However, we need to come up with a different solution to the ones proposed above, the 'seamless' loading of all the institutional pipelines is a large selling point of nf-core - many people don't have to do any special set up or configuration to get the pipepline to run - many don't even install nf-core to run nf-core pipelines (or know of it's existance!).

My only thought currently is whether we can somehow load the configs elsewhere in the pipeline setup subworkflow... although I guess this may be problematic when it comes to config ordering/priority?

Or we just remove the whole try/catch block as Ben mentioned... maybe the default nextflow error message without it is simple enough?

mashehu commented 3 weeks ago

oh, missed that discussion. Can you link to the slack thread please?

jfy133 commented 3 weeks ago

You have to go to the super secret thing, will ping you on slack about it

jfy133 commented 3 weeks ago

OK, I don't think our custom error message for the catching is much more informative than the nextflow default one:

Running command: nextflow run ../main.nf -profile test_nothing,docker --outdir ./results --custom_config_base 'https://mastodon.social' (on local copy nf-core/mag)

with current template:

// Load nf-core custom profiles from different Institutions 
 try { 
     includeConfig "${params.custom_config_base}/nfcore_custom.config" 
 } catch (Exception e) { 
     System.err.println("WARNING: Could not load nf-core/config profiles: ${params.custom_config_base}/nfcore_custom.config") 
 } 

gives

 WARNING: Could not load nf-core/config profiles: https://mastodon.social/nfcore_custom.config

And simply

 // Load nf-core custom profiles from different Institutions
includeConfig "${params.custom_config_base}/nfcore_custom.config"

// Load nf-core/mag custom profiles from different institutions.
includeConfig "${params.custom_config_base}/pipeline/mag.config"

Gives

ERROR ~ No such file or directory: Config file does not exist: https://mastodon.social/nfcore_custom.config

I don't think it's much more informative, so I think it would be fine to drop the try/catch from the template, and then also add a entry to the nf-core troubleshooting page.

What do you think @maxulysse ?

jfy133 commented 3 weeks ago

And @mashehu @ewels

ewels commented 3 weeks ago

@jfy133 does Nextflow keep running without the try catch? I thought that it stopped without that. Meaning that it won't run offline.

jfy133 commented 3 weeks ago

Yes that's true. However if running offline we could include a copy of the configs repo, and that could be passed to custom_config_base?

If you're running offline anyway you have to do a bunch of extra hoops generally, so adding one more step wouldn't be so bad.

bentsherman commented 3 weeks ago

Renamed the issue to be more precise.

I think the main reason for the try-catch was to not fail the pipeline if the configs couldn't be loaded, because some users might not need it and so it shouldn't be a blocker for them. But making the remote config "optional" is not the right way to do this, because now for people who are using the remote config, the try-catch is making the pipeline proceed with an invalid run when really it should fail.

The ideal approach would be to inject these configs at the environment level instead of pipeline level, but I understand that this will likely always be less convenient. So simply removing the try-catch might be the best we can do.

bentsherman commented 2 weeks ago

Discussed with Phil, a few more points:

The include source can be any expression so you should be able to implement a hacky sort of optional include with a ternary:

includeConfig params.custom_config_base ? "${params.custom_config_base}/nfcore_custom.config" : '/dev/null'

Also, the new config parser, when it is added to Nextflow itself, will be opt-in, so it won't break everyone's pipelines immediately, only if they want to use the language server and/or new parser.

ewels commented 2 weeks ago

Nearly, we don't care about the base being set - it's the offline status. So this:

includeConfig !System.getenv('NXF_OFFLINE') ? "${params.custom_config_base}/nfcore_custom.config" : "/dev/null"
bentsherman commented 2 weeks ago

Sure, but conditioning on params.custom_config_base would allow anyone to disable the remote configs by setting params.custom_config_base = null. That would cover the issue of transient errors annoying people who don't use the remote configs

ewels commented 2 weeks ago

You mean the use case of running online but not wanting remote config profiles? Yeah fair enough. So how about this?

includeConfig !System.getenv('NXF_OFFLINE') && params.custom_config_base ? "${params.custom_config_base}/nfcore_custom.config" : "/dev/null"

I quite like the $NXF_OFFLINE thing as hopefully offline users are already specifying this so no change is needed from them.

bentsherman commented 2 weeks ago

Agreed 👍