diff --git a/CHANGELOG.md b/CHANGELOG.md index efffcbf3..f02e58a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -198,6 +198,25 @@ All notable changes to PyBNF are documented below. This project adheres to says what it has not bisected. ### Fixed +- **The cluster tests now notice when an outside program is renamed (#619).** The tests for + starting a cluster checked that PyBNF built a particular command, against a copy of that + command written into the test file. Nothing checked that the command could be run. When + distributed stopped installing `dask-ssh` (#615), every test kept passing while every real + multi-machine run died on `FileNotFoundError` — and because the outdated name was written + in as the expected answer, *correcting* PyBNF would have read as a test failure. + A handful of checks now ask the installed programs themselves. They take each command from + the code that builds it for a real fit — no argument list is written down a second time — + and confirm that the dask command line interface PyBNF invokes runs, that it still has the + `ssh`, `scheduler` and `worker` subcommands, and that its `--help` still declares every + option PyBNF passes it, so a renamed *option* fails as loudly as a renamed command. The + `ssh` command is checked the stricter way, by handing dask the whole command PyBNF builds + and letting dask parse it. The same questions are asked of `srun` and `scontrol`, skipped + wherever SLURM is not installed — which is every developer machine, but not the clusters + where PyBNF's tests are also run, and where a renamed `srun` option is worth catching + before a fit walks into it. Nothing was taken away: the existing tests go on pinning the + argument lists PyBNF is supposed to build, and the one copy of the dask invocation they + keep is now compared against `cluster.DASK_CLI`, so it cannot drift away from the original + unnoticed either. - **A multi-machine fit sizes its worker pool by what the job was granted, not by how big the machine is (#616).** PyBNF decided how many worker processes to start on each node by calling `multiprocessing.cpu_count()`, which reports every processor the machine has whatever the job diff --git a/pybnf/cluster.py b/pybnf/cluster.py index 6529974b..af0f74aa 100644 --- a/pybnf/cluster.py +++ b/pybnf/cluster.py @@ -503,6 +503,22 @@ def cpus_per_node(): '(dask.system.CPU_COUNT)') return cpu_count(), "this machine's whole processor count (multiprocessing.cpu_count)" + @staticmethod + def dask_scheduler_command(scheduler_file): + """ + Build the ``dask scheduler`` invocation that starts the scheduler on this node. + + A one-line command, named anyway so that it sits beside + :meth:`srun_worker_command` and can be read -- and checked against the dask that + is actually installed (#619) -- without starting a cluster to see it. + + :param scheduler_file: Path the scheduler should write its connection information to + :type scheduler_file: str + :return: the dask scheduler argument list + :rtype: list + """ + return [*DASK_CLI, 'scheduler', '--scheduler-file', scheduler_file] + @staticmethod def srun_worker_command(scheduler_file, node_count, parallel_count=None): """ @@ -594,7 +610,7 @@ def setup_srun_cluster(scheduler_file, out_dir, node_count, parallel_count=None) check_dask_subcommand('scheduler') check_dask_subcommand('worker') scheduler_log = os.path.join(out_dir, SRUN_SCHEDULER_LOG) - scheduler_cmd = [*DASK_CLI, 'scheduler', '--scheduler-file', scheduler_file] + scheduler_cmd = Cluster.dask_scheduler_command(scheduler_file) logger.info('Starting the dask scheduler on this node, logging to %s' % scheduler_log) scheduler_proc = Cluster.popen_logged(scheduler_cmd, scheduler_log) try: diff --git a/tests/test_cluster.py b/tests/test_cluster.py index ba97b200..adc72ccf 100644 --- a/tests/test_cluster.py +++ b/tests/test_cluster.py @@ -31,6 +31,14 @@ ``Cluster.setup_cluster`` — **fakes** recording their call args, so the ``__init__`` branch dispatch is asserted without a real dask cluster. +Deliberately *not* substituted (#619): one section asks the programs that are +actually installed whether they still exist and still take the options PyBNF +passes them. Substituting the outside world is right for everything else here, +but it is also why nothing here could notice #615 -- an outside program renamed +out from under PyBNF, every multi-machine run dead, and every test still green. +Those checks build their commands by calling the real builders, so no argv is +written down twice, and they skip where the program is not installed. + #393 note: these assert PyBNF's *own* command-string / branch logic, never dask/distributed internals or a pinned dask version (the version-specific ``reinit_logging`` workaround is asserted *to be called*, not pinned), so they @@ -38,12 +46,16 @@ """ import json import os +import re import sys import types import pytest -from subprocess import TimeoutExpired, CalledProcessError +from functools import lru_cache +from importlib.util import find_spec +from shutil import which +from subprocess import run, PIPE, STDOUT, TimeoutExpired, CalledProcessError from .context import cluster, printing @@ -256,14 +268,233 @@ def test_the_group_queried_is_the_one_dask_itself_reads(self, monkeypatch): # --------------------------------------------------------------------------- # -# setup_cluster — the dask ssh command string + per-node arithmetic +# The programs PyBNF runs, against the programs that are actually installed +# (#619) # --------------------------------------------------------------------------- # -# What PyBNF prepends to every worker-launch command (#615). Spelled out here -# rather than imported from cluster.DASK_CLI so that the tests below pin the -# actual argv, not merely agree with whatever the module happens to build. -DASK_SSH = [sys.executable, '-m', 'dask', 'ssh'] +# Every other test in this file substitutes the outside world. That is right -- +# they are about PyBNF's own branch and command-building logic -- but it is also +# why none of them could notice #615, where distributed stopped installing +# ``dask-ssh``, every multi-machine run died on FileNotFoundError before a single +# simulation, and every test here kept passing: the name they compared against was +# a copy of the wrong name, written into this file. +# +# These checks close that gap by asking the installed programs themselves. Each +# one takes a command from the code that builds it for a real fit -- no argv is +# written down a second time here -- and asks the program named in it whether it +# runs, whether it still has that subcommand, and whether its ``--help`` still +# declares every option PyBNF passes. A renamed command or a renamed option fails +# here, loudly, while the mocked tests below go on pinning the argv PyBNF is +# supposed to build. + + +# What PyBNF prepends to every command it hands to dask (#615). Written out once, +# rather than imported from cluster.DASK_CLI, so that the mocked tests below pin +# the actual argv instead of agreeing with the module by construction -- and +# written out only *once*, because #619 is in part about a name that had to be +# corrected in seven places. The copy and the original are compared in +# test_the_invocation_these_tests_pin_is_the_modules_own, so the copy cannot drift +# away unnoticed either. +DASK = [sys.executable, '-m', 'dask'] +DASK_SSH = [*DASK, 'ssh'] + + +@lru_cache(maxsize=None) +def _help_text(argv): + """Run a program's help screen and return what it printed. + + ``argv`` is a tuple so that it can be a cache key: several checks read the + same screen, and each reading costs a process. + """ + proc = run([*argv, '--help'], stdout=PIPE, stderr=STDOUT, timeout=120) + output = proc.stdout.decode('UTF-8', errors='replace') + assert proc.returncode == 0, ('`%s --help` failed (exit %s), so PyBNF cannot run it ' + 'either:\n%s' % (' '.join(argv), proc.returncode, output)) + return output + + +_DECLARED_OPTION_RE = re.compile(r'^ {1,4}(-[^\s,]+(?:,\s+-[^\s,]+)*)') + + +def _declared_options(help_text): + """The option names a click ``--help`` screen declares. + + Read from the option column alone -- a declaration begins within the first + few columns of its line, while the description beside it wraps far deeper in. + The distinction earns its keep: ``dask worker --help`` names ``--nworkers`` + inside the prose describing three *other* options, so a search of the whole + screen would go on passing after the option itself was gone. + """ + declared = set() + for line in help_text.splitlines(): + match = _DECLARED_OPTION_RE.match(line) + if match: + declared.update(word.strip() for word in match.group(1).split(',')) + return declared + + +def _help_mentions(help_text, option): + """Whether a help screen names an option anywhere, as a whole word. + + Weaker than :func:`_declared_options`, and used only for a program whose help + is not laid out by click and whose layout cannot be checked from a developer + machine. It still catches the failure that matters: an option that no longer + exists, whose name has left the screen entirely. + """ + return re.search(r'(?