-
Notifications
You must be signed in to change notification settings - Fork 191
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
Remove try-catch blocks around institutional configs #3132
Comments
Cc @jfy133 🤔 |
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 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? |
oh, missed that discussion. Can you link to the slack thread please? |
You have to go to the super secret thing, will ping you on slack about it |
OK, I don't think our custom error message for the catching is much more informative than the nextflow default one: Running command: with current template:
gives
And simply
Gives
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 does Nextflow keep running without the try catch? I thought that it stopped without that. Meaning that it won't run offline. |
Yes that's true. However if running offline we could include a copy of the configs repo, and that could be passed to 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. |
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. |
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. |
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" |
Sure, but conditioning on |
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 |
Agreed 👍 |
The nf-core pipeline template has these try-catch blocks to load institutional configs:
tools/nf_core/pipeline-template/nextflow.config
Lines 76 to 88 in 7785b4c
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:
Similarly for a pipeline-specific config:
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.
The text was updated successfully, but these errors were encountered: