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

@@ -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)}'

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 <https://aiidateam.github.io/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``.

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