verdi code setup --config ignores the yaml file

[ConradJohnston]

Preface: I know code setup is deprecated in favor of code create, but we should still document this bug for travelers from search engines.

Version: v2.2.1.post0

Expectation: verdi code setup --config foo.yml should read the contents of the yaml file and setup up the code without prompting the user.

Current situation: verdi code setup ignores the config file. verdi code create does the correct thing.

Proposed solution: verdi code create does work and code setup is deprecated, so don't fix and remove code setup as soon as sufficient notice of deprecation has passed.

[sphuber]

I think verdi code setup may still read the config file. The problem is just that the options differ from verdi code create and so the same YAML will not work for both. Keys that are not understood are simply ignored, and keys that are not explicitly defined by the YAML are prompted for. I think this is the behavior you are seeing. Is it ignoring all the keys in the config? You can test by adding label, which should be supported by both, and if you are not prompted for that by verdi code setup then it is reading it properly.

Not sure what we can do here. Since the deprecation was really recent, I don't think we will be able to remove it anytime soon.

[sphuber]

Was the analysis correct @ConradJohnston or is there another problem you think?

[ConradJohnston]

Hi @sphuber, I played with it a little bit more. I think you're right - it's selective in its hearing.
Perhaps the better thing to do for the older verdi code setup is to print a warning when an unknown key is encountered in the YAML. This way there is no confusion arising from the silent behaviour.

[sphuber]

Perhaps the better thing to do for the older verdi code setup is to print a warning when an unknown key is encountered in the YAML. This way there is no confusion arising from the silent behaviour.

The parsing of the config file is done by the click-config-file third-party package. It turns out @ltalirz already stumped his toe against the same rock in 2019 😄 phha/click_config_file#11

It seems the author looked into this, but the design of click makes it difficult to implement. He linked an issue that would have to be resolved first, but that is from 2016 and is still open, so not sure if we can have much hope here.

Some other guy responded though that his library click-extra does provide this functionality. Since development on click-config-file has died down and click-extra seems a lot more lively, we could consider looking into replacing the former with the latter.

There is a lot more functionality than we need (or even want, a lot of it is way too opinionated and we definitely don't want to adopt) but it seems possible to just use the config parsing component.

[ConradJohnston]

@sphuber If nothing else, click-extra is certainly prettier overall than click.

I see indeed that there is a strict option that does exactly what we want. In the interests of maintaining continuity of the existing functionality, it seems this is the only way forward. I'll have a play with it.

[ConradJohnston]

I tried the click-extra config file option.

I made some progress using it as a drop in replacement.
The bit that blocks me is when it checks the option types using: get_param_type()

It unfortunately seems to use a hard-coded set of types to check against and so chokes on the various custom types implemented in AiiDA.

[ltalirz]

What I like about click-config-file is that it is 165 lines of code, with configobj as the only dependency besides click
https://github.com/phha/click_config_file/blob/0d1940fc3dcd6e2c254798a33aee074c05a90de3/setup.py#L24

After adding some initial feature adds, the dependency has been "set and forget".

click-extra is quite a different beast in this respect https://github.com/kdeldycke/click-extra/blob/5759c140a8636e8e49281940ecde1fcb78323963/pyproject.toml#L70

I'm not saying that we should not switch to click-extra, just mentioning this as something to keep in mind.

[sphuber]

click-extra is quite a different beast in this respect https://github.com/kdeldycke/click-extra/blob/5759c140a8636e8e49281940ecde1fcb78323963/pyproject.toml#L70
I'm not saying that we should not switch to click-extra, just mentioning this as something to keep in mind.

I fully agree, it is adding a lot more complexity and potentially other breakages and headaches. We should really weigh the benefits/costs here and maybe conclude that we leave this as is, unless we can include it is click-config-file ourselves (or even absorb it entirely if getting new releases prove difficult, since it is not that much code after all).

[ConradJohnston]

@ltalirz and @sphuber - we're going around the houses a little.
In any case click-config-file is what we have, and the fix we want is apparently blocked due to a limitation in click.
So I think unless we overhaul how passing a config file is handled (probably in a way that is not backwards compatible) I agree with @sphuber that we probably don't fix this now.

What I'm thinking is that perhaps the config file is handled in a manual way by an additional sub-subcommand, that exploits all of the options being available in a non-interactive way. Something like a verdi code setup fromfile --file config.yml which is ultimately a command that reads and checks the yml (we do this) and then passes values through to a non-interactive call to verdi code setup. It's not pretty, but my preference is not for silent behaviours when working with these config files.

[sphuber]

I had a quick look at the code and couldn't really see why it wouldn't be possible to add the check we need. I made a quick mock-up in #5939 . At least for the following test example it works just fine. Take the following test command:

#!/usr/bin/env python
import click
from aiida.cmdline.params import options

@click.command('test')
@click.option('-a')
@click.option('-b')
@options.CONFIG_FILE()
def test_command(a, b):
    print(a, b)


if __name__ == '__main__':
    test_command()

and then call it with

./cli.py --config config.yml

where config.yml contains:

b: 2
c: 3

And the result is

(aiida_dev) sph@invader:~/code/aiida/env/dev/aiida-core$ ./cli.py --config config.yml 
Usage: cli.py [OPTIONS]
Try 'cli.py --help' for help.

Error: Invalid value for '--config': Invalid configuration file, the following keys are not supported: {'c'}

I haven't tested this with any existing verdi commands yet, but I think this should work.

Only downside is that the fix requires me to override configuration_callback of click_config_file and subsequently the main function click_config_file.configuration_option since the latter doesn't provide a hook to customize the partial callback.

[ConradJohnston]

Reading phha/click_config_file#11 again, I realise now that a particular edge case is being discussed, and the author wanted a complete solution, whereas we can make do.

I tried your branch @sphuber - delicious. Works well for my test cases so far.