Replace the SSH launcher's fixed 10 second waits with readiness checks (#398) - #636
Merged
Conversation
added 2 commits
August 21, 2026 16:59
#398) The SSH cluster launcher used two fixed ten second sleeps. One came after starting dask ssh, on the assumption that the workers were up ten seconds later. The other came after asking the cluster to shut down, on the assumption that it had stopped ten seconds later. Both assumptions are wrong in both directions. Measured on a real SLURM cluster, the workers took 26 to 59 seconds to register, so ten seconds let the run start before the cluster was ready. On a fast cluster ten seconds is longer than needed and every run paid it twice. The newer srun launcher already replaced its waits with real polling. This does the same for the SSH launcher, reusing that pattern. Startup: setup_cluster no longer sleeps. It launches dask ssh and returns the process, the number of workers it should bring up, and the file its output was captured to. The constructor then calls a new wait_for_ssh_workers, which polls the scheduler until all of the expected workers register, up to a time limit. It watches the dask ssh process on every pass, so a failed login is reported as soon as dask ssh exits, quoting what it said, rather than after a fixed wait. Requiring the full worker count also turns a quietly undersized cluster into a clear error (the concern in #200). Teardown: the fixed sleep in pybnf.py is gone. Cluster teardown now asks each process it started to stop and waits until it has actually exited, and kills one that will not stop within a bounded time. Teardown returns as soon as the processes are really gone. The two time limits are named constants in pybnf/cluster.py (SSH_WORKER_TIMEOUT and TEARDOWN_TIMEOUT), matching how the srun launcher sets its limits, so there is one clear place to change them. Verification: the SSH startup check is covered by unit tests. The SSH login uses paramiko, which cannot authenticate on a host based or Kerberos cluster, so an end to end SSH startup run needs a cluster whose nodes accept a paramiko login. The teardown check is shared by both launchers and was confirmed on a real two node srun run: teardown returned in about one second once the processes exited, rather than after ten.
The startup check this change added for the SSH launcher sat only on the dask ssh path, which uses paramiko and cannot log in on a host based or Kerberos cluster. That left the new code unable to run on the cluster available here, so it was verified by unit tests alone. #398 asks for the readiness checks to be verified on a real multi machine cluster, and names the SSH login as the reason it could not be. Pull the polling itself into one method, _poll_for_workers, that both launchers call. It watches the scheduler for the workers to register and watches the process that is bringing them up, and returns an outcome rather than raising, so each launcher still phrases its own error. The srun wrapper keeps waiting for one worker and quotes srun's log; the SSH wrapper keeps waiting for the full expected count and quotes what dask ssh said. Their observable behavior is unchanged. Because the srun launcher needs no login, it runs on this cluster, so the shared loop is now exercised on a real two node run rather than only in unit tests. Confirmed: the workers registered about three seconds after srun started and the run began at once, and teardown returned in about one second once the processes exited.
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 #398.
The SSH cluster launcher used two fixed ten second sleeps. One came after starting dask ssh, assuming the workers were up ten seconds later. The other came after asking the cluster to shut down, assuming it had stopped ten seconds later. Both assumptions are wrong in both directions. Measured on a real SLURM cluster, the workers took 26 to 59 seconds to register, so ten seconds let the run start before the cluster was ready. On a fast cluster ten seconds is longer than necessary and every run paid it twice.
The srun launcher merged in #614 already replaced its own startup waits with real polling. This does the same for the SSH launcher and, so that the readiness check runs on a path a real cluster can exercise, pulls the polling into one method both launchers share.
Startup: setup_cluster no longer sleeps. It launches dask ssh and returns the process, the number of workers it should bring up, and the file its output was captured to. The constructor then waits for all of those workers to register with the scheduler, up to a time limit, and watches the launched process so a failed login is reported as soon as dask ssh exits rather than after a fixed wait. Requiring the full worker count also turns a quietly undersized cluster into a clear error.
Shared loop: the waiting itself is one method, _poll_for_workers, that both the srun and SSH launchers call. It watches the scheduler for the workers to register and watches the process bringing them up, and returns an outcome rather than raising, so each launcher still phrases its own error. srun keeps waiting for one worker and quotes its log; the SSH path keeps waiting for the full count and quotes what dask ssh said. Their behavior is unchanged.
Teardown: the fixed sleep is gone. Teardown now asks each process it started to stop and waits until it has actually exited, and kills one that will not stop within a bounded time.
The two time limits are named constants in pybnf/cluster.py (SSH_WORKER_TIMEOUT and TEARDOWN_TIMEOUT), matching how the srun launcher sets its limits.
Verification on a real two node SLURM cluster, using the srun launcher, which needs no login and so runs where the SSH login cannot:
The SSH path adds its own worker count and its own message vocabulary on top of that shared loop; those pieces are covered by unit tests. The SSH login uses paramiko, which cannot authenticate on a host based or Kerberos cluster, so exercising the dask ssh transport itself end to end needs a cluster whose nodes accept a paramiko login.