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

FIX: Do not evaluate callable defaults during tab-completion #6144

Merged
merged 3 commits into from
Oct 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions aiida/cmdline/params/options/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# yapf: disable
# pylint: disable=wildcard-import

from .callable import *
from .config import *
from .main import *
from .multivalue import *
Expand All @@ -39,6 +40,7 @@
'COMPUTER',
'COMPUTERS',
'CONFIG_FILE',
'CallableDefaultOption',
'ConfigFileOption',
'DATA',
'DATUM',
Expand Down
34 changes: 34 additions & 0 deletions aiida/cmdline/params/options/callable.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# -*- 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 #
###########################################################################
"""
.. py:module::callable
:synopsis: A monkey-patched subclass of click.Option that does not evaluate callable default during tab completion
"""

import typing as t

import click

__all__ = ('CallableDefaultOption',)


class CallableDefaultOption(click.Option):
"""A monkeypatch for click.Option that does not evaluate default callbacks during tab completion

This is a temporary solution until a proper fix is implemented in click, see:
https://github.com/pallets/click/issues/2614
"""

def get_default(self, ctx: click.Context, call: bool = True) -> t.Optional[t.Union[t.Any, t.Callable[[], t.Any]]]:
"""provides the functionality of :meth:`click.Option.get_default`,
but ensures we do not evaluate callable defaults when in tab-completion context."""
if ctx.resilient_parsing:
return None
return super().get_default(ctx=ctx, call=call)
2 changes: 1 addition & 1 deletion aiida/cmdline/params/options/interactive.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def get_default(self, ctx: click.Context, call: bool = True) -> t.Optional[t.Uni
if self._contextual_default is not None:
default = self._contextual_default(ctx)
else:
default = super().get_default(ctx)
default = super().get_default(ctx, call=call)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated fix


try:
default = self.type.deconvert_default(default)
Expand Down
6 changes: 5 additions & 1 deletion aiida/cmdline/params/options/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from .. import types
from ...utils import defaults, echo # pylint: disable=no-name-in-module
from .callable import CallableDefaultOption
from .config import ConfigFileOption
from .multivalue import MultipleValueOption
from .overridable import OverridableOption
Expand Down Expand Up @@ -110,8 +111,10 @@ def set_log_level(_ctx, _param, value):
log.CLI_ACTIVE = True

# If the value is ``None``, it means the option was not specified, but we still configure logging for the CLI
# However, we skip this when we are in a tab-completion context.
if value is None:
configure_logging()
if not _ctx.resilient_parsing:
configure_logging()
Comment on lines +116 to +117
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return None

try:
Expand Down Expand Up @@ -145,6 +148,7 @@ def set_log_level(_ctx, _param, value):
'profile',
type=types.ProfileParamType(),
default=defaults.get_default_profile,
cls=CallableDefaultOption,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

help='Execute the command for this profile instead of the default profile.'
)

Expand Down
42 changes: 42 additions & 0 deletions tests/cmdline/params/options/test_callable.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# -*- 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."""
from click.shell_completion import ShellComplete
import pytest

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
configuration.CONFIG = None
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

yield
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
completions = [c.value for c in _get_completions(verdi, [], '')]
assert 'help' in completions
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

assert configuration.CONFIG is None