From 062a5826077907f43165084d4dc02db3f71bfb73 Mon Sep 17 00:00:00 2001 From: Sebastiaan Huber Date: Wed, 24 May 2023 19:23:48 +0200 Subject: [PATCH] `verdi computer test`: Improve messaging of login shell check (#6026) The recently added check for the effect of using a login shell for the transport of the computer is improved: * The timings are now compared with a relative tolerance as well. Only if the timings differ by a factor of two is a warning printed. * The transport type in the suggested command to change the setting was hardcoded to `core.local`. It is now taken from the computer. This also allows to actually put dynamically use the correct computer label. Now the command can be literally copy-pasted. Note that this required adding the `computer` argument to all test functions. * If the timings are not close, instead of failing the test, it simply prints a warning. This is less alarming since really nothing is really broken. --- aiida/cmdline/commands/cmd_computer.py | 33 +++++++++++++++---------- docs/source/topics/transport.rst | 18 ++++++++++++++ tests/cmdline/commands/test_computer.py | 2 +- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/aiida/cmdline/commands/cmd_computer.py b/aiida/cmdline/commands/cmd_computer.py index cbe624010c..182d8ecc52 100644 --- a/aiida/cmdline/commands/cmd_computer.py +++ b/aiida/cmdline/commands/cmd_computer.py @@ -11,6 +11,7 @@ """`verdi computer` command.""" from copy import deepcopy from functools import partial +from math import isclose import click import tabulate @@ -46,7 +47,7 @@ def prompt_for_computer_configuration(computer): # pylint: disable=unused-argum pass -def _computer_test_get_jobs(transport, scheduler, authinfo): # pylint: disable=unused-argument +def _computer_test_get_jobs(transport, scheduler, authinfo, computer): # pylint: disable=unused-argument """Internal test to check if it is possible to check the queue state. :param transport: an open transport @@ -58,7 +59,7 @@ def _computer_test_get_jobs(transport, scheduler, authinfo): # pylint: disable= return True, f'{len(found_jobs)} jobs found in the queue' -def _computer_test_no_unexpected_output(transport, scheduler, authinfo): # pylint: disable=unused-argument +def _computer_test_no_unexpected_output(transport, scheduler, authinfo, computer): # pylint: disable=unused-argument """Test that there is no unexpected output from the connection. This can happen if e.g. there is some spurious command in the @@ -93,7 +94,7 @@ def _computer_test_no_unexpected_output(transport, scheduler, authinfo): # pyli return True, None -def _computer_get_remote_username(transport, scheduler, authinfo): # pylint: disable=unused-argument +def _computer_get_remote_username(transport, scheduler, authinfo, computer): # pylint: disable=unused-argument """Internal test to check if it is possible to determine the username on the remote. :param transport: an open transport @@ -105,7 +106,7 @@ def _computer_get_remote_username(transport, scheduler, authinfo): # pylint: di return True, remote_user -def _computer_create_temp_file(transport, scheduler, authinfo): # pylint: disable=unused-argument +def _computer_create_temp_file(transport, scheduler, authinfo, computer): # pylint: disable=unused-argument """ Internal test to check if it is possible to create a temporary file and then delete it in the work directory @@ -187,7 +188,7 @@ def time_use_login_shell(authinfo, auth_params, use_login_shell: bool, iteration return sum(timings) / iterations -def _computer_use_login_shell_performance(transport, scheduler, authinfo): # pylint: disable=unused-argument +def _computer_use_login_shell_performance(transport, scheduler, authinfo, computer): # pylint: disable=unused-argument """Execute a command over the transport with and without the ``use_login_shell`` option enabled. By default, AiiDA uses a login shell when connecting to a computer in order to operate in the same environment as a @@ -197,7 +198,8 @@ def _computer_use_login_shell_performance(transport, scheduler, authinfo): # py by at least 100 ms. If the computer is already configured to avoid using a login shell, the test is skipped and the function returns a successful test result. """ - tolerance = 0.1 # 100 ms + rel_tol = 0.5 # Factor of two + abs_tol = 0.1 # 100 ms iterations = 3 auth_params = authinfo.get_auth_params() @@ -216,13 +218,16 @@ def _computer_use_login_shell_performance(transport, scheduler, authinfo): # py echo.echo_debug(f'Execution time: {timing_true} vs {timing_false} for login shell and normal, respectively') - if timing_true - timing_false > tolerance: - message = ( - 'computer is configured to use a login shell, which is slower compared to a normal shell (Command execution' - f' time of {timing_true} s versus {timing_false}, respectively).\nUnless this setting is really necessary, ' - 'consider disabling it with: verdi computer configure core.local COMPUTER_NAME -n --no-use-login-shell' + if not isclose(timing_true, timing_false, rel_tol=rel_tol, abs_tol=abs_tol): + return True, ( + f"\n\n{click.style('Warning:', fg='yellow', bold=True)} " + 'The computer is configured to use a login shell, which is slower compared to a normal shell.\n' + f'Command execution time of {timing_true:.3f} versus {timing_false:.3f} seconds, respectively).\n' + 'Unless this setting is really necessary, consider disabling it with:\n' + f'\n verdi computer configure {computer.transport_type} {computer.label} -n --no-use-login-shell\n\n' + 'For details, please refer to the documentation: ' + 'https://aiida.readthedocs.io/projects/aiida-core/en/latest/topics/transport.html#login-shells\n' ) - return False, message return True, None @@ -551,7 +556,9 @@ def computer_test(user, print_traceback, computer): echo.echo(f'* {test_label}... ', nl=False) num_tests += 1 try: - success, message = test(transport=transport, scheduler=scheduler, authinfo=authinfo) + success, message = test( + transport=transport, scheduler=scheduler, authinfo=authinfo, computer=computer + ) except Exception as exception: # pylint:disable=broad-except success = False message = f'{exception.__class__.__name__}: {str(exception)}' diff --git a/docs/source/topics/transport.rst b/docs/source/topics/transport.rst index 50bd62c2f7..45706b94a7 100644 --- a/docs/source/topics/transport.rst +++ b/docs/source/topics/transport.rst @@ -57,3 +57,21 @@ It contains the interface with all the methods that need to be implemented, incl To inform AiiDA about your new transport plugin you must register an entry point in the ``aiida.transports`` entry point group. Please visit the `AiiDA registry `_ to see an example of how this can be done. + + +.. _topics:transport:login-shells: + +Login shells +------------ + +The base transport class :class:`aiida.transports.transport.Transport` defines the ``use_login_shell`` option. +When set to ``True``, all commands executed over the transport will use the ``-l/--login`` option of ``bash``. +This instructs bash to load a login shell, which, according to the ``bash`` manpage, means: + + When bash is invoked as an interactive login shell, or as a non-interactive shell with the ``--login`` option, it first reads and executes commands from the file ``/etc/profile``, if that file exists. + After reading that file, it looks for ``~/.bash_profile``, ``~/.bash_login``, and ``~/.profile``, in that order, and reads and executes commands from the first one that exists and is readable. + +By default ``use_login_shell`` is set to ``True`` as it ensures that the commands executed over the transport by AiiDA see the same shell environment as the user, if they were to login manually and execute the command. +However, in certain cases, the login scripts might not be necessary for AiiDA to properly run codes on the target computer. +At the same time, it is possible that the login scripts have a non-negligible run time, and so can significantly slow down all the commands AiiDA has to execute. +In this case, it may be useful to set the ``use_login_shell`` to ``False``. diff --git a/tests/cmdline/commands/test_computer.py b/tests/cmdline/commands/test_computer.py index 99a911dfb6..6ec2bec645 100644 --- a/tests/cmdline/commands/test_computer.py +++ b/tests/cmdline/commands/test_computer.py @@ -796,5 +796,5 @@ def time_use_login_shell(authinfo, auth_params, use_login_shell, iterations) -> monkeypatch.setattr(cmd_computer, 'time_use_login_shell', time_use_login_shell) result = run_cli_command(computer_test, [aiida_localhost.label], use_subprocess=False) - assert 'Warning: 1 out of 6 tests failed' in result.output + assert 'Success: all 6 tests succeeded' in result.output assert 'computer is configured to use a login shell, which is slower compared to a normal shell' in result.output