From 8ad2164b1b5acc937bfb67d27ffe0578380af7dc Mon Sep 17 00:00:00 2001 From: Bill Hlavacek Date: Fri, 21 Aug 2026 13:36:22 -0600 Subject: [PATCH] test(cluster): check the cluster commands against the programs actually installed (#619) The tests for starting a cluster compared the command PyBNF builds 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 one of those tests kept passing while every real multi-machine run died on FileNotFoundError before a single simulation -- and because the outdated name sat in the test file as the expected answer, correcting PyBNF would have read as a test failure rather than as the fix. Substituting the outside world is the right way to test PyBNF's own logic, and none of those tests are taken away. What was missing beside them is a small number of checks that ask the installed programs themselves. Each new check takes its command from the code that builds it for a real fit -- `setup_cluster`, `srun_worker_command`, and the newly named `Cluster.dask_scheduler_command`, extracted so the scheduler command can be read without starting a cluster. No argument list is written down a second time, and locating `cluster.DASK_CLI` inside each command is itself the check that every worker-launch command goes through the single place that decides how dask is invoked. The checks then confirm that this interpreter's dask command line interface runs, that it still has the `ssh`, `scheduler` and `worker` subcommands, and that each subcommand's `--help` still declares every option PyBNF passes it, so a renamed *option* fails as loudly as a renamed command -- `--nworkers` is itself a survivor of that, having been `--nprocs` until distributed removed the old name. `dask ssh` is checked the stricter way: the whole command PyBNF builds is handed to dask with `--help` appended, so dask does the parsing and refuses an unknown option with a non-zero exit. `scheduler` and `worker` cannot be asked that way, since they forward unrecognized arguments to preload modules rather than refusing them; for those the help screen is the witness, read from the option column alone because `dask worker --help` names `--nworkers` in the prose describing three other options. The same questions are asked of SLURM's own programs, and skipped wherever SLURM is absent -- every developer machine and every CI runner. That does not make them dead weight: PyBNF's tests are run on clusters, which is the one place a renamed `srun` option can be caught before a fit walks into it, and #619 is about the whole class of outside programs rather than about dask alone. `srun`'s help is not laid out by click and its layout cannot be checked from here, so its options are matched as whole words -- weaker, but still fatal to an option whose name has left the screen entirely. Every check was verified by injecting the failure it exists to catch. Renaming `--nworkers` back to `--nprocs` fails three of them, with dask's own "No such option '--nprocs'" among the messages. Restoring the `dask-ssh` spelling fails seventeen, where before it failed none. A renamed `srun` option and an absent `scontrol`, against a stand-in SLURM, each fail naming what is wrong. The dask invocation now appears in the test file exactly once, in `DASK`, and is compared against `cluster.DASK_CLI` in a single test. It still pins the real argv rather than agreeing with the module by construction, but a rename no longer has to be made in seven places. --- CHANGELOG.md | 19 ++++ pybnf/cluster.py | 18 ++- tests/test_cluster.py | 250 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 276 insertions(+), 11 deletions(-) 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'(?