test(cluster): check the cluster commands against the programs actually installed (#619) - #633
Merged
wshlavacek merged 1 commit intoAug 21, 2026
Conversation
…ly 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.
wshlavacek
force-pushed
the
test/619-check-external-programs-against-reality
branch
from
August 21, 2026 19:50
ec18ff0 to
8ad2164
Compare
wshlavacek
deleted the
test/619-check-external-programs-against-reality
branch
August 21, 2026 20:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #619.
The problem
The tests for starting a cluster checked that PyBNF built a particular command, comparing it 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 onFileNotFoundErrorbefore a single simulation. Worse, the outdated name sat in the test file as the expected answer, so correcting PyBNF would have read as a test failure rather than as the fix.Some of #619 was already addressed by #631, which introduced
cluster.DASK_CLIandcheck_dask_subcommand. What remained is the part the issue's reproduction is really about: nothing runs the program, and nothing checks the options at all.What this adds
Two test classes that ask the installed programs themselves, alongside (not instead of) the existing mocked tests.
Each check takes its command from the code that builds it for a real fit —
setup_cluster,srun_worker_command, and the newly namedCluster.dask_scheduler_command— so no argument list is written down a second time. Locatingcluster.DASK_CLIinside each command is itself the check that every worker-launch command goes through the one place that decides how dask is invoked.The checks then confirm that this interpreter's dask CLI runs, that it still has the
ssh,schedulerandworkersubcommands, and that each subcommand's--helpstill declares every option PyBNF passes — so a renamed option fails as loudly as a renamed command.--nworkersis itself a survivor of exactly that, having been--nprocsuntil distributed removed the old name.dask sshgets the stricter treatment: the whole command PyBNF builds is handed to dask with--helpappended, so dask does the parsing and refuses an unknown option with a non-zero exit.schedulerandworkercannot be asked that way — they forward unrecognized arguments to preload modules rather than refusing them — so for those the help screen is the witness, read from the option column alone (becausedask worker --helpnames--nworkersin the prose describing three other options).The same questions are asked of
srunandscontrol, skipped wherever SLURM is absent. That does not make them dead weight: PyBNF's tests are run on clusters, which is the one place a renamedsrunoption can be caught before a fit walks into it —--cpus-per-taskin particular is load-bearing, since without it a task confines every worker it forks to one CPU.srun's help is not laid out by click and its layout cannot be checked from a developer machine, so its options are matched as whole words: weaker, but still fatal to an option whose name has left the screen entirely.Verification
Every check was confirmed to go red by injecting the failure it exists to catch:
--nworkers→--nprocsNo such option '--nprocs'DASK_CLI→['dask-ssh'](the exact #615 regression)srun's--cpus-per-taskrenamed (stand-in SLURM)scontrolabsent on a SLURM systemThe whole-word matcher was also checked not to confuse
--ntaskswith--ntasks-per-node.On the single named value
#619 asked that the command name live in one place instead of seven.
cluster.DASK_CLIalready existed; the test file now spells the invocation out exactly once, inDASK, and compares it againstcluster.DASK_CLIin a single test.The remaining full-argv oracles build on that one constant rather than repeating the literal. They are deliberately not changed to import
DASK_CLIwholesale: a mocked test that assertsargv == cluster.DASK_CLI + [...]agrees with the module by construction and can no longer pin anything. The reality checks above are what make that spelled-out copy safe to keep.Test plan
tests/test_cluster.py: 132 passed, 2 skipped (the SLURM class, correctly skipping with nosrunpresent).ruff@0.15.14 check .clean;sphinx-build -W --keep-goingsucceeds.