FIX: Do not evaluate callable defaults during tab-completion

[danielhollas]

In #6140 we've tried to speed up verdi invocation by lazy loading config / profile. Unfortunately, the configuration is still being loaded during tab-completion.

After fair amount of going through the code in both click and aiida, I now think this is a bug in click, see pallets/click#2614. I've devised a fix that passes the click test suite so it seems that the current behavior is unintented.

I will submit a PR to click, but given the speedup that we stand to gain (~50ms) for such a time-sensitive thing as tab-completion, I think it is worth fixing here for the next aiida-core release, which I suspect will happen before the next click release.

I've verified that with this fix, the profile is indeed not being loaded during tab-completion, by stucking raise ValueError in the aiida.manage.configuration.get_config() and observing that it raises on main and does not raise here during tab completion.

It would be great to have a regression test for this, but I am not sure how to do it. Here's how click tests it: https://github.com/pallets/click/blob/main/tests/test_shell_completion.py

[danielhollas]

Unrelated fix

[danielhollas]

@sphuber this is ready for review

[sphuber]

Thanks @danielhollas . As for the test, maybe you could try something like the following:

# -*- coding: utf-8 -*-
###########################################################################
# Copyright (c), The AiiDA team. All rights reserved.                     #
# This file is part of the AiiDA code.                                    #
#                                                                         #
# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core #
# For further information on the license, see the LICENSE.txt file        #
# For further information please visit http://www.aiida.net               #
###########################################################################
# pylint: disable=redefined-outer-name
"""Tests for the :mod:`aiida.cmdline.params.options.callable` module."""
import pytest

from click.shell_completion import ShellComplete

from aiida.cmdline.commands.cmd_verdi import verdi


def _get_completions(cli, args, incomplete):
    comp = ShellComplete(cli, {}, cli.name, '_CLICK_COMPLETE')
    return comp.get_completions(args, incomplete)


@pytest.fixture
def unload_config():
    """Temporarily unload the config by setting ``aiida.manage.configuration.CONFIG`` to ``None``."""
    from aiida.manage import configuration

    config = configuration.CONFIG

    try:
        configuration.CONFIG = None
        yield
    finally:
        configuration.CONFIG = config


@pytest.mark.usefixtures('unload_config')
def test_callable_default_resilient_parsing():
    """Test that tab-completion of ``verdi`` does not evaluate defaults that load the config, which is expensive."""
    from aiida.manage import configuration

    assert configuration.CONFIG is None
    [c.value for c in _get_completions(verdi, [], '')]
    assert configuration.CONFIG is None

This fails for the main branch as it should. If it passes on your branch, I would say this provides some assurance it is working as intended

[sphuber]

Is this really the only option that has an expensive callable default?

[danielhollas]

It's the only place where the config/profile is loaded, all the other use the InteractiveOption where this is already handled. But you are right that there likely are other expensive defaults, but I plan to look into this in a followup PR where I will also look at the timings more closely.

[sphuber]

One more place that loaded the config

Seems like the test is doing its job 👍

[danielhollas]

Haha, indeed, thank you very much for the test! This is now ready from my side.

[danielhollas]

NOTE: I have removed the try-except block, I don't think it is necessary, pytest should ensure that the fixture is run to completion after the test, unless the fixture itself excepts before the yield point, but here we only have two assignments.

https://docs.pytest.org/en/latest/how-to/fixtures.html#teardown-cleanup-aka-fixture-finalization

https://docs.pytest.org/en/latest/how-to/fixtures.html#safe-teardowns

[danielhollas]

I noticed that we do not test autocompletion anywhere else in the test suite. I'll try adding more tests in a separate PR, for now I added at least this simple assert.
(also to shutup pylint which was complaining about unassigned expression)

[sphuber]

I am a bit surprised by this change. This function set_log_level is only assigned as the callback of the VERBOSITY option. I don't think this is supposed to be called during tab-completion anyway. I just tested this and it indeed doesn't seem to be called during tab-completion. Was this the part of the code that caused the new test to fail? Do you understand why?

[danielhollas]

Good catch, sorry for not being clear, I am also confused, but you can try when you remove it the test fails. But when I test the completion on the actual command line it is not called. Maybe the click function used in the test is not exactly the one that gets called?? Btw: I was testing on BASH, wonder if other shells may behave differently.

[danielhollas]

Perhaps there is some weird interaction with the test suite. Not sure if it is worth deeper investigation, since the change itself seems like an okay thing to do on its own.

[sphuber]

Figured it out. I tested whether this was being called when actually tab-completing verdi by adding a print statement. Since that print statement never showed up, I concluded that the function wasn't being called. But that is not true. It was actually called, but during tab-completion, all output to sys.stdout is captured and so I didn't see anything. Printing to sys.stderr would actually show, or simply raising an exception would confirm the function was being called.

[sphuber]

Thanks @danielhollas