Skip to content
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

Refactor verdi config #4584

Closed
chrisjsewell opened this issue Nov 21, 2020 · 11 comments · Fixed by #4712
Closed

Refactor verdi config #4584

chrisjsewell opened this issue Nov 21, 2020 · 11 comments · Fixed by #4712

Comments

@chrisjsewell
Copy link
Member

chrisjsewell commented Nov 21, 2020

I'm grouping here #4260 and #4480 (also linked to #4240):

verdi config currently has limited functionality, since it is a terminal/leaf command, rather than a command group, and also because it relies on tab completion for listing available options.

It should be turned it to a command group with at minimal list, show, set, unset commands.

Proposals:

$ verdi config list -h

Usage: verdi config list [OPTIONS]

  List AiiDA options for the current profile.

Options:
  --default/--no-default    Include default options that have not been overriden (prefixed **)
  --global/--no-global      Include global options that have not been overriden (prefixed *)
  -v/--verbose              Include the option help string
  -h, --help  Show this message and exit.
$ verdi config list --default --global --verbose
** some.default.option   value   description...
*  some.global.option    value   description...
   some.profile.option   value   description...
$ verdi config show -h

Usage: verdi config show [OPTIONS] [CONFIG_NAME]

  Show an AiiDA options for the current profile.

Options:
  -h, --help  Show this message and exit.
$ verdi config show some.option

description: This is the variable description ...
valid_type: int
valid_values: None
global_only: False
values:
  default: value
  global:  value
  profile: value
$ verdi config set -h

Usage: verdi config set [OPTIONS] CONFIG_NAME CONFIG_VALUE

  Set an AiiDA option.

Options:
  --force       Override existing without confirmation.
  --global      Set the option configuration wide.
  -h, --help  Show this message and exit.
$ verdi config unset -h

Usage: verdi config unset [OPTIONS] [CONFIG_NAMES]...

  Unset an AiiDA option.

Options:
  --force       Unset without confirmation.
  --global      Unset the option configuration wide.
  -h, --help  Show this message and exit.

thoughts?

@chrisjsewell
Copy link
Member Author

Note deprecation could be a bit of a pain.
Do we need to deprecate first and, if so, perhaps it would be easiest to move to a new name, e.g. verdi configure or verdi settings

@giovannipizzi
Copy link
Member

Thanks, I personally like the idea.
Note that you have verdi config unset that says it's instead a remove command in the doctoring.

In terms of deprecation: luckily, we don't have any config option called set, list, ..
How complex would it be, at the verdi config level, to get any other subcommand that is not explicitly defined (set, list, ...) and check if it's a known option, and in this case forward the call to set after issuing a deprecation warning (both a python warning and the warning printout that we use for the verdi cli)?

The list of options is, according to tab completion on my virtualenv:

daemon.default_workers       logging.plumpy_loglevel
daemon.timeout               logging.sqlalchemy_loglevel
daemon.worker_process_slots  logging.tornado_loglevel
db.batch_size                runner.poll.interval
logging.aiida_loglevel       user.email
logging.alembic_loglevel     user.first_name
logging.circus_loglevel      user.institution
logging.db_loglevel          user.last_name
logging.kiwipy_loglevel      verdi.shell.auto_import
logging.paramiko_loglevel    warnings.showdeprecations

In the worst case, if it's not possible to do it dynamically, we could have a small module that just defines these 20 subcommands explicitly, that we then simply delete in AiiDA v2.0?

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Nov 22, 2020

Note that you have verdi config unset that says it's instead a remove command in the doctoring.

fixed 👍

to get any other subcommand that is not explicitly defined (set, list, ...) and check if it's a known option

Well I guess we could add this deprecation logic (for that list of keys) in some way to the code already in place to suggest closest matches for non-existent commands. Just a bit of a pain in the backside lol

@chrisjsewell chrisjsewell self-assigned this Nov 22, 2020
@chrisjsewell
Copy link
Member Author

Ok well unless there are any objections, I'll assign myself to make a PR sometime soon(ish)

@chrisjsewell
Copy link
Member Author

Also on a related note, I'll just link this thought about the validation of options here: #4583 (comment)

@sphuber
Copy link
Contributor

sphuber commented Nov 23, 2020

Let me note once more that I don't think the configuration of the caching can be included in this command. All the settings of verdi config are simple key value assignments, but for caching, we need a different interface because you need to add/remove from enabled/disabled lists.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Nov 23, 2020

because you need to add/remove from lists

Off the top of my head

$ verdi config set --add key list_value1 list_value2

$ verdi config unset --pop key list_value1 list_value2

@sphuber
Copy link
Contributor

sphuber commented Nov 23, 2020

Sure, you could make it work, but don't think that is the best solution. For one, I don't think unset --pop is very intuitive and second, it doesn't allow to do a bunch of other commands that one might want for the caching command. I would imagine something like verdi caching validate to check that the config is correct and prints the resolved rules (since it can still be edited by hand). As well as verdi caching check ENTRY_POINT to see if an entry point would be cached given the current rules. Just because it may at some point be stored in the config file (and it currently isn't) I don't think it should necessarily be shoe-horned into verdi config at all cost

@chrisjsewell
Copy link
Member Author

validate would also be a config command, to validate the entire configuration, caching doesn't need to be special cased there.

This proposal does not anyhow inhibit there being a later cache command, it does though make it less essential.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Nov 23, 2020

A note on the configuration file validation: basically I think it should work very much like VS Code's settings.json https://code.visualstudio.com/docs/getstarted/settings, such that it is validated via a JSON schema
(which could also be interchangeably a yaml file)

@chrisjsewell chrisjsewell added this to the v1.6.0 milestone Jan 27, 2021
@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jan 27, 2021

I have "tentatively" added this to the v1.6.0, because it involves a deprecation. But obviously there are more pressing issues to close first, so we shall see if there is time to squeeze it in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants