Closed ewels closed 2 years ago
@piotr-faba-ardigen - can we chat about this ?
Thanks for letting me know. I didn't test this scenario. I will check how I can modify this no to break this.
Just to provide some more clarity on this: We use this config to run all nf-core pipelines internally at BI, but don't want / can't disclose the URL to our own managed central config repository publicly.
This way worked quite nicely without breaking the way we import configs in nf-core, so we thought it would be fine, given that the config will just work from within here with someone having the ENV variable set appropriately. I wondered in the beginning whether it will break things in the central configs repository (which it doesn't - just provides a proper error if its not set), but seemingly there is a side effect on the function you posted above @ewels 😞
Yeah, I guess it happens whenever that config file is parsed. Usually that's only when loading the profile, but in this case we are trying to list all profiles and it gets executed.
Could you just tweak it to throw a warning instead of halting all execution with an exception? Or better still, if you have static hostnames, could you check that before running the function?
FYI, I verified one more thing. I run the command nextflow config -show-profiles
pointing at configs at this commit which is before my addition.
In this case another error from the prince.config
profile is thrown.
[faba@xxx sarek]$ nextflow config -show-profiles
Unknown config attribute `profiles.prince.SCRATCH` -- check config file: /home/faba/nf-core/sarek/nextflow.config
So there may be more fixes to do, before it works. Probably a good follow up would be to add nextflow config -show-profiles
command to CI as another test. Otherwise this may get broken again in the future.
The PRINCE config also has a custom variable set - so similar issue there and would need a fix too ;-)
I think your fix looks good - if you want to add a CI test yourself that would be cool, otherwise I can do it too.
Reopening this issue and renaming it, just to keep it here until nextflow config -show-profiles
works...
Note that when I run in the nf-core/configs
root directory I get an different to the profiles.prince.SCRATCH
one above:
$ nextflow config -show-profiles .
No such file: Config file does not exist: /Users/philewels/GitHub/nf-core/configs/[:]/conf/awsbatch.config
Any ideas?
Phil
It's because prince profile uses also a ENV VAR which if undefined breaks things. An if statement verifying if it's set should be added in it, like in bi.config
. I noticed that missing ENV VAR is often substituted with [:]
by Groovy/Nextflow.
I'm not quite sure that's right. It was a problem with the BI config because the code was throwing an Exception, which halted everything. Missing env vars throw a warning but that's it I think.
After playing a bit, I think it's because when I run nextflow config
in the configs directory, it uses the nextflow.config
which is used basically nowhere. With this file, params.cusom_config_base
is not set, so all of the includeConfig
statements fail when they try to load the profile config files. Adding this to nextflow.config
solves the problem.
I've made a PR in #165 to fix this.
Ok, so running nextflow config -show-profiles
in the configs root now gives me this:
That's all to STDOUT
. The log warning from the BI config comes to STDERR
as expected:
WARNING: For bi.config requires NXF_GLOBAL_CONFIG env var to be set. Point it to global.config file if you want to use this profile.
Buried at the bottom of that big config dump is the prince config SCRATCH
warning from Nextflow:
WARN: Unknown config attribute `profiles.prince.SCRATCH` -- check config file: /Users/philewels/GitHub/nf-core/configs/nextflow.config
Updated the Prince config to do the same thing as the BI config. Now both warnings come to STDERR. I also added CI checks for this. Should be good now I think!
Should be fixed now in #165
This code block breaks the functionality of
nextflow config -show-profiles
with all nf-core pipelines:https://github.com/nf-core/configs/blob/bb7f67e2b556e1deda34b51799d654603d9c8c9f/conf/bi.config#L10-L18
Gives:
I thought that this was a nextflow bug at first, see https://github.com/nextflow-io/nextflow/issues/1639
@apeltzer - what's going on in this code? Can we refactor it somehow?