Verdi: Load config / profile lazily to speed up tab-completion #6140
Conversation
|
@sphuber can you take a quick look to see if I am on the right track here? (no rush) |
There was a problem hiding this comment.
So far nobody replied on the click repo so we'll have to deal with autocompletion ourselved. But I'll do that in a separate PR so that we can focus here on lazy config loading. So this is ready for review @sphuber
| profile_uuid = str(uuid.uuid4) | ||
| options = [ | ||
| '--non-interactive', '--profile', profile_name, '--profile-uuid', profile_uuid, '--db-backend', | ||
| self.storage_backend_name, '--db-name', db_name, '--db-username', db_user, '--db-password', db_pass, | ||
| '--db-port', self.pg_test.dsn['port'] | ||
| '--db-port', self.pg_test.dsn['port'], '--email', user_email |
There was a problem hiding this comment.
This test started failing with the changes introduced here. I am unsure if that means that I broke something, or whether the test worked by accident.
There was a problem hiding this comment.
This actually looks like a regression because you made the default for this option lazy with a functools partial. I don't think the partial is properly called, so the value that is passed for the option is the partial and not the actual config option value
There was a problem hiding this comment.
Okay, after going through the guts of click and its modification in aiida, I've determined that the default value seems to be evaluating correctly, i.e. the callback is called, but it returns an empty tuple. I think the test worked previously because the default was evaluated before the actual test, and hence the get_config('autofill.user.email') was called in the local context, not in test context, where this value seems to be unavailable.
@sphuber could you maybe try if you can make the test fail locally on main branch in a fresh aiida install without any profile? That would I think confirm my hypothesis.
There was a problem hiding this comment.
You are right. The problem is that the cmd_setup module is imported at the top of the test file. This will evaluate the setup command and with it the defaults. So even if I add the empty_config fixture to the test_setup_profile_uuid test, by the time that fixture gets invoked to provide an empty config for the test, the defaults of the setup command will already have been set. So by making the defaults lazily evaluated, this would indeed explain the observed behavior.
danielhollas
changed the title
The `VerdiContext` class, which provides the custom context of the `verdi` commands, loads the configuration. This has a non-negligible cost and so slows down the responsiveness of the CLI. This is especially noticeable during tab-completion. The `obj` custom object of the `VerdiContext` is replaced with a subclass of `AttributeDict` that lazily populates the `config` key when it is called with the loaded `Config` class. In addition, the defaults of some options of the `verdi setup` command, which load a value from the config and so require the config, are turned into partials such that they also are lazily evaluated. These changes should give a reduction in load time of `verdi` of the order of ~50 ms. A test of `verdi setup` had to be updated to explicitly provide a value for the email. This is because now the default is evaluated lazily, i.e. when the command is actually called in the test. At this point, there is no value for this config option and so the default is empty. Before, the default would be evaluated as soon as `aiida.cmdline.commands.cmd_setup` was imported, at which point an existing config would still contain these values, binding them to the default, even if the config would be reset afterwards before the test.
As discussed in #6117 (comment), we want to avoid loading the config file in verdi CLI until it is really needed. This is especially important for speeding up tab-completion.
There are several changes here:
VerdiContexthas only been introduced recently in CLI: allow setting options for config without profiles #5544, previously the config was loaded lazily as well it seems.ctx.resilient_parsing. EDIT: Postponed for a separate PR.For point 3 I created an issue on the click repo, let's see what they say pallets/click#2614
Benchmark after implementing points 1. and 2.
verdi --version(main)verdi --version(this PR)