From 98805eb1ee4177f2cc1caf0d49f6a8361db660f2 Mon Sep 17 00:00:00 2001 From: joncrall Date: Thu, 21 May 2026 21:25:12 -0400 Subject: [PATCH 01/31] [skip ci] Start branch for 0.3.1 --- CHANGELOG.md | 5 ++++- cmd_queue/__init__.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee0c83f..3d0a6be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,10 @@ We are currently working on porting this changelog to the specifications in This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## Version 0.3.0 - Unreleased +## Version 0.3.1 - Unreleased + + +## Version 0.3.0 - Released 2026-05-21 ### Added: * generalized the monitor so it can be launched in an independent process and reports errors better. diff --git a/cmd_queue/__init__.py b/cmd_queue/__init__.py index e6798aa..ad86b97 100644 --- a/cmd_queue/__init__.py +++ b/cmd_queue/__init__.py @@ -306,7 +306,7 @@ __mkinit__ = """ mkinit -m cmd_queue """ -__version__ = '0.3.0' +__version__ = '0.3.1' __submodules__ = { From 7469aaf1a3cd06f6679d23eeb290f40ec4ccfbae Mon Sep 17 00:00:00 2001 From: agent Date: Fri, 22 May 2026 17:23:27 +0000 Subject: [PATCH 02/31] Fix textual monitor diagnostics, add reattach hint, confirm-before-kill - Textual monitor now uses _build_live_renderable as its table_fn so failed/skipped job tables appear in the UI (same as the rich monitor). The post-exit rich printout also shows the full diagnostic renderable. - Both _textual_monitor and _simple_rich_monitor now accept manifest_path and print a reattach hint ("cmd_queue monitor --manifest=...") before the live UI starts, so users can recover if the terminal is killed. manifest_path is threaded through monitor() and _dispatch_monitor. - Textual [k] kill-jobs binding now pushes a ConfirmKillScreen modal instead of killing immediately; [y]/[n] confirm or cancel. - Fixed typo: "do you to kill" -> "Do you want to kill". Co-Authored-By: Claude Sonnet 4.6 --- cmd_queue/backends/tmux.py | 48 ++++++++++++++++++++++++------------- cmd_queue/monitor_app.py | 49 ++++++++++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 21 deletions(-) diff --git a/cmd_queue/backends/tmux.py b/cmd_queue/backends/tmux.py index 97ba2d6..39a32ca 100644 --- a/cmd_queue/backends/tmux.py +++ b/cmd_queue/backends/tmux.py @@ -846,6 +846,7 @@ def _dispatch_monitor( with_textual=with_textual, onfail=onfail, onexit=onexit, + manifest_path=manifest_path, ) if monitor == 'hybrid': side_session = None @@ -882,6 +883,7 @@ def _dispatch_monitor( onfail=onfail, onexit=onexit, side_session=side_session, + manifest_path=manifest_path, ) finally: if side_session and tmux.has_session(side_session): @@ -1004,6 +1006,7 @@ def monitor( onfail: str = '', onexit: str = '', side_session: Optional[str] = None, + manifest_path: Optional[Any] = None, ) -> None: """ Monitor progress until the jobs are done. @@ -1077,9 +1080,9 @@ def monitor( with_textual = False if with_textual: - self._textual_monitor(side_session=side_session) + self._textual_monitor(side_session=side_session, manifest_path=manifest_path) else: - self._simple_rich_monitor(refresh_rate, side_session=side_session) + self._simple_rich_monitor(refresh_rate, side_session=side_session, manifest_path=manifest_path) table, finished, agg_state = self._build_status_table() if onexit == 'capture': self.capture() @@ -1088,24 +1091,30 @@ def monitor( self._print_done_summary(agg_state) return agg_state - def _textual_monitor(self, side_session: Optional[str] = None): + def _textual_monitor( + self, + side_session: Optional[str] = None, + manifest_path: Optional[Any] = None, + ): from rich import print as rich_print - if 0: - print('Kill commands:') - for command in self._kill_commands(): - print(command) + if manifest_path is not None: + rich_print( + f'[dim]To reattach: cmd_queue monitor --manifest={manifest_path}[/dim]' + ) is_running = True while is_running: - table_fn = self._build_status_table + table_fn = lambda: self._build_live_renderable(side_session=side_session) app = CmdQueueMonitorApp( table_fn, kill_fn=self.kill, attach_session=side_session ) app.run() - table, finished, agg_state = self._build_status_table() - rich_print(table) + renderable, finished, agg_state = self._build_live_renderable( + side_session=side_session + ) + rich_print(renderable) if getattr(app, 'attach_requested', False): # User pressed 'a' inside the textual UI; perform the @@ -1121,7 +1130,7 @@ def _textual_monitor(self, side_session: Optional[str] = None): else: from rich.prompt import Confirm - flag = Confirm.ask('do you to kill the procs?') + flag = Confirm.ask('Do you want to kill the procs?') if flag: self.kill() is_running = False @@ -1252,14 +1261,19 @@ def _build_live_renderable(self, side_session: Optional[str] = None): return renderable, finished, agg_state def _simple_rich_monitor( - self, refresh_rate=0.4, side_session: Optional[str] = None + self, + refresh_rate=0.4, + side_session: Optional[str] = None, + manifest_path: Optional[Any] = None, ): import sys - if 0: - print('Kill commands:') - for command in self._kill_commands(): - print(command) + from rich import print as rich_print + + if manifest_path is not None: + rich_print( + f'[dim]To reattach: cmd_queue monitor --manifest={manifest_path}[/dim]' + ) use_keys = side_session is not None and sys.stdin.isatty() try: @@ -1273,7 +1287,7 @@ def _simple_rich_monitor( except KeyboardInterrupt: from rich.prompt import Confirm - flag = Confirm.ask('do you to kill the procs?') + flag = Confirm.ask('Do you want to kill the procs?') if flag: self.kill() diff --git a/cmd_queue/monitor_app.py b/cmd_queue/monitor_app.py index fc5d7a9..e343ff0 100644 --- a/cmd_queue/monitor_app.py +++ b/cmd_queue/monitor_app.py @@ -6,6 +6,7 @@ from rich.text import Text from textual.app import App, ComposeResult from textual.containers import VerticalScroll + from textual.screen import ModalScreen from textual.widgets import Footer, Header, Static TEXTUAL_AVAILABLE = True @@ -15,6 +16,7 @@ VerticalScroll: type = object # type: ignore Footer: type = object # type: ignore Header: type = object # type: ignore + ModalScreen: type = object # type: ignore Static: type = object # type: ignore TEXTUAL_AVAILABLE = False @@ -89,6 +91,41 @@ def refresh_status(self) -> None: app.exit() +class ConfirmKillScreen(ModalScreen): # type: ignore[misc] + """Modal confirmation dialog shown before killing jobs.""" + + CSS = """ + ConfirmKillScreen { + align: center middle; + } + #confirm-dialog { + padding: 1 2; + width: 52; + height: 5; + border: double red; + background: $surface; + content-align: center middle; + } + """ + + def compose(self) -> ComposeResult: + yield Static( + '[bold red]Kill all jobs?[/bold red]\n' + '[dim][[y]] confirm kill [[n]] cancel[/dim]', + id='confirm-dialog', + ) + + def on_mount(self) -> None: + self.bind('y', 'confirm_kill', description='Confirm kill') + self.bind('n', 'cancel_kill', description='Cancel') + + def action_confirm_kill(self) -> None: + self.dismiss(True) + + def action_cancel_kill(self) -> None: + self.dismiss(False) + + class CmdQueueMonitorApp(App): # type: ignore[misc] """Textual app used by the tmux monitor. @@ -205,10 +242,14 @@ def action_quit(self) -> None: self.exit() def action_kill_jobs(self) -> None: - if self.kill_fn is not None: - self.kill_fn() - self.graceful_exit = True - self.exit() + def _on_confirmed(confirmed: bool) -> None: + if confirmed: + if self.kill_fn is not None: + self.kill_fn() + self.graceful_exit = True + self.exit() + + self.push_screen(ConfirmKillScreen(), _on_confirmed) def action_attach_monitor(self) -> None: # The actual tmux attach has to happen *after* the textual app From e8a245df571b97ecbbf7991d8b414b87943d26e3 Mon Sep 17 00:00:00 2001 From: agent Date: Fri, 22 May 2026 17:54:45 +0000 Subject: [PATCH 03/31] Expand CMDQueueConfig.monitor choices to include hybrid and none Adds 'hybrid' and 'none' to the allowed choices so callers using the CMDQueueConfig / ScheduleEvaluationConfig path can access all four monitor modes. Default remains 'inline' to preserve existing behaviour. Co-Authored-By: Claude Sonnet 4.6 --- cmd_queue/cli_boilerplate.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cmd_queue/cli_boilerplate.py b/cmd_queue/cli_boilerplate.py index dc76be8..c828403 100644 --- a/cmd_queue/cli_boilerplate.py +++ b/cmd_queue/cli_boilerplate.py @@ -168,9 +168,17 @@ class CMDQueueConfig(scfg.DataConfig): monitor = scfg.Value( 'inline', - help=('where the live status UI runs while'), + help=ub.paragraph( + """ + Where the live status UI runs while jobs execute. + hybrid = inline monitor + attachable tmux session (best for + interactive use); inline = inline only (default); tmux = + detached tmux session only (survives the calling shell); none + = headless (reattach hint still printed). + """ + ), group='cmd-queue', - choices=['inline', 'tmux'], + choices=['hybrid', 'inline', 'tmux', 'none'], ) queue_name = scfg.Value( From 0cb1ac21ca41474eeb3a6810566e5794fc1d2e13 Mon Sep 17 00:00:00 2001 From: Edward Wang Date: Tue, 23 Jun 2026 14:57:08 -0400 Subject: [PATCH 04/31] Add first-class job setup/teardown lifecycle Add `setup` and `teardown` to BashJob (serial/tmux) and SlurmJob so a job can bracket an external resource (acquire before, release after) without modeling acquire/release as separate, skippable DAG nodes. - `setup`: a gating precondition. Shares the existing PREAMBLE_OK gating, so a failing setup skips the command and marks the job failed. - `teardown`: cleanup that always runs after the command -- on success, failure, and SIGINT/SIGTERM -- provided setup succeeded. It is the job-level try/finally. Rendering is signal-safe and per-job scoped: - serial/tmux concatenate many jobs into one script, so teardown is wrapped in a per-job subshell whose trap cannot leak across jobs. The EXIT trap runs cleanup exactly once; INT/TERM just `exit` (which fires EXIT), so a signalled command still cleans up and never double-runs teardown. - slurm runs one process per job, so the trap lives directly in `--wrap`, gated as `setup && { trap...; command; }`. The main command's exit code stays authoritative (a teardown failure does not flip the job result). A hard SIGKILL cannot be trapped; an out-of-band reclaim (e.g. a lease TTL) is the only backstop for that case. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + cmd_queue/backends/serial.py | 61 ++++++++++++++++++++++++++++++++++-- cmd_queue/backends/slurm.py | 49 +++++++++++++++++++++++++++-- 3 files changed, 107 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee0c83f..e9d9c6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## Version 0.3.0 - Unreleased ### Added: +* First-class job `setup` / `teardown` lifecycle on `BashJob` (serial/tmux) and `SlurmJob`. `setup` is a gating precondition (shares the preamble's `PREAMBLE_OK` gating; a failing setup skips the command and marks the job failed). `teardown` always runs after the command — on success, failure, and SIGINT/SIGTERM — provided setup succeeded. It is rendered as a per-job, signal-safe cleanup (a scoped subshell trap for serial/tmux so it cannot leak across the many jobs in one script; an in-`--wrap` trap for slurm). The main command's exit code stays authoritative (a teardown failure does not flip the job result). A hard SIGKILL cannot be trapped — an out-of-band reclaim (e.g. a lease TTL) is the only backstop for that. This is the job-level try/finally for bracketing an external resource (e.g. acquire/release a GPU lease). * generalized the monitor so it can be launched in an independent process and reports errors better. * New `monitor='hybrid'` mode (now the default for tmux and slurm `run()`): renders the live status table inline in the current shell and *also* spawns a detached `cmd_queue monitor` tmux session. Press `[a]` from the inline UI to attach (or `switch-client` when already inside tmux), `[q]` to stop watching while the queue keeps running. The side session is killed when the inline monitor exits. diff --git a/cmd_queue/backends/serial.py b/cmd_queue/backends/serial.py index 31a2930..5377011 100644 --- a/cmd_queue/backends/serial.py +++ b/cmd_queue/backends/serial.py @@ -43,6 +43,21 @@ class BashJob(base_queue.Job): preamble (str | List[str] | None): One or more setup steps to execute before all commands. + setup (str | List[str] | None): + A gating precondition run before the main command (shares the + preamble's gating: if it fails, the command is skipped and the job + is marked failed). Pairs with ``teardown`` to bracket a resource + (e.g. acquire a GPU lease before the job). + + teardown (str | List[str] | None): + Cleanup run after the main command regardless of success, failure, + or SIGINT/SIGTERM (provided ``setup`` succeeded) -- the job-level + try/finally. Rendered as a per-job subshell with a trap so it is + scoped to this job (serial/tmux concatenate many jobs into one + script) and runs exactly once even when the command is signalled. + A hard SIGKILL cannot be trapped; an out-of-band reclaim (e.g. a + lease TTL) is the only backstop for that. + allow_indent (bool): In some cases indentation matters for the shell command. In that case ensure this is False at the cost of readability in the @@ -99,6 +114,8 @@ def __init__( allow_indent: bool = True, cwd: Optional[str] = None, preamble: Optional[List[str]] = None, + setup: Optional[List[str]] = None, + teardown: Optional[List[str]] = None, **kwargs: Any, ) -> None: if depends is not None and not ub.iterable(depends): @@ -124,7 +141,22 @@ def __init__( self.allow_indent = allow_indent if isinstance(preamble, str): preamble = [preamble] - self.preamble: Optional[List[str]] = preamble + if isinstance(setup, str): + setup = [setup] + # ``setup`` is a gating precondition: it shares the preamble's + # PREAMBLE_OK gating (a failing setup skips the command and marks the + # job failed). It is kept as a distinct, well-named argument so it can + # pair with ``teardown`` for resource-bracketing (acquire / release). + combined_pre = list(preamble or []) + list(setup or []) + self.preamble: Optional[List[str]] = combined_pre or None + if isinstance(teardown, str): + teardown = [teardown] + # ``teardown`` always runs after the command -- on success, failure, and + # SIGINT/SIGTERM -- provided ``setup`` succeeded. It is the job-level + # try/finally for any bracketed resource. See :meth:`finalize_text`. + self.teardown: Optional[List[str]] = ( + list(teardown) if teardown else None + ) def _test_bash_syntax_errors(self) -> None: """ @@ -264,7 +296,32 @@ def finalize_text( script.append('# ********') script.append('# command:') - if self.log: + if self.teardown: + # Per-job cleanup that runs even on failure or signal. Wrap the + # command in a SUBSHELL so the trap is scoped to THIS job -- serial + # and tmux concatenate many jobs into one bash script, so a + # script-level ``trap ... EXIT`` would leak across jobs. EXIT runs + # the teardown exactly once; INT/TERM merely ``exit`` (which fires + # EXIT), so a signalled command still cleans up and teardown never + # double-runs. The subshell exits with the main command's status + # (the EXIT trap does not alter it), so RETURN_CODE stays + # authoritative -- a teardown failure does not flip the job result. + teardown_str = ' ; '.join(self.teardown) + sub_lines = [ + '(', + f' __cmdq_teardown() {{ {teardown_str} ; }}', + ' trap __cmdq_teardown EXIT', + " trap 'exit 143' TERM", + " trap 'exit 130' INT", + f' {self.command}', + ')', + ] + subshell = '\n'.join(sub_lines) + if self.log: + script.append(f'{subshell} 2>&1 | tee {self.log_fpath}') + else: + script.append(subshell) + elif self.log: # If the user requested logging, we use tee to log all output to # disk logged_command = f'({self.command}) 2>&1 | tee {self.log_fpath}' diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index 25d5279..5a6b20a 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -247,6 +247,8 @@ def __init__( shell: Optional[Any] = None, tags: Optional[Any] = None, preamble: List[str] | str | None = None, + setup: List[str] | str | None = None, + teardown: List[str] | str | None = None, **kwargs: Any, ) -> None: super().__init__() @@ -273,6 +275,30 @@ def __init__( self._sbatch_kvargs = ub.udict(kwargs) & SLURM_SBATCH_KVARGS # ty: ignore[unsupported-operator] self._sbatch_flags = ub.udict(kwargs) & SLURM_SBATCH_FLAGS # ty: ignore[unsupported-operator] self.preamble = preamble + # ``setup`` is a gating precondition: fold it into the preamble (slurm + # joins ``preamble && command``, so a failing setup short-circuits the + # command). Keep ``preamble`` a string, matching how it is consumed in + # :meth:`_build_sbatch_args`. + if isinstance(setup, str): + setup = [setup] + if setup: + setup_str = ' && '.join(setup) + if self.preamble: + base = ( + self.preamble + if isinstance(self.preamble, str) + else ' && '.join(self.preamble) + ) + self.preamble = base + ' && ' + setup_str + else: + self.preamble = setup_str + # ``teardown`` always runs after the command (success, failure, or + # SIGTERM within the scancel/timeout ``--signal`` grace window) provided + # setup succeeded. Each slurm job is its own process, so a shell-level + # trap in ``--wrap`` is correctly scoped. See :meth:`_build_sbatch_args`. + if isinstance(teardown, str): + teardown = [teardown] + self.teardown = list(teardown) if teardown else None # if shell not in {None, 'bash'}: # raise NotImplementedError(shell) @@ -405,10 +431,29 @@ def _coerce_gres(gpus): if self.preamble: _preamble.append(self.preamble) + if self.teardown: + # Guaranteed cleanup co-located in the job. EXIT runs teardown once; + # INT/TERM ``exit`` (firing EXIT) so a scancel/timeout (SIGTERM, + # given a ``--signal=B:TERM@`` grace window) still cleans up. + # The ``setup && { ... }`` shape gates teardown on setup success: + # if the preamble/setup precondition fails, the group (which sets + # the trap) never runs, so nothing is torn down. + teardown_str = ' ; '.join(self.teardown) + main = ( + '{ ' + f'__cmdq_teardown() {{ {teardown_str} ; }} ; ' + 'trap __cmdq_teardown EXIT ; ' + "trap 'exit 143' TERM ; trap 'exit 130' INT ; " + f'{self.command} ; ' + '}' + ) + else: + main = self.command + if _preamble: - wrp_command = shlex.quote(' && '.join(_preamble + [self.command])) # ty: ignore[invalid-argument-type] + wrp_command = shlex.quote(' && '.join(_preamble + [main])) # ty: ignore[invalid-argument-type] else: - wrp_command = shlex.quote(self.command) # ty: ignore[invalid-argument-type] + wrp_command = shlex.quote(main) # ty: ignore[invalid-argument-type] if self.shell: wrp_command = shlex.quote(self.shell + ' -c ' + wrp_command) From e7f576526854ddfd32ed11f3ff7d65c4df7c2006 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Tue, 23 Jun 2026 16:20:35 -0400 Subject: [PATCH 05/31] update xcookie --- dev/setup_secrets.sh | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/dev/setup_secrets.sh b/dev/setup_secrets.sh index 08b7363..6ea6d7c 100644 --- a/dev/setup_secrets.sh +++ b/dev/setup_secrets.sh @@ -347,13 +347,40 @@ _secret_fingerprint(){ } +_gitlab_pick_remote(){ + # Echo the name of the remote that points at a GitLab instance. + # A project may have multiple backends (e.g. `origin` -> github.com and + # `gitlab` -> gitlab.kitware.com), so we cannot assume `origin` is the + # GitLab remote. Preference order: + # 1. a remote literally named `gitlab` + # 2. any remote whose URL host contains `gitlab` + # 3. `origin` (legacy fallback for single-backend repos) + local name url + if git remote get-url gitlab >/dev/null 2>&1; then + printf '%s\n' gitlab + return 0 + fi + while read -r name; do + [[ -z "$name" ]] && continue + url=$(git remote get-url "$name" 2>/dev/null) || continue + if [[ "$url" == *gitlab* ]]; then + printf '%s\n' "$name" + return 0 + fi + done < <(git remote 2>/dev/null) + printf '%s\n' origin +} + + _gitlab_remote_info(){ - # Parse the `origin` remote URL and emit three lines: HOST PROJECT_PATH GROUP_PATH. + # Parse the GitLab remote URL and emit three lines: HOST PROJECT_PATH GROUP_PATH. # Supports SSH (user@host:ns/repo.git) and HTTPS (https://host/ns/repo.git) - # and arbitrarily nested namespaces. - local remote_url host path - remote_url=$(git remote get-url origin 2>/dev/null) || { - echo "ERROR: cannot read origin remote URL" >&2 + # and arbitrarily nested namespaces. The GitLab remote is auto-detected + # (see _gitlab_pick_remote) rather than assumed to be `origin`. + local remote_name remote_url host path + remote_name=$(_gitlab_pick_remote) + remote_url=$(git remote get-url "$remote_name" 2>/dev/null) || { + echo "ERROR: cannot read '$remote_name' remote URL" >&2 return 1 } if [[ "$remote_url" =~ ^[^@/:]+@([^:]+):(.+)$ ]]; then @@ -369,7 +396,7 @@ _gitlab_remote_info(){ host="${BASH_REMATCH[2]}" path="${BASH_REMATCH[3]}" else - echo "ERROR: unrecognized origin URL: $remote_url" >&2 + echo "ERROR: unrecognized GitLab remote URL: $remote_url" >&2 return 1 fi path="${path%.git}" From e6d1111eddd60db0aeeed62cd18d084816f85452 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Tue, 23 Jun 2026 19:22:41 -0400 Subject: [PATCH 06/31] Add tests and docs for job setup/teardown lifecycle Follow-up to the setup/teardown feature (0cb1ac2). Adds the missing test coverage and documentation, and fixes a CHANGELOG misfiling. Tests: - serial/tmux (test_bash_variants.py): execution-based tests covering teardown-on-success, teardown-on-command-failure (exit code stays authoritative), setup-failure-skips-command-and-teardown, teardown failure not flipping a passing result, the per-job subshell trap not leaking across concatenated jobs, and teardown firing on a SIGTERM to the process group. - slurm (test_slurm_variants.py): render tests for setup folding into the preamble gate and teardown wrapping the command with a trap, plus an execution test confirming exit-code authority and the setup gate. Docs: - Document setup/teardown on the SlurmJob class docstring (previously had no Args section) and in both SlurmQueue.submit and base Queue.submit. CHANGELOG: - Move the setup/teardown entry from the released 0.3.0 section to the unreleased 0.3.1 section (the feature postdates the 0.3.0 release). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 4 +- cmd_queue/backends/slurm.py | 32 ++++++ cmd_queue/base_queue.py | 7 +- tests/test_bash_variants.py | 206 +++++++++++++++++++++++++++++++++++ tests/test_slurm_variants.py | 83 ++++++++++++++ 5 files changed, 330 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b0c51b..dca1d97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,13 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## Version 0.3.1 - Unreleased +### Added: +* First-class job `setup` / `teardown` lifecycle on `BashJob` (serial/tmux) and `SlurmJob`. `setup` is a gating precondition (shares the preamble's `PREAMBLE_OK` gating; a failing setup skips the command and marks the job failed). `teardown` always runs after the command — on success, failure, and SIGINT/SIGTERM — provided setup succeeded. It is rendered as a per-job, signal-safe cleanup (a scoped subshell trap for serial/tmux so it cannot leak across the many jobs in one script; an in-`--wrap` trap for slurm). The main command's exit code stays authoritative (a teardown failure does not flip the job result). A hard SIGKILL cannot be trapped — an out-of-band reclaim (e.g. a lease TTL) is the only backstop for that. This is the job-level try/finally for bracketing an external resource (e.g. acquire/release a GPU lease). + ## Version 0.3.0 - Released 2026-05-21 ### Added: -* First-class job `setup` / `teardown` lifecycle on `BashJob` (serial/tmux) and `SlurmJob`. `setup` is a gating precondition (shares the preamble's `PREAMBLE_OK` gating; a failing setup skips the command and marks the job failed). `teardown` always runs after the command — on success, failure, and SIGINT/SIGTERM — provided setup succeeded. It is rendered as a per-job, signal-safe cleanup (a scoped subshell trap for serial/tmux so it cannot leak across the many jobs in one script; an in-`--wrap` trap for slurm). The main command's exit code stays authoritative (a teardown failure does not flip the job result). A hard SIGKILL cannot be trapped — an out-of-band reclaim (e.g. a lease TTL) is the only backstop for that. This is the job-level try/finally for bracketing an external resource (e.g. acquire/release a GPU lease). * generalized the monitor so it can be launched in an independent process and reports errors better. * New `monitor='hybrid'` mode (now the default for tmux and slurm `run()`): renders the live status table inline in the current shell and *also* spawns a detached `cmd_queue monitor` tmux session. Press `[a]` from the inline UI to attach (or `switch-client` when already inside tmux), `[q]` to stop watching while the queue keeps running. The side session is killed when the inline monitor exits. diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index 5a6b20a..0e4d277 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -223,6 +223,31 @@ class SlurmJob(base_queue.Job): """ Represents a slurm job that hasn't been submitted yet + Args: + command (str): the command line to execute. + + preamble (str | List[str] | None): + job-specific setup step(s) executed before the command. Folded + into the ``&&`` chain ahead of the command, so a failing preamble + short-circuits (skips) the command. + + setup (str | List[str] | None): + A gating precondition run before the command. It is folded into the + preamble (slurm joins ``preamble && command``), so a failing setup + short-circuits the command and the job exits non-zero. Pairs with + ``teardown`` to bracket an external resource (e.g. acquire a GPU + lease before the job). + + teardown (str | List[str] | None): + Cleanup run after the command regardless of success, failure, or + SIGINT/SIGTERM (within the scancel/timeout ``--signal`` grace + window), provided ``setup`` succeeded -- the job-level try/finally. + Each slurm job is its own process, so it is rendered as a trap + inside ``--wrap``. The command's exit code stays authoritative (a + teardown failure does not flip the job result). A hard SIGKILL + cannot be trapped; an out-of-band reclaim (e.g. a lease TTL) is the + only backstop for that. + Example: >>> # xdoctest: +REQUIRES(module:pint) >>> from cmd_queue.slurm_queue import * # NOQA @@ -673,6 +698,13 @@ def submit( # ty: ignore[invalid-method-override] name (str | None): name of job shell (str | None): shell to use, defaults to bash depends (str | List[str] | None): name of jobs to depend on + setup (str | List[str] | None): a gating precondition run + before the command (a failing setup skips the command and + fails the job); pairs with ``teardown``. See + :class:`SlurmJob`. + teardown (str | List[str] | None): cleanup that always runs + after the command -- on success, failure, or signal -- + provided ``setup`` succeeded. See :class:`SlurmJob`. **slurm_options:see SLURM_SBATCH_KVARGS and SLURM_SBATCH_FLAGS Returns: diff --git a/cmd_queue/base_queue.py b/cmd_queue/base_queue.py index efdbab8..f1ccf51 100644 --- a/cmd_queue/base_queue.py +++ b/cmd_queue/base_queue.py @@ -182,7 +182,12 @@ def submit(self, command: Union[str, Job], **kwargs: Any) -> Job: Args: command (str | Job): The command to execute name: specify the name of the job - **kwargs: passed to :class:`cmd_queue.serial_queue.BashJob` + **kwargs: passed to :class:`cmd_queue.serial_queue.BashJob`. + Notably this includes ``setup`` (a gating precondition run + before the command) and ``teardown`` (cleanup that always runs + after the command -- on success, failure, or SIGINT/SIGTERM -- + provided ``setup`` succeeded), which together bracket an + external resource. See :class:`cmd_queue.serial_queue.BashJob`. """ # TODO: we could accept additional args here that modify how we handle # the command in the bash script we build (i.e. if the script is diff --git a/tests/test_bash_variants.py b/tests/test_bash_variants.py index 817a9f3..fee6e13 100644 --- a/tests/test_bash_variants.py +++ b/tests/test_bash_variants.py @@ -509,3 +509,209 @@ def test_bashjob_exec_happy_path(): status = kwutil.Json.load(job.stat_fpath) assert status['ret'] == 0 + + +def _make_teardown_job(tmp_path, command, setup=None, teardown=None): + """ + Helper to build a BashJob with status paths redirected into ``tmp_path``. + + Returns the job plus the marker file that ``teardown`` writes to (so a test + can detect whether teardown actually ran). + """ + td_marker = tmp_path / 'teardown_ran.txt' + job = BashJob( + command, + name='job', + setup=setup, + teardown=teardown, + ) + job.log = False + job.stat_fpath = tmp_path / 'job.status.json' + job.pass_fpath = tmp_path / 'job.pass' + job.fail_fpath = tmp_path / 'job.fail' + return job, td_marker + + +def test_bashjob_exec_teardown_runs_on_success(): + # teardown is the job-level try/finally: on a clean run it fires after the + # command, and a teardown of its own does not change the (passing) result. + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + td_marker = tmp_path / 'teardown_ran.txt' + job, td_marker = _make_teardown_job( + tmp_path, + 'echo CMD', + setup='echo SETUP', + teardown=f'echo TD > "{td_marker}"', + ) + + text = job.finalize_text(with_status=True, with_gaurds=True) + subprocess.run(['bash', '-n'], input=text, text=True, check=True) + subprocess.run( + ['bash'], input=text, text=True, cwd=str(tmp_path), + capture_output=True, check=False, + ) + + assert td_marker.exists(), 'teardown should run on success' + assert job.pass_fpath.exists(), 'job should pass' + status = kwutil.Json.load(job.stat_fpath) + assert status['ret'] == 0 + + +def test_bashjob_exec_teardown_runs_on_command_failure(): + # teardown must still run when the command fails, and the failing command's + # exit code stays authoritative (the job is marked failed). + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + td_marker = tmp_path / 'teardown_ran.txt' + job, td_marker = _make_teardown_job( + tmp_path, + 'echo CMD; exit 5', + setup='echo SETUP', + teardown=f'echo TD > "{td_marker}"', + ) + + text = job.finalize_text(with_status=True, with_gaurds=True) + subprocess.run(['bash', '-n'], input=text, text=True, check=True) + subprocess.run( + ['bash'], input=text, text=True, cwd=str(tmp_path), + capture_output=True, check=False, + ) + + assert td_marker.exists(), 'teardown should run even when command fails' + assert job.fail_fpath.exists(), 'job should fail' + status = kwutil.Json.load(job.stat_fpath) + assert status['ret'] == 5, 'command exit code stays authoritative' + + +def test_bashjob_exec_teardown_skipped_when_setup_fails(): + # setup is a gating precondition: if it fails the command is skipped, and + # because the resource was never acquired teardown must not run either. + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + outfile = tmp_path / 'ran.txt' + td_marker = tmp_path / 'teardown_ran.txt' + job, td_marker = _make_teardown_job( + tmp_path, + f'echo ran > "{outfile}"', + setup='false', + teardown=f'echo TD > "{td_marker}"', + ) + + text = job.finalize_text(with_status=True, with_gaurds=True) + subprocess.run(['bash', '-n'], input=text, text=True, check=True) + subprocess.run( + ['bash'], input=text, text=True, cwd=str(tmp_path), + capture_output=True, check=False, + ) + + assert not outfile.exists(), 'command should not run if setup fails' + assert not td_marker.exists(), 'teardown should not run if setup fails' + assert job.fail_fpath.exists(), 'job should fail' + status = kwutil.Json.load(job.stat_fpath) + assert status['ret'] != 0 + + +def test_bashjob_exec_teardown_failure_does_not_flip_result(): + # A failing teardown must not turn a passing job into a failure. + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + td_marker = tmp_path / 'teardown_ran.txt' + job, td_marker = _make_teardown_job( + tmp_path, + 'echo CMD', + setup='echo SETUP', + teardown=f'echo TD > "{td_marker}"; false', + ) + + text = job.finalize_text(with_status=True, with_gaurds=True) + subprocess.run(['bash', '-n'], input=text, text=True, check=True) + subprocess.run( + ['bash'], input=text, text=True, cwd=str(tmp_path), + capture_output=True, check=False, + ) + + assert td_marker.exists(), 'teardown should run' + assert job.pass_fpath.exists(), 'teardown failure must not flip result' + status = kwutil.Json.load(job.stat_fpath) + assert status['ret'] == 0 + + +def test_bashjob_teardown_trap_does_not_leak_across_jobs(): + # serial/tmux concatenate many jobs into ONE bash script. The teardown trap + # is wrapped in a per-job subshell so it must fire exactly once for its own + # job and never leak into a sibling job that has no teardown. + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + td_marker = tmp_path / 'teardown_ran.txt' + + j1 = BashJob('echo J1', name='j1', + teardown=f'printf x >> "{td_marker}"') + j1.log = False + j1.stat_fpath = tmp_path / 'j1.status.json' + j1.pass_fpath = tmp_path / 'j1.pass' + j1.fail_fpath = tmp_path / 'j1.fail' + + j2 = BashJob('echo J2', name='j2') # no teardown + j2.log = False + j2.stat_fpath = tmp_path / 'j2.status.json' + j2.pass_fpath = tmp_path / 'j2.pass' + j2.fail_fpath = tmp_path / 'j2.fail' + + text = '\n'.join([ + j1.finalize_text(with_status=True, with_gaurds=True), + j2.finalize_text(with_status=True, with_gaurds=True), + ]) + subprocess.run(['bash', '-n'], input=text, text=True, check=True) + subprocess.run( + ['bash'], input=text, text=True, cwd=str(tmp_path), + capture_output=True, check=False, + ) + + # Exactly one teardown invocation -- the trap did not leak to j2. + assert td_marker.read_text() == 'x', ( + 'teardown must run exactly once and not leak across jobs' + ) + assert j1.pass_fpath.exists() + assert j2.pass_fpath.exists() + + +def test_bashjob_exec_teardown_runs_on_sigterm(): + # teardown advertises signal-safety: a SIGTERM to the whole process group + # (what a terminal Ctrl-C / cancel does) must still fire the cleanup. + import os + import signal + import time + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + td_marker = tmp_path / 'teardown_ran.txt' + job, td_marker = _make_teardown_job( + tmp_path, + 'echo START; sleep 30', + setup='echo SETUP', + teardown=f'echo TD > "{td_marker}"', + ) + text = job.finalize_text(with_status=True, with_gaurds=True) + subprocess.run(['bash', '-n'], input=text, text=True, check=True) + + proc = subprocess.Popen( + ['bash', '-c', text], + cwd=str(tmp_path), + start_new_session=True, # own process group, like a real job + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + ) + try: + time.sleep(1.0) # let it reach the sleep inside the command + os.killpg(os.getpgid(proc.pid), signal.SIGTERM) + proc.wait(timeout=10) + finally: + if proc.poll() is None: # pragma: no cover + proc.kill() + + # Give the trap a beat to flush its marker file. + for _ in range(20): + if td_marker.exists(): + break + time.sleep(0.1) + assert td_marker.exists(), 'teardown should run on SIGTERM' diff --git a/tests/test_slurm_variants.py b/tests/test_slurm_variants.py index 2baf9b8..ee6e2f0 100644 --- a/tests/test_slurm_variants.py +++ b/tests/test_slurm_variants.py @@ -73,3 +73,86 @@ def test_slurm_wrap_omits_missing_preambles(): sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) payload = _extract_wrap_payload(sbatch_args) assert payload == 'echo JOB && echo CMD' + + +def test_slurm_setup_folds_into_preamble_gate(): + # ``setup`` is a gating precondition: it is folded into the preamble so the + # ``&&`` chain short-circuits the command if setup fails. With no teardown + # the command is not wrapped. + queue = SlurmQueue(preamble=None) + job = queue.submit('echo CMD', setup='acquire_lease') + sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) + payload = _extract_wrap_payload(sbatch_args) + assert payload == 'acquire_lease && echo CMD' + + # setup composes after an existing preamble (global then job then setup) + queue = SlurmQueue(preamble=['echo GLOBAL']) + job = queue.submit('echo CMD', preamble='echo JOB', setup='acquire_lease') + sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) + payload = _extract_wrap_payload(sbatch_args) + assert payload == 'echo GLOBAL && echo JOB && acquire_lease && echo CMD' + + +def test_slurm_teardown_wraps_command_with_trap(): + # ``teardown`` co-locates a signal-safe cleanup trap with the command. slurm + # runs one process per job, so the trap lives directly in ``--wrap``. + queue = SlurmQueue(preamble=None) + job = queue.submit('echo CMD', teardown='release_lease') + sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) + payload = _extract_wrap_payload(sbatch_args) + + # The command is wrapped in a brace group that installs the trap. + assert '__cmdq_teardown() { release_lease ; }' in payload + assert 'trap __cmdq_teardown EXIT' in payload + assert "trap 'exit 143' TERM" in payload + assert "trap 'exit 130' INT" in payload + # The actual command runs inside the trapped group. + assert 'echo CMD' in payload + + +def test_slurm_setup_gates_teardown(): + # ``setup && { trap...; command; }``: when setup fails the whole group + # (which installs the trap) never runs, so nothing is torn down. + queue = SlurmQueue(preamble=None) + job = queue.submit('echo CMD', setup='acquire_lease', teardown='release_lease') + sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) + payload = _extract_wrap_payload(sbatch_args) + + # setup precedes the trapped group via ``&&`` so it gates teardown. + assert payload.startswith('acquire_lease && {') + assert 'trap __cmdq_teardown EXIT' in payload + + +def test_slurm_teardown_executes_as_documented(): + # Execute the rendered wrap payload to confirm the runtime contract: + # the command's exit code stays authoritative and teardown always runs. + import subprocess + queue = SlurmQueue(preamble=None) + + # command fails -> exit code preserved, teardown still runs + job = queue.submit('echo CMD; exit 5', name='a', + setup='echo SETUP', teardown='echo TD') + payload = _extract_wrap_payload( + job._build_sbatch_args(global_preamble=queue.header_commands)) + r = subprocess.run(['bash', '-c', payload], capture_output=True, text=True) + assert r.returncode == 5, 'command exit code stays authoritative' + assert 'TD' in r.stdout, 'teardown runs even when command fails' + + # teardown fails -> does not flip a passing command + job = queue.submit('echo CMD', name='b', + setup='echo SETUP', teardown='echo TD; false') + payload = _extract_wrap_payload( + job._build_sbatch_args(global_preamble=queue.header_commands)) + r = subprocess.run(['bash', '-c', payload], capture_output=True, text=True) + assert r.returncode == 0, 'teardown failure must not flip the result' + assert 'TD' in r.stdout + + # setup fails -> command skipped, teardown not run + job = queue.submit('echo CMD', name='c', + setup='false', teardown='echo TD') + payload = _extract_wrap_payload( + job._build_sbatch_args(global_preamble=queue.header_commands)) + r = subprocess.run(['bash', '-c', payload], capture_output=True, text=True) + assert r.returncode != 0, 'setup failure fails the job' + assert 'CMD' not in r.stdout, 'command should not run if setup fails' + assert 'TD' not in r.stdout, 'teardown should not run if setup fails' From cb5144d59e0b3836a8d936978e7a47b796364fc8 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Tue, 23 Jun 2026 19:26:39 -0400 Subject: [PATCH 07/31] Fix ty check errors for setup/teardown tests - Broaden BashJob's preamble/setup/teardown annotations to `List[str] | str | None`, matching the constructor's actual behavior (it coerces a str to a list), the docstring, and how SlurmJob already types them. Fixes the new teardown test passing a str argument. - Annotate the pre-existing `job_kwargs` dicts in the variant tests as `dict` so `**job_kwargs` unpacking no longer trips the heterogeneous union inference. `ty check cmd_queue tests` now passes cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/serial.py | 6 +++--- tests/test_bash_variants.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd_queue/backends/serial.py b/cmd_queue/backends/serial.py index 5377011..4fd89c0 100644 --- a/cmd_queue/backends/serial.py +++ b/cmd_queue/backends/serial.py @@ -113,9 +113,9 @@ def __init__( tags: Optional[Any] = None, allow_indent: bool = True, cwd: Optional[str] = None, - preamble: Optional[List[str]] = None, - setup: Optional[List[str]] = None, - teardown: Optional[List[str]] = None, + preamble: List[str] | str | None = None, + setup: List[str] | str | None = None, + teardown: List[str] | str | None = None, **kwargs: Any, ) -> None: if depends is not None and not ub.iterable(depends): diff --git a/tests/test_bash_variants.py b/tests/test_bash_variants.py index fee6e13..505e3d0 100644 --- a/tests/test_bash_variants.py +++ b/tests/test_bash_variants.py @@ -39,7 +39,7 @@ def test_primary_bash_job_text_variants(): dep = BashJob('echo hi', name='job1') for variant in main_variants: - job_kwargs = {} + job_kwargs: dict = {} if variant['depends']: job_kwargs['depends'] = [dep] if variant['cwd']: @@ -96,7 +96,7 @@ def test_bash_job_variants_syntax_grided(): dep = BashJob('echo hi', name='job1') for variant in grid_variants: - job_kwargs = {} + job_kwargs: dict = {} if variant['depends']: job_kwargs['depends'] = [dep] if variant['cwd']: From 4eeacaf927b659daf13c172f5568ba89a121184c Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Tue, 23 Jun 2026 19:41:40 -0400 Subject: [PATCH 08/31] Resolve all `ty check cmd_queue tests` diagnostics A newer ty (with full deps installed so real types resolve) surfaced 11 diagnostics. Fixed each at the source: - serial/slurm `str.join` overload errors: the base `Job` types `command` as `str | None`, leaking `None` into the rendered script list. Narrow `self.command` (and `self.name` for slurm, always uuid-filled) to `str` in the concrete jobs. - slurm list-valued `preamble`: ty caught a real latent bug -- a list preamble was appended as a single element, crashing `' && '.join(...)` at runtime. Flatten lists into the `&&` chain. Drop the now-stale `# ty: ignore[invalid-argument-type]` comments. - slurm `_build_command`: narrowing `self.name` to `str` fixes the `dict[str | None, str]` key type passed as `jobname_to_varname`. - tmux `_semaphore_wait_command`: broaden `flag_fpaths` to `Iterable[str | os.PathLike]` (it only str-formats its inputs, which are Paths at the call site). - tmux monitor stub assignment: the rehydrated `SimpleNamespace` job stubs are intentional duck-typing; scope a `# ty: ignore`. - `Queue.is_available`: declare it on the base `Queue` (every backend already implements it and `available_backends` relies on it). Align `SlurmQueue.is_available` from staticmethod to classmethod to match. - tests: narrow the base `Queue` to `TMUXMultiQueue` via `cast` where the tmux-only `workers` attribute is accessed. - monitor_app: add `# ty: ignore[unsupported-base]` to the three classes that subclass the optional textual base classes (ty cannot compute an MRO from the optional-import union base). `ty check cmd_queue tests` now passes cleanly; full test suite green (51 passed, 2 skipped). Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/serial.py | 4 +++- cmd_queue/backends/slurm.py | 27 +++++++++++++++++++-------- cmd_queue/backends/tmux.py | 7 +++++-- cmd_queue/base_queue.py | 9 +++++++++ cmd_queue/monitor_app.py | 6 +++--- tests/test_backend_contract.py | 10 ++++++++-- tests/test_generated_output_compat.py | 5 ++++- 7 files changed, 51 insertions(+), 17 deletions(-) diff --git a/cmd_queue/backends/serial.py b/cmd_queue/backends/serial.py index 4fd89c0..1772d24 100644 --- a/cmd_queue/backends/serial.py +++ b/cmd_queue/backends/serial.py @@ -125,7 +125,9 @@ def __init__( self.pathid = self.name + '_' + ub.hash_data(uuid.uuid4())[0:8] self.kwargs = kwargs # unused kwargs self.cwd = cwd - self.command = command + # The base ``Job`` types ``command`` as ``str | None``; a BashJob always + # has a concrete command, so narrow it (keeps ``'\n'.join`` well-typed). + self.command: str = command self.depends: List[base_queue.Job] = list(depends) if depends else [] self.bookkeeper = bookkeeper self.log = log diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index 0e4d277..ec8e444 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -284,8 +284,13 @@ def __init__( if depends is not None and not ub.iterable(depends): depends = [depends] # type: ignore self.unused_kwargs = kwargs - self.command = command - self.name = name + # The base ``Job`` types ``command`` as ``str | None``; a SlurmJob always + # has a concrete command, so narrow it (keeps the ``--wrap`` join and + # ``shlex.quote`` well-typed). + self.command: str = command + # ``name`` is filled with a uuid above when omitted, so it is always a + # concrete ``str`` here (the base ``Job`` types it as ``str | None``). + self.name: str = name self.output_fpath = output_fpath self.depends = depends self.cpus = cpus @@ -450,11 +455,17 @@ def _coerce_gres(gpus): import shlex - _preamble = [] + _preamble: List[str] = [] if global_preamble: _preamble.extend(global_preamble) if self.preamble: - _preamble.append(self.preamble) + # ``preamble`` may be a single string or a list of steps; flatten a + # list into the ``&&`` chain rather than appending it as one element + # (which would crash the join below). + if isinstance(self.preamble, str): + _preamble.append(self.preamble) + else: + _preamble.extend(self.preamble) if self.teardown: # Guaranteed cleanup co-located in the job. EXIT runs teardown once; @@ -476,9 +487,9 @@ def _coerce_gres(gpus): main = self.command if _preamble: - wrp_command = shlex.quote(' && '.join(_preamble + [main])) # ty: ignore[invalid-argument-type] + wrp_command = shlex.quote(' && '.join(_preamble + [main])) else: - wrp_command = shlex.quote(main) # ty: ignore[invalid-argument-type] + wrp_command = shlex.quote(main) if self.shell: wrp_command = shlex.quote(self.shell + ' -c ' + wrp_command) @@ -621,8 +632,8 @@ def _slurm_checks() -> None: ) status['has_working_nodes'] = has_working_nodes - @staticmethod - def is_available() -> bool: + @classmethod + def is_available(cls) -> bool: """ Determines if we can run the slurm queue or not. """ diff --git a/cmd_queue/backends/tmux.py b/cmd_queue/backends/tmux.py index 39a32ca..ef4a77e 100644 --- a/cmd_queue/backends/tmux.py +++ b/cmd_queue/backends/tmux.py @@ -49,6 +49,7 @@ """ from __future__ import annotations +import os import uuid from typing import Any, Dict, Iterable, List, Optional @@ -299,7 +300,7 @@ def __nice__(self) -> str: return str(ub.urepr(self.jobs)) def _semaphore_wait_command( - self, flag_fpaths: Iterable[str], msg: str + self, flag_fpaths: Iterable[str | os.PathLike], msg: str ) -> str: r""" TODO: use flock? or inotify? @@ -1561,7 +1562,9 @@ def _from_manifest(cls, manifest: Dict[str, Any]) -> 'TMUXMultiQueue': depends=list(j.get('depends') or []), ) ) - worker.jobs = stubs + # These are deliberately lightweight duck-typed stubs (only the + # attributes the failed-jobs renderer reads), not full Job objects. + worker.jobs = stubs # ty: ignore[invalid-assignment] workers.append(worker) self.workers = workers return self diff --git a/cmd_queue/base_queue.py b/cmd_queue/base_queue.py index f1ccf51..8c244c2 100644 --- a/cmd_queue/base_queue.py +++ b/cmd_queue/base_queue.py @@ -250,6 +250,15 @@ def submit(self, command: Union[str, Job], **kwargs: Any) -> Job: self.num_real_jobs += 1 return job + @classmethod + def is_available(cls) -> bool: + """ + Check if this backend can run on the current system. Each concrete + backend overrides this; the base declares it so it is part of the + common queue contract (see :meth:`available_backends`). + """ + raise NotImplementedError + @classmethod def _backend_classes(cls): from cmd_queue import _registry diff --git a/cmd_queue/monitor_app.py b/cmd_queue/monitor_app.py index e343ff0..77b760a 100644 --- a/cmd_queue/monitor_app.py +++ b/cmd_queue/monitor_app.py @@ -41,7 +41,7 @@ def _missing_textual_error() -> ImportError: ) -class JobTable(Static): # type: ignore[misc] +class JobTable(Static): # type: ignore[misc] # ty: ignore[unsupported-base] """A small auto-refreshing widget that displays the queue status table.""" DEFAULT_CSS = """ @@ -91,7 +91,7 @@ def refresh_status(self) -> None: app.exit() -class ConfirmKillScreen(ModalScreen): # type: ignore[misc] +class ConfirmKillScreen(ModalScreen): # type: ignore[misc] # ty: ignore[unsupported-base] """Modal confirmation dialog shown before killing jobs.""" CSS = """ @@ -126,7 +126,7 @@ def action_cancel_kill(self) -> None: self.dismiss(False) -class CmdQueueMonitorApp(App): # type: ignore[misc] +class CmdQueueMonitorApp(App): # type: ignore[misc] # ty: ignore[unsupported-base] """Textual app used by the tmux monitor. The constructor and runtime attributes are intentionally stable because diff --git a/tests/test_backend_contract.py b/tests/test_backend_contract.py index 52eeed7..e8c5d90 100644 --- a/tests/test_backend_contract.py +++ b/tests/test_backend_contract.py @@ -33,8 +33,14 @@ def test_backend_classes_support_minimal_contract(backend, tmp_path): combined_text = text if backend == 'tmux': # ``TMUXMultiQueue.finalize_text`` returns the driver script. The - # per-worker scripts contain the actual job commands. - combined_text += '\n'.join(worker.finalize_text() for worker in queue.workers) + # per-worker scripts contain the actual job commands. ``workers`` is a + # tmux-only attribute, so narrow the base ``Queue`` to the concrete type. + from typing import cast + from cmd_queue.backends.tmux import TMUXMultiQueue + tmux_queue = cast(TMUXMultiQueue, queue) + combined_text += '\n'.join( + worker.finalize_text() for worker in tmux_queue.workers + ) assert isinstance(text, str) assert 'echo first' in combined_text diff --git a/tests/test_generated_output_compat.py b/tests/test_generated_output_compat.py index ed22a39..a58a67e 100644 --- a/tests/test_generated_output_compat.py +++ b/tests/test_generated_output_compat.py @@ -23,7 +23,9 @@ def test_serial_generated_text_invariants(tmp_path): def test_tmux_generated_text_invariants(tmp_path): + from typing import cast from cmd_queue import Queue + from cmd_queue.backends.tmux import TMUXMultiQueue queue = Queue.create( backend='tmux', name='compat_tmux', rootid='root', dpath=tmp_path, size=1 @@ -32,9 +34,10 @@ def test_tmux_generated_text_invariants(tmp_path): queue.submit('echo second', name='second', depends=first) text = queue.finalize_text(with_status=False, with_gaurds=False, with_locks=False) + # ``workers`` is a tmux-only attribute; narrow to the concrete queue type. worker_text = '\n'.join( worker.finalize_text(with_status=False, with_gaurds=False) - for worker in queue.workers + for worker in cast(TMUXMultiQueue, queue).workers ) assert 'tmux new-session' in text assert 'source ' in text From 3e9082a73fb052356c38c8f5d2eeb59afb8eee3f Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Tue, 23 Jun 2026 19:54:38 -0400 Subject: [PATCH 09/31] Update docs for the 0.3.1 preamble fix - CHANGELOG: record the slurm list-valued preamble crash fix under 0.3.1. - slurm: correct a now-stale comment that claimed `_build_sbatch_args` requires `preamble` to be a string (it handles lists too after the fix). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 3 +++ cmd_queue/backends/slurm.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dca1d97..081a08e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added: * First-class job `setup` / `teardown` lifecycle on `BashJob` (serial/tmux) and `SlurmJob`. `setup` is a gating precondition (shares the preamble's `PREAMBLE_OK` gating; a failing setup skips the command and marks the job failed). `teardown` always runs after the command — on success, failure, and SIGINT/SIGTERM — provided setup succeeded. It is rendered as a per-job, signal-safe cleanup (a scoped subshell trap for serial/tmux so it cannot leak across the many jobs in one script; an in-`--wrap` trap for slurm). The main command's exit code stays authoritative (a teardown failure does not flip the job result). A hard SIGKILL cannot be trapped — an out-of-band reclaim (e.g. a lease TTL) is the only backstop for that. This is the job-level try/finally for bracketing an external resource (e.g. acquire/release a GPU lease). +### Fixed: +* A list-valued `preamble` passed to `SlurmJob` (e.g. `submit(..., preamble=['a', 'b'])`) no longer crashes script construction; list steps are now flattened into the `&&` chain instead of being appended as a single element. + ## Version 0.3.0 - Released 2026-05-21 diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index ec8e444..edb7dda 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -307,8 +307,8 @@ def __init__( self.preamble = preamble # ``setup`` is a gating precondition: fold it into the preamble (slurm # joins ``preamble && command``, so a failing setup short-circuits the - # command). Keep ``preamble`` a string, matching how it is consumed in - # :meth:`_build_sbatch_args`. + # command). Folding collapses ``preamble`` to a single string here; + # :meth:`_build_sbatch_args` also accepts a plain list preamble. if isinstance(setup, str): setup = [setup] if setup: From 9e26f28ab8d32b493cdf71a5ab8277cf33bf6132 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Wed, 24 Jun 2026 14:46:34 -0400 Subject: [PATCH 10/31] Add availability-gated backend execution tests; fix slurm doctest gating Until now every slurm test only rendered the sbatch text; nothing actually ran a queue, and the only slurm-submitting code (the --run doctests) was gated on a manual flag, not on slurm availability. - tests/test_backend_execution.py: actually runs a queue on each backend and verifies execution via on-disk marker files. Covers a simple dependent DAG, the setup/teardown lifecycle (setup before command, teardown after), and the setup-failure gate (command skipped, teardown not run). serial always runs; tmux and slurm are skipped unless is_available(), so a real slurm install is exercised where present. Working dirs use ub.Path.appdir ($HOME, shared across cluster nodes) rather than /tmp so a compute node's marker writes are readable back by the submitting process. Each backend blocks correctly (serial foreground; tmux headless poll via monitor='none'; slurm via the inline monitor). - slurm: fix the inverted is_available() guard in two --run doctests (`if not self.is_available(): self.run()` -> `if self.is_available()`). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 3 + cmd_queue/backends/slurm.py | 4 +- tests/test_backend_execution.py | 169 ++++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 2 deletions(-) create mode 100644 tests/test_backend_execution.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 081a08e..2285d40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added: * First-class job `setup` / `teardown` lifecycle on `BashJob` (serial/tmux) and `SlurmJob`. `setup` is a gating precondition (shares the preamble's `PREAMBLE_OK` gating; a failing setup skips the command and marks the job failed). `teardown` always runs after the command — on success, failure, and SIGINT/SIGTERM — provided setup succeeded. It is rendered as a per-job, signal-safe cleanup (a scoped subshell trap for serial/tmux so it cannot leak across the many jobs in one script; an in-`--wrap` trap for slurm). The main command's exit code stays authoritative (a teardown failure does not flip the job result). A hard SIGKILL cannot be trapped — an out-of-band reclaim (e.g. a lease TTL) is the only backstop for that. This is the job-level try/finally for bracketing an external resource (e.g. acquire/release a GPU lease). +* Backend execution tests (`tests/test_backend_execution.py`) that actually run a queue — simple DAGs and the `setup`/`teardown` lifecycle — on the serial backend, and on tmux/slurm when those backends report themselves available (skipped otherwise). + ### Fixed: * A list-valued `preamble` passed to `SlurmJob` (e.g. `submit(..., preamble=['a', 'b'])`) no longer crashes script construction; list steps are now flattened into the `&&` chain instead of being appended as a single element. +* Corrected the inverted ``is_available()`` guard in two ``SlurmQueue`` ``--run`` doctests (they read `if not self.is_available(): self.run()`, which would only submit when slurm was *un*available). ## Version 0.3.0 - Released 2026-05-21 diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index edb7dda..3c1810e 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -513,7 +513,7 @@ class SlurmQueue(base_queue.Queue): >>> self.write() >>> self.print_commands() >>> # xdoctest: +REQUIRES(--run) - >>> if not self.is_available(): + >>> if self.is_available(): >>> self.run() Example: @@ -530,7 +530,7 @@ class SlurmQueue(base_queue.Queue): >>> self.write() >>> self.print_commands() >>> # xdoctest: +REQUIRES(--run) - >>> if not self.is_available(): + >>> if self.is_available(): >>> self.run() Example: diff --git a/tests/test_backend_execution.py b/tests/test_backend_execution.py new file mode 100644 index 0000000..c8e4f9b --- /dev/null +++ b/tests/test_backend_execution.py @@ -0,0 +1,169 @@ +""" +End-to-end execution tests that actually **run** a queue on each backend and +verify that jobs -- and their ``setup`` / ``teardown`` lifecycle -- executed. + +Unlike the render-only tests (which only build the bash/sbatch text), these +submit and run a real queue: + +* ``serial`` always runs (no external dependencies). +* ``tmux`` and ``slurm`` are skipped unless the backend reports itself + available via ``is_available()``. So on a machine with a working slurm + install these exercise real ``sbatch`` submission; elsewhere they skip + cleanly. + +Each job (and its setup/teardown) writes a marker file, so execution is +verified by the side effects on disk -- a backend-agnostic check. + +Working directories use :func:`ubelt.Path.appdir` (under the user's home) +rather than pytest's ``tmp_path``: for slurm the job body runs on a compute +node, and the marker files must land somewhere the submitting process can +read them back. ``$HOME`` is shared across nodes on a typical cluster, +whereas ``/tmp`` is often node-local. +""" +from __future__ import annotations + +import pytest +import ubelt as ub + +import cmd_queue + +# Computed once: which backends can actually run on this machine. +_AVAILABLE = set(cmd_queue.Queue.available_backends()) + + +def _work_dpath(slug: str) -> ub.Path: + """A clean, shared-filesystem working directory for one test.""" + dpath = ub.Path.appdir('cmd_queue/tests/backend_execution') / slug + dpath.delete().ensuredir() + return dpath + + +def _backend_param(name: str): + return pytest.param( + name, + marks=pytest.mark.skipif( + name not in _AVAILABLE, + reason=f'{name} backend is not available on this machine', + ), + ) + + +BACKENDS = [ + _backend_param('serial'), + _backend_param('tmux'), + _backend_param('slurm'), +] + + +def _make_queue(backend: str, name: str, dpath: ub.Path): + """Construct a queue with the per-backend kwargs each one expects.""" + kwargs: dict = {'backend': backend, 'name': name} + if backend in {'serial', 'tmux'}: + kwargs['dpath'] = dpath + kwargs['rootid'] = 'test' + if backend == 'tmux': + kwargs['size'] = 1 + return cmd_queue.Queue.create(**kwargs) + + +def _run_blocking(queue, backend: str) -> None: + """Run the queue and block until every job reaches a terminal state. + + The blocking mechanism differs per backend: + * serial runs the script in the foreground (inherently blocking); + * tmux blocks via a headless state-file poll (``monitor='none'``); + * slurm blocks by polling ``scontrol`` in the inline monitor. + """ + try: + if backend == 'tmux': + queue.run( + block=True, + monitor='none', + with_textual=False, + onfail='', + other_session_handler='ignore', + ) + elif backend == 'slurm': + queue.run(block=True, monitor='inline', onfail='') + else: + queue.run(block=True, verbose=0) + finally: + # Best-effort cleanup so a failure never leaves tmux sessions or + # queued slurm jobs lingering for the next test. + kill = getattr(queue, 'kill', None) + if callable(kill): + try: + kill() + except Exception: + pass + + +@pytest.mark.parametrize('backend', BACKENDS) +def test_backend_executes_simple_dag(backend): + """A two-job dependent DAG runs to completion and produces its output.""" + dpath = _work_dpath(f'simple-{backend}') + queue = _make_queue(backend, 'cmdq-exec-simple', dpath / 'qdir') + + out1 = dpath / 'job1.out' + out2 = dpath / 'job2.out' + job1 = queue.submit(f'echo hi > "{out1}"', name='job1') + queue.submit(f'echo done > "{out2}"', name='job2', depends=[job1]) + + _run_blocking(queue, backend) + + assert out1.exists(), 'first job did not run' + assert out2.exists(), 'dependent job did not run' + assert out1.read_text().strip() == 'hi' + assert out2.read_text().strip() == 'done' + + +@pytest.mark.parametrize('backend', BACKENDS) +def test_backend_executes_setup_teardown(backend): + """setup runs before the command and teardown runs after it (success).""" + dpath = _work_dpath(f'setup-teardown-{backend}') + queue = _make_queue(backend, 'cmdq-exec-st', dpath / 'qdir') + + setup_marker = dpath / 'setup.marker' + cmd_marker = dpath / 'cmd.marker' + teardown_marker = dpath / 'teardown.marker' + + queue.submit( + f'echo cmd > "{cmd_marker}"', + name='bracketed', + setup=f'echo s > "{setup_marker}"', + teardown=f'echo t > "{teardown_marker}"', + ) + + _run_blocking(queue, backend) + + assert setup_marker.exists(), 'setup should run before the command' + assert cmd_marker.exists(), 'command should run after a successful setup' + assert teardown_marker.exists(), 'teardown should run after the command' + + +@pytest.mark.parametrize('backend', BACKENDS) +def test_backend_setup_failure_skips_command_and_teardown(backend): + """A failing setup gates the command (skipped) and teardown (not run). + + setup is the resource acquisition: if it fails the resource was never + acquired, so neither the command nor the release (teardown) should run. + """ + dpath = _work_dpath(f'setup-fail-{backend}') + queue = _make_queue(backend, 'cmdq-exec-stfail', dpath / 'qdir') + + cmd_marker = dpath / 'cmd.marker' + teardown_marker = dpath / 'teardown.marker' + + queue.submit( + f'echo cmd > "{cmd_marker}"', + name='bracketed', + setup='false', # gating precondition fails + teardown=f'echo t > "{teardown_marker}"', + ) + + _run_blocking(queue, backend) + + assert not cmd_marker.exists(), 'command must be skipped when setup fails' + assert not teardown_marker.exists(), ( + 'teardown must not run when setup never succeeded' + ) From 8378ece8b1a17f3a0a51cf26fe8faf04a196854a Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Wed, 24 Jun 2026 15:30:20 -0400 Subject: [PATCH 11/31] Assert setup failure fails the job in the execution test The execution-level setup-failure test only checked that the command and teardown were skipped; also assert the job is marked failed (serial/tmux record this in the on-disk fail marker). slurm tracks job state through the scheduler, so its "setup failure exits non-zero" stays covered by test_slurm_variants.py. (BashJob-level coverage already exists in test_bash_variants.py.) Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_backend_execution.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/tests/test_backend_execution.py b/tests/test_backend_execution.py index c8e4f9b..39dc19f 100644 --- a/tests/test_backend_execution.py +++ b/tests/test_backend_execution.py @@ -142,11 +142,13 @@ def test_backend_executes_setup_teardown(backend): @pytest.mark.parametrize('backend', BACKENDS) -def test_backend_setup_failure_skips_command_and_teardown(backend): - """A failing setup gates the command (skipped) and teardown (not run). +def test_backend_setup_failure_fails_job_and_skips_command(backend): + """A failing setup marks the job failed and gates the command (skipped) + and teardown (not run). setup is the resource acquisition: if it fails the resource was never - acquired, so neither the command nor the release (teardown) should run. + acquired, so the job fails and neither the command nor the release + (teardown) should run. """ dpath = _work_dpath(f'setup-fail-{backend}') queue = _make_queue(backend, 'cmdq-exec-stfail', dpath / 'qdir') @@ -154,7 +156,7 @@ def test_backend_setup_failure_skips_command_and_teardown(backend): cmd_marker = dpath / 'cmd.marker' teardown_marker = dpath / 'teardown.marker' - queue.submit( + job = queue.submit( f'echo cmd > "{cmd_marker}"', name='bracketed', setup='false', # gating precondition fails @@ -167,3 +169,10 @@ def test_backend_setup_failure_skips_command_and_teardown(backend): assert not teardown_marker.exists(), ( 'teardown must not run when setup never succeeded' ) + # The failing setup must mark the job failed (not passed). serial/tmux + # record this in on-disk pass/fail markers; slurm tracks job state through + # the scheduler (no marker), and its "setup failure exits non-zero" is + # covered by test_slurm_variants.py. + if backend != 'slurm': + assert job.fail_fpath.exists(), 'a failing setup must fail the job' + assert not job.pass_fpath.exists(), 'a failed job must not pass' From e48885f2c84f6fc23cc1fa1e8535024b89af500e Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Wed, 24 Jun 2026 16:21:54 -0400 Subject: [PATCH 12/31] Add dev/slurm toolkit for testing the slurm backend Stand up a single-node Slurm cluster so the slurm backend execution tests (tests/test_backend_execution.py) actually run instead of skipping. Two paths: * Docker (run_slurm_tests_in_docker.sh): builds an image that boots munge + MariaDB + slurmdbd + slurmctld + slurmd, registers an accounting association, and runs the tests against the working tree. Verified: all 9 slurm tests pass, including the real sbatch-executing ones. * Native (setup_slurm_local.sh): the same recipe via the host's systemd. Notable details captured in the scripts/README: * accounting_storage/none is deprecated in Slurm 23.11, so a local slurmdbd + MariaDB association is required or every job is rejected with Reason=InvalidAccount. * cgroup v2 delegation (cgroup_prep.sh) + IgnoreSystemd=yes lets slurmd start inside a --privileged container. * the munge socket dir must be 0755 so the slurm user can authenticate. Config is templated (slurm.conf.tmpl, slurmdbd.conf.tmpl) and shared by both paths to keep them in lockstep. Co-Authored-By: Claude Opus 4.8 (1M context) --- dev/slurm/Dockerfile | 47 +++++++ dev/slurm/README.md | 71 ++++++++++ dev/slurm/cgroup_prep.sh | 46 +++++++ dev/slurm/entrypoint.sh | 111 ++++++++++++++++ dev/slurm/render_slurm_conf.sh | 52 ++++++++ dev/slurm/run_slurm_tests_in_docker.sh | 45 +++++++ dev/slurm/setup_slurm_local.sh | 174 +++++++++++++++++++++++++ dev/slurm/slurm.conf.tmpl | 47 +++++++ dev/slurm/slurmdbd.conf.tmpl | 23 ++++ 9 files changed, 616 insertions(+) create mode 100644 dev/slurm/Dockerfile create mode 100644 dev/slurm/README.md create mode 100755 dev/slurm/cgroup_prep.sh create mode 100755 dev/slurm/entrypoint.sh create mode 100755 dev/slurm/render_slurm_conf.sh create mode 100755 dev/slurm/run_slurm_tests_in_docker.sh create mode 100755 dev/slurm/setup_slurm_local.sh create mode 100644 dev/slurm/slurm.conf.tmpl create mode 100644 dev/slurm/slurmdbd.conf.tmpl diff --git a/dev/slurm/Dockerfile b/dev/slurm/Dockerfile new file mode 100644 index 0000000..c59dea6 --- /dev/null +++ b/dev/slurm/Dockerfile @@ -0,0 +1,47 @@ +# Single-node Slurm cluster in a container, for exercising cmd_queue's slurm +# backend end-to-end (sbatch/squeue/scontrol) without touching the host. +# +# Build + run via ``dev/slurm/run_slurm_tests_in_docker.sh`` (it mounts the +# repo and runs pytest), or by hand: +# +# docker build -f dev/slurm/Dockerfile -t cmd_queue-slurm dev/slurm +# docker run --rm -it --privileged -v "$PWD:/io" cmd_queue-slurm \ +# bash -lc 'cd /io && pip install -e . && pytest tests/test_backend_execution.py -k slurm' +# +# --privileged is required so the entrypoint can delegate cgroup v2 +# controllers for slurmd (see cgroup_prep.sh). +FROM ubuntu:24.04 + +ENV DEBIAN_FRONTEND=noninteractive + +# slurm-wlm pulls in slurmctld/slurmd/sbatch; slurmdbd + mariadb-server back +# the accounting database (required since accounting_storage/none was +# deprecated in 23.11); munge is slurm's auth layer. +RUN apt-get update \ + && apt-get install -y --no-install-recommends \ + slurm-wlm \ + slurmdbd \ + mariadb-server \ + munge \ + python3 \ + python3-pip \ + python3-venv \ + git \ + ca-certificates \ + procps \ + && rm -rf /var/lib/apt/lists/* + +# Spool/state/log dirs the daemons need, owned by the distro 'slurm' user. +RUN mkdir -p /var/spool/slurm/ctld /var/spool/slurm/d /var/log/slurm \ + && chown -R slurm:slurm /var/spool/slurm /var/log/slurm + +COPY slurm.conf.tmpl slurmdbd.conf.tmpl render_slurm_conf.sh cgroup_prep.sh entrypoint.sh \ + /opt/slurm-setup/ +RUN chmod +x /opt/slurm-setup/render_slurm_conf.sh \ + /opt/slurm-setup/cgroup_prep.sh \ + /opt/slurm-setup/entrypoint.sh + +# The entrypoint boots munge + slurmctld + slurmd, waits for the node to go +# IDLE, then exec's whatever command you pass (default: an interactive shell). +ENTRYPOINT ["/opt/slurm-setup/entrypoint.sh"] +CMD ["bash"] diff --git a/dev/slurm/README.md b/dev/slurm/README.md new file mode 100644 index 0000000..2dadb08 --- /dev/null +++ b/dev/slurm/README.md @@ -0,0 +1,71 @@ +# Single-node Slurm for testing the slurm backend + +cmd_queue's slurm backend is normally untested on dev machines: the execution +tests in `tests/test_backend_execution.py` (and `SlurmQueue.is_available()`) +**skip** unless a working slurm controller + compute node is reachable. This +folder stands up a tiny one-node cluster so those tests actually run. + +Two ways to use it: + +| Path | Isolation | Use when | +| --- | --- | --- | +| **Docker** (`run_slurm_tests_in_docker.sh`) | full | you just want to run the slurm tests; recommended | +| **Native** (`setup_slurm_local.sh`) | none — modifies this host | you want slurm running directly on the VM | + +## Docker (recommended) + +Builds an image that boots munge + MariaDB + slurmdbd + slurmctld + slurmd, +registers an accounting association, then runs the tests against your current +working tree (mounted at `/io`): + +```bash +dev/slurm/run_slurm_tests_in_docker.sh # run the slurm tests +dev/slurm/run_slurm_tests_in_docker.sh shell # interactive shell in the cluster +dev/slurm/run_slurm_tests_in_docker.sh pytest -k slurm # arbitrary pytest invocation +``` + +It runs the container `--privileged` so the entrypoint can delegate cgroup v2 +controllers to slurmd (see `cgroup_prep.sh`). Inside the container: + +```bash +sinfo # node should be 'idle' +sbatch --wrap 'echo hi' # submits and runs +``` + +## Native (this VM) + +```bash +dev/slurm/setup_slurm_local.sh # install + configure + start (uses sudo) +dev/slurm/setup_slurm_local.sh status # sinfo / squeue / associations +dev/slurm/setup_slurm_local.sh stop # stop the daemons +dev/slurm/setup_slurm_local.sh teardown # stop + remove the slurm config +``` + +This installs and starts system services (slurm, slurmdbd, munge, MariaDB) on +the host — it is meaningfully invasive. It registers an association for the +user who invoked `sudo` (override with `SLURM_ASSOC_USER=...`). After setup: + +```bash +sinfo +python -m pytest tests/test_backend_execution.py -v -k slurm +``` + +## Why slurmdbd + MariaDB? + +Slurm 23.11 (Ubuntu 24.04) deprecated `accounting_storage/none`. Without a +slurmdbd association, every submitted job is rejected with +`Reason=InvalidAccount` and never runs. The smallest reliable single-node +setup therefore includes a local MariaDB + slurmdbd and one registered +`default` account / user association. + +## Files + +| File | Purpose | +| --- | --- | +| `slurm.conf.tmpl` | single-node `slurm.conf` template (shared) | +| `slurmdbd.conf.tmpl` | `slurmdbd.conf` template (shared) | +| `render_slurm_conf.sh` | fills the slurm.conf template for the local host | +| `cgroup_prep.sh` | cgroup v2 delegation for slurmd inside a container | +| `Dockerfile` / `entrypoint.sh` | the containerised cluster | +| `run_slurm_tests_in_docker.sh` | build the image + run the tests | +| `setup_slurm_local.sh` | native (systemd) install/configure/start | diff --git a/dev/slurm/cgroup_prep.sh b/dev/slurm/cgroup_prep.sh new file mode 100755 index 0000000..fd43a44 --- /dev/null +++ b/dev/slurm/cgroup_prep.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash +# +# Prepare cgroup v2 for slurmd *inside a container*. Slurm 23.11's slurmd +# always initializes a cgroup plugin; in a stock container the cgroup2 root +# has no delegated controllers and no ``system.slice``, so slurmd dies with +# "cannot create cgroup context for cgroup/v2". +# +# This does the standard cgroup-v2 delegation dance: +# 1. move every process out of the cgroup root into a leaf (the no-internal- +# process rule forbids enabling controllers while procs live in the root); +# 2. delegate all available controllers into the root's subtree; +# 3. create the ``system.slice`` that slurm (with IgnoreSystemd=yes) expects +# to create its ``_slurmstepd.scope`` under. +# +# Pair this with a ``cgroup.conf`` containing ``IgnoreSystemd=yes`` (so slurm +# manages cgroups directly instead of asking a non-existent systemd/dbus). +# +# Requires a writable cgroup2 fs -- run the container with ``--privileged`` +# (or equivalent cgroup delegation). On a native systemd host none of this is +# needed (systemd handles delegation), so this script is a no-op there. +set -euo pipefail + +CG=/sys/fs/cgroup + +if [[ ! -f "$CG/cgroup.controllers" ]]; then + echo "[cgroup_prep] no cgroup2 at $CG (or already prepared); skipping" + exit 0 +fi + +# 1. Move all processes into a leaf so the root has no internal processes. +mkdir -p "$CG/init" +while read -r pid; do + echo "$pid" > "$CG/init/cgroup.procs" 2>/dev/null || true +done < "$CG/cgroup.procs" + +# 2. Delegate every controller the kernel exposes into the subtree. +add="" +for c in $(cat "$CG/cgroup.controllers"); do + add="$add +$c" +done +echo "$add" > "$CG/cgroup.subtree_control" 2>/dev/null || true + +# 3. Pre-create the slice slurm's stepd scope is nested under. +mkdir -p "$CG/system.slice" + +echo "[cgroup_prep] delegated controllers:$(cat "$CG/cgroup.subtree_control")" diff --git a/dev/slurm/entrypoint.sh b/dev/slurm/entrypoint.sh new file mode 100755 index 0000000..d880817 --- /dev/null +++ b/dev/slurm/entrypoint.sh @@ -0,0 +1,111 @@ +#!/usr/bin/env bash +# +# Container entrypoint: bring up a complete one-node slurm cluster +# (munge + MariaDB + slurmdbd + slurmctld + slurmd, with a registered +# accounting association), then exec the command passed to ``docker run`` +# (default: an interactive bash shell). +# +# Runs as root inside the container, so no sudo is needed. Must be run in a +# container with writable cgroups (``--privileged``); see cgroup_prep.sh. +set -euo pipefail + +SETUP_DIR=/opt/slurm-setup +HOST="$(hostname -s)" +DB_PASS="${SLURM_DB_PASS:-slurmpw}" +# Users that should own a slurm association (so their jobs aren't rejected +# with InvalidAccount). Whoever runs the tests must be in this list; tests in +# the image run as root. +SLURM_ASSOC_USERS="${SLURM_ASSOC_USERS:-root}" + +log() { echo "[entrypoint] $*"; } + +# --- cgroup v2 delegation (no-op on a native systemd host) ----------------- +bash "$SETUP_DIR/cgroup_prep.sh" + +# --- munge (slurm's auth layer) -------------------------------------------- +log "starting munge..." +install -d -m 0755 -o munge -g munge /run/munge # 0755 so the slurm user can reach the socket +install -d -m 0700 -o munge -g munge /etc/munge /var/log/munge +if [[ ! -f /etc/munge/munge.key ]]; then + dd if=/dev/urandom bs=1 count=1024 of=/etc/munge/munge.key 2>/dev/null + chown munge:munge /etc/munge/munge.key + chmod 0400 /etc/munge/munge.key +fi +runuser -u munge -- /usr/sbin/munged --force + +# --- MariaDB (slurmdbd's backing store) ------------------------------------ +log "starting MariaDB..." +install -d -o mysql -g mysql /run/mysqld /var/lib/mysql +if [[ ! -d /var/lib/mysql/mysql ]]; then + mariadb-install-db --user=mysql --datadir=/var/lib/mysql >/dev/null 2>&1 +fi +runuser -u mysql -- /usr/sbin/mariadbd --datadir=/var/lib/mysql \ + >/var/log/mariadb.log 2>&1 & +for _ in $(seq 1 30); do + mysqladmin ping >/dev/null 2>&1 && break + sleep 1 +done +mysql < /etc/slurm/slurmdbd.conf +chown slurm:slurm /etc/slurm/slurmdbd.conf +chmod 0600 /etc/slurm/slurmdbd.conf +# Inside a container slurm must manage cgroups itself (no systemd/dbus). +printf 'CgroupPlugin=autodetect\nIgnoreSystemd=yes\n' > /etc/slurm/cgroup.conf + +# --- slurmdbd -------------------------------------------------------------- +log "starting slurmdbd..." +slurmdbd +for _ in $(seq 1 20); do + sacctmgr -i show cluster >/dev/null 2>&1 && break + sleep 1 +done + +# --- slurmctld + slurmd ---------------------------------------------------- +log "starting slurmctld + slurmd..." +slurmctld +sleep 1 +# Launch slurmd from inside system.slice so its stepd scope lands in a +# delegated, controller-enabled cgroup. +echo $$ > /sys/fs/cgroup/system.slice/cgroup.procs 2>/dev/null || true +slurmd + +# --- register the accounting association ----------------------------------- +# slurmctld auto-registers the cluster with slurmdbd; we just add an account +# and the user(s) that will submit jobs. +log "registering accounting association for: ${SLURM_ASSOC_USERS}" +sacctmgr -i add account default Cluster=cmdq \ + Description="cmd_queue test account" 2>/dev/null || true +for u in $SLURM_ASSOC_USERS; do + sacctmgr -i add user "$u" Account=default Cluster=cmdq 2>/dev/null || true +done + +# --- wait for the node to be usable so the first sbatch doesn't race boot -- +log "waiting for node to become available..." +for _ in $(seq 1 30); do + if sinfo -h -o '%T' 2>/dev/null | grep -qE 'idle|mixed|alloc'; then + break + fi + scontrol update nodename="$HOST" state=RESUME 2>/dev/null || true + sleep 1 +done + +sinfo || true +log "cluster up; exec: $*" +exec "$@" diff --git a/dev/slurm/render_slurm_conf.sh b/dev/slurm/render_slurm_conf.sh new file mode 100755 index 0000000..4c3e3e0 --- /dev/null +++ b/dev/slurm/render_slurm_conf.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# +# Render ``slurm.conf.tmpl`` into a concrete ``slurm.conf`` for the machine +# this runs on. Shared by the native setup script and the Docker entrypoint +# so both produce an identical, correct config. +# +# Usage: +# render_slurm_conf.sh OUTPUT_PATH +# +# Honors these optional environment overrides (sensible defaults otherwise): +# SLURM_NODE_HOST node + control hostname (default: `hostname -s`) +# SLURM_CPUS CPUs the node advertises (default: `nproc`) +# SLURM_REALMEM_MB RealMemory in MB (default: ~85% of total) +# SLURM_STATE_DIR StateSaveLocation (default: /var/spool/slurm/ctld) +# SLURM_SPOOL_DIR SlurmdSpoolDir (default: /var/spool/slurm/d) +# SLURM_RUN_DIR pid file dir (default: /run) +# SLURM_LOG_DIR log dir (default: /var/log/slurm) +set -euo pipefail + +OUT_PATH="${1:?usage: render_slurm_conf.sh OUTPUT_PATH}" +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +TMPL="$HERE/slurm.conf.tmpl" + +host="${SLURM_NODE_HOST:-$(hostname -s)}" +cpus="${SLURM_CPUS:-$(nproc)}" + +if [[ -n "${SLURM_REALMEM_MB:-}" ]]; then + mem="$SLURM_REALMEM_MB" +else + # Advertise ~85% of physical RAM so slurm never flags the node as + # over-reporting memory (which would mark it invalid/down). + total_mb=$(awk '/MemTotal/ {printf "%d", $2/1024}' /proc/meminfo) + mem=$(( total_mb * 85 / 100 )) + [[ "$mem" -lt 64 ]] && mem=64 +fi + +state_dir="${SLURM_STATE_DIR:-/var/spool/slurm/ctld}" +spool_dir="${SLURM_SPOOL_DIR:-/var/spool/slurm/d}" +run_dir="${SLURM_RUN_DIR:-/run}" +log_dir="${SLURM_LOG_DIR:-/var/log/slurm}" + +sed \ + -e "s|@HOSTNAME@|${host}|g" \ + -e "s|@CPUS@|${cpus}|g" \ + -e "s|@MEM@|${mem}|g" \ + -e "s|@STATE_DIR@|${state_dir}|g" \ + -e "s|@SPOOL_DIR@|${spool_dir}|g" \ + -e "s|@RUN_DIR@|${run_dir}|g" \ + -e "s|@LOG_DIR@|${log_dir}|g" \ + "$TMPL" > "$OUT_PATH" + +echo "[render_slurm_conf] wrote $OUT_PATH (host=$host cpus=$cpus mem=${mem}MB)" diff --git a/dev/slurm/run_slurm_tests_in_docker.sh b/dev/slurm/run_slurm_tests_in_docker.sh new file mode 100755 index 0000000..6cc3b5c --- /dev/null +++ b/dev/slurm/run_slurm_tests_in_docker.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +# +# Build the single-node slurm image and run cmd_queue's slurm backend tests +# inside it, against the *current working tree* (mounted read-write at /io). +# +# dev/slurm/run_slurm_tests_in_docker.sh # run the slurm exec tests +# dev/slurm/run_slurm_tests_in_docker.sh shell # drop into a shell in the cluster +# dev/slurm/run_slurm_tests_in_docker.sh pytest -k slurm # run an arbitrary pytest invocation +# +# The image boots munge + slurmctld + slurmd (see entrypoint.sh) before the +# command runs, so ``SlurmQueue.is_available()`` is True inside the container. +set -euo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$HERE/../.." && pwd)" +IMAGE=cmd_queue-slurm + +echo "[run_slurm_tests_in_docker] building $IMAGE ..." +docker build -f "$HERE/Dockerfile" -t "$IMAGE" "$HERE" + +# Install the repo (editable) into the container, then run the requested +# command. Default to the slurm-specific execution tests. +inner_cmd='pytest tests/test_backend_execution.py tests/test_slurm_variants.py -v -k slurm' +if [[ "${1:-}" == "shell" ]]; then + inner_cmd='exec bash' +elif [[ $# -gt 0 ]]; then + inner_cmd="$*" +fi + +echo "[run_slurm_tests_in_docker] running: $inner_cmd" +# --privileged is required so the entrypoint can delegate cgroup v2 controllers +# for slurmd (see cgroup_prep.sh). -t only when attached to a TTY. +tty_flags=(-i) +[[ -t 0 && -t 1 ]] && tty_flags=(-it) +docker run --rm "${tty_flags[@]}" \ + --privileged \ + -v "$REPO_ROOT:/io" \ + -w /io \ + "$IMAGE" \ + bash -lc " + set -e + python3 -m pip install --quiet --break-system-packages -e '.[tests]' 2>/dev/null \ + || python3 -m pip install --quiet --break-system-packages -e . + $inner_cmd + " diff --git a/dev/slurm/setup_slurm_local.sh b/dev/slurm/setup_slurm_local.sh new file mode 100755 index 0000000..6c3b3f0 --- /dev/null +++ b/dev/slurm/setup_slurm_local.sh @@ -0,0 +1,174 @@ +#!/usr/bin/env bash +# +# Set up a single-node Slurm cluster on *this* machine so the slurm backend +# tests actually run instead of skipping. Mirrors the Docker image +# (dev/slurm/Dockerfile) but uses the host's systemd to manage daemons. +# +# Targeted at Debian/Ubuntu (apt + the distro slurm-wlm/slurmdbd packages). +# It is idempotent: re-running re-renders config and restarts daemons. Needs +# root for install/config (re-execs under sudo). +# +# dev/slurm/setup_slurm_local.sh # install + configure + start +# dev/slurm/setup_slurm_local.sh status # show sinfo / squeue / sacctmgr +# dev/slurm/setup_slurm_local.sh stop # stop the slurm daemons +# dev/slurm/setup_slurm_local.sh teardown # stop + remove the slurm config +# +# NOTE: this installs and starts system services (slurm, slurmdbd, munge, +# MariaDB) on the host -- it is meaningfully invasive. Prefer the Docker path +# (run_slurm_tests_in_docker.sh) if you just want to run the tests in +# isolation. ``teardown`` removes the slurm config but intentionally leaves +# the installed packages and MariaDB data in place. +# +# After it finishes, verify with: +# sinfo +# python -c "import cmd_queue; print('slurm' in cmd_queue.Queue.available_backends())" +# python -m pytest tests/test_backend_execution.py -v -k slurm +set -euo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ACTION="${1:-setup}" + +CONF_DIR=/etc/slurm +CONF_PATH="$CONF_DIR/slurm.conf" +DBD_CONF_PATH="$CONF_DIR/slurmdbd.conf" +STATE_DIR=/var/spool/slurm/ctld +SPOOL_DIR=/var/spool/slurm/d +LOG_DIR=/var/log/slurm +DB_PASS="${SLURM_DB_PASS:-slurmpw}" + +# The unprivileged user whose jobs need a slurm association. Defaults to +# whoever invoked sudo (the developer running the tests), falling back to root. +ASSOC_USER="${SLURM_ASSOC_USER:-${SUDO_USER:-root}}" + +need_root() { + if [[ "$(id -u)" -ne 0 ]]; then + echo "[setup_slurm_local] re-executing under sudo..." + exec sudo -E bash "$0" "$@" + fi +} + +install_pkgs() { + if command -v sbatch >/dev/null 2>&1 \ + && command -v slurmdbd >/dev/null 2>&1 \ + && command -v mariadbd >/dev/null 2>&1; then + echo "[setup_slurm_local] slurm + slurmdbd + mariadb already installed" + return + fi + echo "[setup_slurm_local] installing slurm-wlm slurmdbd mariadb-server munge..." + export DEBIAN_FRONTEND=noninteractive + apt-get update -qq + apt-get install -y -qq slurm-wlm slurmdbd mariadb-server munge >/dev/null +} + +setup_munge() { + if [[ ! -f /etc/munge/munge.key ]]; then + echo "[setup_slurm_local] creating munge key..." + install -d -m 0700 -o munge -g munge /etc/munge + dd if=/dev/urandom bs=1 count=1024 >/etc/munge/munge.key 2>/dev/null + chown munge:munge /etc/munge/munge.key + chmod 0400 /etc/munge/munge.key + fi + systemctl enable --now munge >/dev/null 2>&1 || systemctl restart munge +} + +setup_mariadb() { + echo "[setup_slurm_local] configuring MariaDB for slurmdbd..." + systemctl enable --now mariadb >/dev/null 2>&1 || systemctl restart mariadb + # Root authenticates via the unix socket on a stock Ubuntu install, so + # ``mysql`` works without a password while we run as root. + mysql < "$DBD_CONF_PATH" + chown slurm:slurm "$DBD_CONF_PATH" + chmod 0600 "$DBD_CONF_PATH" + # On a native systemd host slurm manages cgroups through systemd, so no + # IgnoreSystemd workaround (and thus no custom cgroup.conf) is needed here. +} + +start_daemons() { + systemctl enable --now slurmdbd >/dev/null 2>&1 || systemctl restart slurmdbd + # Give slurmdbd a moment to come up before slurmctld registers with it. + for _ in $(seq 1 20); do + sacctmgr -i show cluster >/dev/null 2>&1 && break + sleep 1 + done + systemctl enable --now slurmctld slurmd >/dev/null 2>&1 || \ + systemctl restart slurmctld slurmd + sleep 1 + scontrol update nodename="$(hostname -s)" state=RESUME 2>/dev/null || true +} + +register_assoc() { + echo "[setup_slurm_local] registering association for user '$ASSOC_USER'..." + sacctmgr -i add account default Cluster=cmdq \ + Description="cmd_queue test account" 2>/dev/null || true + sacctmgr -i add user "$ASSOC_USER" Account=default Cluster=cmdq 2>/dev/null || true + # root submits during teardown/debugging too; harmless if it already exists. + sacctmgr -i add user root Account=default Cluster=cmdq 2>/dev/null || true +} + +stop_daemons() { + systemctl stop slurmd slurmctld slurmdbd 2>/dev/null || true +} + +case "$ACTION" in + setup) + need_root "$@" + install_pkgs + setup_munge + setup_mariadb + make_dirs + render_conf + start_daemons + register_assoc + echo + echo "[setup_slurm_local] done. Cluster status:" + sinfo || true + echo + echo "Next: python -m pytest tests/test_backend_execution.py -v -k slurm" + ;; + status) + echo "== sinfo =="; sinfo || true + echo "== squeue =="; squeue || true + echo "== sacctmgr =="; sacctmgr show assoc format=Cluster,Account,User 2>/dev/null || true + ;; + stop) + need_root "$@" + stop_daemons + echo "[setup_slurm_local] slurm daemons stopped" + ;; + teardown) + need_root "$@" + stop_daemons + rm -f "$CONF_PATH" "$DBD_CONF_PATH" + echo "[setup_slurm_local] daemons stopped; $CONF_PATH and $DBD_CONF_PATH removed" + echo "[setup_slurm_local] (packages + MariaDB data left in place)" + ;; + *) + echo "usage: $0 [setup|status|stop|teardown]" >&2 + exit 2 + ;; +esac diff --git a/dev/slurm/slurm.conf.tmpl b/dev/slurm/slurm.conf.tmpl new file mode 100644 index 0000000..836b18e --- /dev/null +++ b/dev/slurm/slurm.conf.tmpl @@ -0,0 +1,47 @@ +# cmd_queue single-node Slurm config template. +# +# This is rendered into a real ``slurm.conf`` by ``render_slurm_conf.sh``, +# which substitutes the @PLACEHOLDER@ tokens with values discovered on the +# host (or container) it is running on. The same template is shared by the +# native setup (``setup_slurm_local.sh``) and the Docker image so the two +# stay in lockstep. +# +# The goal is the *smallest* config that makes ``SlurmQueue.is_available()`` +# return True and lets ``sbatch`` actually run jobs on one machine: +# * proctrack/linuxproc + task/none keeps process tracking off cgroups. +# * accounting_storage/slurmdbd is REQUIRED: ``accounting_storage/none`` is +# deprecated as of Slurm 23.11, and without a slurmdbd association every +# job is rejected with ``Reason=InvalidAccount`` and never runs. The boot +# scripts stand up a tiny MariaDB + slurmdbd and register an association. +# * ReturnToService=2 brings the node back UP automatically after a reboot +# even if it was previously marked DOWN. +ClusterName=cmdq +SlurmctldHost=@HOSTNAME@ + +SlurmUser=slurm +StateSaveLocation=@STATE_DIR@ +SlurmdSpoolDir=@SPOOL_DIR@ +SlurmctldPidFile=@RUN_DIR@/slurmctld.pid +SlurmdPidFile=@RUN_DIR@/slurmd.pid +SlurmctldLogFile=@LOG_DIR@/slurmctld.log +SlurmdLogFile=@LOG_DIR@/slurmd.log + +# Keep the footprint tiny and cgroup-free for process tracking. +ProctrackType=proctrack/linuxproc +TaskPlugin=task/none +MpiDefault=none +SwitchType=switch/none + +SchedulerType=sched/backfill +SelectType=select/cons_tres +SelectTypeParameters=CR_Core +ReturnToService=2 + +# Accounting via slurmdbd (see note above). Host is local to this machine. +AccountingStorageType=accounting_storage/slurmdbd +AccountingStorageHost=localhost + +# One node, one partition. RealMemory is deliberately a little below the +# physical total so slurm never marks the node invalid for over-reporting. +NodeName=@HOSTNAME@ CPUs=@CPUS@ RealMemory=@MEM@ State=UNKNOWN +PartitionName=debug Nodes=ALL Default=YES MaxTime=INFINITE State=UP diff --git a/dev/slurm/slurmdbd.conf.tmpl b/dev/slurm/slurmdbd.conf.tmpl new file mode 100644 index 0000000..f394b5e --- /dev/null +++ b/dev/slurm/slurmdbd.conf.tmpl @@ -0,0 +1,23 @@ +# Minimal slurmdbd configuration for the cmd_queue test cluster. +# +# slurmdbd is the accounting daemon. Slurm 23.11 deprecated +# ``accounting_storage/none``, so a working slurmdbd backed by MariaDB is the +# supported way to give jobs a valid association (otherwise every job is +# rejected with ``Reason=InvalidAccount``). +# +# Rendered by substituting @STORAGE_PASS@ (the DB password the boot script +# also configures in MariaDB). Everything else is fixed for a localhost, +# single-node setup. +DbdHost=localhost +DbdPort=6819 +SlurmUser=slurm + +StorageType=accounting_storage/mysql +StorageHost=localhost +StoragePort=3306 +StorageUser=slurm +StoragePass=@STORAGE_PASS@ +StorageLoc=slurm_acct_db + +LogFile=@LOG_DIR@/slurmdbd.log +PidFile=@RUN_DIR@/slurmdbd.pid From 05d13380198497ac3c0ef28b9b8106aeb355c7cf Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 10:22:34 -0400 Subject: [PATCH 13/31] Make backend examples runnable; add serial example The slurm example hardcoded placeholder cluster credentials (partition='project123', account='user123') and fake commands, so it could not run on a real scheduler. Rework it to mirror the tmux example: a real sleep-based DAG, injectable failures, monitor-mode selection, an is_available() guard, and a --dry flag. partition/account are now optional CLI flags that are omitted when unset, letting the example run on the cluster's default partition (including a vanilla local install). Add a self-contained serial example as the level-1 baseline using the same DAG so serial/tmux/slurm can be compared directly. Pin the monitor mode option to type=str in the slurm and tmux examples: scriptconfig otherwise smartcasts --mode=none into Python None, which fails the choices check. Co-Authored-By: Claude Opus 4.8 (1M context) --- examples/serial_example.py | 179 ++++++++++++++++++++++++ examples/slurm_example.py | 273 +++++++++++++++++++++++++++++++++---- examples/tmux_example.py | 1 + 3 files changed, 429 insertions(+), 24 deletions(-) create mode 100644 examples/serial_example.py diff --git a/examples/serial_example.py b/examples/serial_example.py new file mode 100644 index 0000000..8bec247 --- /dev/null +++ b/examples/serial_example.py @@ -0,0 +1,179 @@ +""" +The simplest backend: ``backend='serial'``. + +The serial backend writes the DAG to a single bash script and runs the +jobs one at a time in topological order, in the current process. No +tmux, no scheduler, nothing to install -- it works anywhere. That makes +it the natural "level 1" of the serial -> tmux -> slurm progression: the +same queue API you use here scales out unchanged to the other backends. + +Because everything runs sequentially, the total runtime is the *sum* of +every job's duration (~50s for the demo DAG below), whereas the tmux and +slurm backends run independent branches in parallel. Watching the serial +run is a good way to feel why the parallel backends exist. + +The job DAG has four logical levels (identical to the tmux/slurm +examples so the three can be compared directly). Each logical job is +split into a serial chain of smaller one-second jobs. + + Level 1 (prep): prep-A prep-B prep-C prep-D + Level 2 (process): proc-A proc-B proc-C proc-D (each after one prep) + Level 3 (merge): merge-X (after proc-A + proc-B) + merge-Y (after proc-C + proc-D) + Level 4 (finalize): final (after both merges) + +By default one of the proc jobs is forced to fail so the failure +summary (and dependency-skip cascade) is visible. Pass ``--failures=0`` +for a clean run, or higher numbers for more failures. + +CommandLine: + # Run the demo DAG serially + python ~/code/cmd_queue/examples/serial_example.py + + # Force a clean run (no injected failures) + python ~/code/cmd_queue/examples/serial_example.py --failures=0 +""" + +import scriptconfig as scfg +import ubelt as ub + + +class SerialExampleConfig(scfg.DataConfig): + """ + Command line options for the serial backend example. + """ + + name = scfg.Value( + 'serial-example', + help='Queue name.', + ) + failures = scfg.Value( + 1, + type=int, + help=ub.paragraph( + """ + Number of proc-* logical jobs to force into failure (0-4). + The failures cascade: dependent merge/final jobs are skipped. + """ + ), + ) + logs = scfg.Value( + True, + isflag=True, + help=ub.paragraph( + """ + Set to False to disable per-job log capture (default: enabled). + """ + ), + ) + + +def main(): + import cmd_queue + + args = SerialExampleConfig.cli() + + queue = cmd_queue.Queue.create( + backend='serial', + name=args.name, + ) + + proc_names = ['proc-A', 'proc-B', 'proc-C', 'proc-D'] + fail_set = set(proc_names[: max(0, min(args.failures, len(proc_names)))]) + + submit_kw = {'log': args.logs} + + def submit_sleep_chain(base_name, total_sleep, depends=None, fail=False): + """ + Submit a logical sleep job as a chain of smaller queue jobs. + + This keeps the logical runtime roughly equal to ``total_sleep``, + but gives the queue more individual jobs to track. + + Example: + ``submit_sleep_chain('prep-A', 5)`` creates: + + prep-A-01 -> prep-A-02 -> prep-A-03 -> prep-A-04 -> prep-A-05 + + Each part sleeps for one second, so the total duration is still + about five seconds, plus a small amount of scheduling overhead. + """ + if total_sleep <= 0: + raise ValueError('total_sleep must be positive') + + prev_depends = list(depends or []) + last_job = None + + for idx in range(total_sleep): + part = idx + 1 + name = f'{base_name}-{part:02d}' + is_final_part = part == total_sleep + + cmd = f'echo "[{name}] start"; sleep 1; ' + + if is_final_part and fail: + cmd += f'echo "[{base_name}] FORCED FAILURE" >&2; exit 1' + elif is_final_part: + cmd += f'echo "[{base_name}] done"' + else: + cmd += f'echo "[{name}] done"' + + last_job = queue.submit( + cmd, + name=name, + depends=prev_depends, + **submit_kw, + ) + prev_depends = [last_job] + + return last_job + + # Level 1: four independent prep jobs. With a parallel backend these + # would run concurrently; serially they run back-to-back. + prep_a = submit_sleep_chain('prep-A', 5) + prep_b = submit_sleep_chain('prep-B', 7) + prep_c = submit_sleep_chain('prep-C', 6) + prep_d = submit_sleep_chain('prep-D', 8) + + # Level 2: each process job depends on exactly one prep job; some + # may be forced to fail by --failures. + proc_a = submit_sleep_chain( + 'proc-A', 3, depends=[prep_a], fail='proc-A' in fail_set + ) + proc_b = submit_sleep_chain( + 'proc-B', 4, depends=[prep_b], fail='proc-B' in fail_set + ) + proc_c = submit_sleep_chain( + 'proc-C', 5, depends=[prep_c], fail='proc-C' in fail_set + ) + proc_d = submit_sleep_chain( + 'proc-D', 3, depends=[prep_d], fail='proc-D' in fail_set + ) + + # Level 3: two merge jobs, each waiting on a pair of proc jobs. + merge_x = submit_sleep_chain('merge-X', 4, depends=[proc_a, proc_b]) + merge_y = submit_sleep_chain('merge-Y', 3, depends=[proc_c, proc_d]) + + # Level 4: single finalize job -- the whole pipeline converges here. + submit_sleep_chain('final', 2, depends=[merge_x, merge_y]) + + queue.print_graph() + + if not queue.is_available(): + raise SystemExit('serial backend not available on this machine') + + print( + f'\nRunning serially: failures={args.failures}, logs={args.logs}\n' + ) + + result = queue.run(block=True) + + print(f'\nrun() returned: {result}') + + +if __name__ == '__main__': + """ + CommandLine: + python ~/code/cmd_queue/examples/serial_example.py + """ + main() diff --git a/examples/slurm_example.py b/examples/slurm_example.py index 0d34cb2..4916471 100644 --- a/examples/slurm_example.py +++ b/examples/slurm_example.py @@ -1,40 +1,265 @@ -def main(): - import ubelt as ub +""" +Submitting the demo DAG to a real slurm scheduler (``backend='slurm'``). - import cmd_queue +This is the "level 3" backend: cmd_queue converts the job DAG into +``sbatch`` submissions (with ``--dependency`` edges) and lets slurm +schedule them across the cluster. The same queue API used by the serial +and tmux examples is reused here unchanged -- only the backend and a few +scheduler-specific options differ. - queue = cmd_queue.Queue.create( - backend='slurm', partition='project123', account='user123', ntasks=1 - ) +Partition / account +------------------- +On a shared cluster you typically must route jobs to a specific +partition and bill them to an account, e.g.:: + + python ~/code/cmd_queue/examples/slurm_example.py \ + --partition=general --account=my_project + +When ``--partition``/``--account`` are omitted (the default) the options +are simply left off the ``sbatch`` command, so slurm uses the cluster's +default partition and no accounting. That is what lets this example run +as-is on a vanilla single-node slurm install (which usually exposes a +default ``debug`` partition). + +Monitoring +---------- +Like the tmux example, ``--mode`` selects where the live status UI runs: + + * ``hybrid`` (default) -- inline table in this shell *and* an + attachable detached tmux monitor session. + * ``inline`` -- in-shell live UI only. + * ``tmux`` -- detached tmux monitor session only. + * ``none`` -- headless; ``run()`` blocks until jobs finish. + +The job DAG has four logical levels (identical to the serial/tmux +examples so the three can be compared directly). Each logical job is +split into a serial chain of smaller one-second jobs. + + Level 1 (prep): prep-A prep-B prep-C prep-D (parallel) + Level 2 (process): proc-A proc-B proc-C proc-D (each after one prep) + Level 3 (merge): merge-X (after proc-A + proc-B) + merge-Y (after proc-C + proc-D) (parallel) + Level 4 (finalize): final (after both merges) + +By default one of the proc jobs is forced to fail so the failure +summary (and dependency-skip cascade) is visible. Pass ``--failures=0`` +for a clean run, or higher numbers for more failures. - job1 = queue.submit( - ub.codeblock( +CommandLine: + # Run on the cluster's default partition (works on a local install) + python ~/code/cmd_queue/examples/slurm_example.py + + # Target a specific partition / account on a shared cluster + python ~/code/cmd_queue/examples/slurm_example.py \ + --partition=general --account=my_project + + # Just print the sbatch script without submitting anything + python ~/code/cmd_queue/examples/slurm_example.py --dry=1 + + # Force a clean run (no injected failures) + python ~/code/cmd_queue/examples/slurm_example.py --failures=0 +""" + +import scriptconfig as scfg +import ubelt as ub + + +class SlurmExampleConfig(scfg.DataConfig): + """ + Command line options for the slurm backend example. + """ + + mode = scfg.Value( + 'hybrid', + type=str, + help='Where the monitor UI runs.', + choices=['hybrid', 'inline', 'tmux', 'none'], + ) + name = scfg.Value( + 'slurm-example', + help=ub.paragraph( """ - command1 --input=foo.txt --output=bar.txt - """ - ) + Queue name; also doubles as the lookup key for `cmd_queue + monitor `. + """ + ), ) - - job2 = queue.submit( - ub.codeblock( + partition = scfg.Value( + None, + help=ub.paragraph( """ - command2 --input=foo.txt --output=baz.txt - """ - ) + Slurm partition to submit to. If unset, the sbatch + --partition option is omitted and the cluster's default + partition is used. + """ + ), ) - - queue.submit( - ub.codeblock( + account = scfg.Value( + None, + help=ub.paragraph( + """ + Slurm account to bill jobs to. If unset, the sbatch + --account option is omitted. + """ + ), + ) + cpus = scfg.Value( + 1, + type=int, + help='Value for sbatch --cpus-per-task.', + ) + dry = scfg.Value( + False, + isflag=True, + help='Print the sbatch script and exit without submitting.', + ) + failures = scfg.Value( + 1, + type=int, + help=ub.paragraph( + """ + Number of proc-* logical jobs to force into failure (0-4). + The failures cascade: dependent merge/final jobs are skipped. + """ + ), + ) + logs = scfg.Value( + True, + isflag=True, + help=ub.paragraph( + """ + Set to False to disable per-job log capture (default: enabled). """ - command3 --input1=bar.txt --input2=baz.txt --output=buz.txt - """ ), - depends=[job2, job1], ) + +def main(): + import cmd_queue + + args = SlurmExampleConfig.cli() + + # Only pass partition/account through to sbatch when the user + # actually specified them; otherwise let slurm use its defaults. + create_kw = { + 'backend': 'slurm', + 'name': args.name, + 'cpus': args.cpus, + } + if args.partition is not None: + create_kw['partition'] = args.partition + if args.account is not None: + create_kw['account'] = args.account + + queue = cmd_queue.Queue.create(**create_kw) + + proc_names = ['proc-A', 'proc-B', 'proc-C', 'proc-D'] + fail_set = set(proc_names[: max(0, min(args.failures, len(proc_names)))]) + + submit_kw = {'log': args.logs} + + def submit_sleep_chain(base_name, total_sleep, depends=None, fail=False): + """ + Submit a logical sleep job as a chain of smaller queue jobs. + + This keeps the logical runtime roughly equal to ``total_sleep``, + but gives the monitor more individual jobs to display. + + Example: + ``submit_sleep_chain('prep-A', 5)`` creates: + + prep-A-01 -> prep-A-02 -> prep-A-03 -> prep-A-04 -> prep-A-05 + + Each part sleeps for one second, so the total duration is still + about five seconds, plus a small amount of scheduling overhead. + """ + if total_sleep <= 0: + raise ValueError('total_sleep must be positive') + + prev_depends = list(depends or []) + last_job = None + + for idx in range(total_sleep): + part = idx + 1 + name = f'{base_name}-{part:02d}' + is_final_part = part == total_sleep + + cmd = f'echo "[{name}] start"; sleep 1; ' + + if is_final_part and fail: + cmd += f'echo "[{base_name}] FORCED FAILURE" >&2; exit 1' + elif is_final_part: + cmd += f'echo "[{base_name}] done"' + else: + cmd += f'echo "[{name}] done"' + + last_job = queue.submit( + cmd, + name=name, + depends=prev_depends, + **submit_kw, + ) + prev_depends = [last_job] + + return last_job + + # Level 1: four independent prep jobs -- slurm can run these in + # parallel across nodes/cores as resources allow. + prep_a = submit_sleep_chain('prep-A', 5) + prep_b = submit_sleep_chain('prep-B', 7) + prep_c = submit_sleep_chain('prep-C', 6) + prep_d = submit_sleep_chain('prep-D', 8) + + # Level 2: each process job depends on exactly one prep job; some + # may be forced to fail by --failures. + proc_a = submit_sleep_chain( + 'proc-A', 3, depends=[prep_a], fail='proc-A' in fail_set + ) + proc_b = submit_sleep_chain( + 'proc-B', 4, depends=[prep_b], fail='proc-B' in fail_set + ) + proc_c = submit_sleep_chain( + 'proc-C', 5, depends=[prep_c], fail='proc-C' in fail_set + ) + proc_d = submit_sleep_chain( + 'proc-D', 3, depends=[prep_d], fail='proc-D' in fail_set + ) + + # Level 3: two merge jobs, each waiting on a pair of proc jobs. + merge_x = submit_sleep_chain('merge-X', 4, depends=[proc_a, proc_b]) + merge_y = submit_sleep_chain('merge-Y', 3, depends=[proc_c, proc_d]) + + # Level 4: single finalize job -- the whole pipeline converges here. + submit_sleep_chain('final', 2, depends=[merge_x, merge_y]) + + queue.print_graph() + + # Show the actual sbatch submission script that would run. queue.print_commands() - queue.run() + if args.dry: + print('\n--dry set: printed sbatch script, not submitting.') + return + + if not queue.is_available(): + raise SystemExit( + 'slurm backend not available on this machine ' + '(is slurmd running and squeue working?)' + ) + + print( + f'\nSubmitting with monitor={args.mode!r}, ' + f'partition={args.partition!r}, account={args.account!r}, ' + f'failures={args.failures}, logs={args.logs}\n' + ) + + result = queue.run( + block=True, + monitor=args.mode, + onfail='kill', + ) + + print(f'\nrun() returned: {result}') if __name__ == '__main__': diff --git a/examples/tmux_example.py b/examples/tmux_example.py index cb7784e..2df268e 100644 --- a/examples/tmux_example.py +++ b/examples/tmux_example.py @@ -64,6 +64,7 @@ class TmuxExampleConfig(scfg.DataConfig): mode = scfg.Value( 'hybrid', + type=str, help='Where the monitor UI runs.', choices=['hybrid', 'inline', 'tmux', 'none'], ) From 154823b0289e8d50c8bde67a8dec2eb1e32d8c5b Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 11:52:49 -0400 Subject: [PATCH 14/31] Add pytest-only timeout to blocking monitor loops The tmux backend's blocking monitor loops (_headless_block_until_done, _run_live_with_attach, block_with_attach_prompt) wait forever for workers to finish. That is correct for production, where queues may run for days or weeks, but under pytest a worker that never reaches 'done' hangs the suite and can grow captured output until the runner is OOM-killed. Add resolve_block_timeout() / block_deadline() in util_tmux. The default stays infinite (None); a finite bound is applied only when running under pytest (detected via PYTEST_CURRENT_TEST, default 300s). CMD_QUEUE_BLOCK_TIMEOUT overrides in any context so production can opt in or tests can force infinite. Each poll loop now calls a per-iteration deadline check that is a no-op when the timeout is None and raises TimeoutError once it expires. Add tests/test_block_timeout.py covering the resolution precedence and the deadline no-op / expiry behavior. Co-Authored-By: Claude Opus 4.8 --- cmd_queue/backends/tmux.py | 7 +++ cmd_queue/util/util_tmux.py | 68 +++++++++++++++++++++ tests/test_block_timeout.py | 114 ++++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+) create mode 100644 tests/test_block_timeout.py diff --git a/cmd_queue/backends/tmux.py b/cmd_queue/backends/tmux.py index ef4a77e..f756a24 100644 --- a/cmd_queue/backends/tmux.py +++ b/cmd_queue/backends/tmux.py @@ -59,6 +59,7 @@ from cmd_queue import base_queue from cmd_queue.backends.serial import SerialQueue from cmd_queue.util.util_tmux import tmux +from cmd_queue.util.util_tmux import block_deadline class TMUXMultiQueue(base_queue.Queue): @@ -959,10 +960,12 @@ def _headless_block_until_done(self, refresh_rate: float = 1.0) -> Any: """ import time + check_deadline = block_deadline(label=f'queue {self.name}') while True: table, finished, agg_state = self._build_status_table() if finished: return agg_state + check_deadline() time.sleep(refresh_rate) def read_state(self) -> Any: @@ -1611,12 +1614,15 @@ def _run_live_with_attach( from rich.live import Live + check_deadline = block_deadline(label='queue') + if side_session is None: # Plain path with no input handling — preserves old behavior # exactly when there is no side session to attach to. renderable, finished, _ = build_renderable() with Live(renderable, refresh_per_second=4) as live: while not finished: + check_deadline() time.sleep(refresh_rate) renderable, finished, _ = build_renderable() live.update(renderable) @@ -1635,6 +1641,7 @@ def _run_live_with_attach( renderable, finished, _ = build_renderable() with Live(renderable, refresh_per_second=4) as live: while not finished: + check_deadline() ready, _, _ = select.select( [sys.stdin], [], [], refresh_rate ) diff --git a/cmd_queue/util/util_tmux.py b/cmd_queue/util/util_tmux.py index c61e32d..6a8f0a6 100644 --- a/cmd_queue/util/util_tmux.py +++ b/cmd_queue/util/util_tmux.py @@ -8,6 +8,70 @@ import ubelt as ub +def resolve_block_timeout(explicit: Any = None) -> float | None: + """ + Resolve the wall-clock timeout (in seconds) for a blocking monitor loop. + + Production queues may run for days or weeks, so the default is to wait + forever (returns None). Under pytest we impose a finite bound instead, so + a worker that never finishes fails the test loudly rather than hanging the + suite (or slowly growing captured output until the runner is OOM-killed). + + Resolution order (first match wins): + 1. ``explicit`` argument, if not None and not ``'auto'``. A value of + ``0`` or ``inf`` means wait forever (returns None). + 2. ``CMD_QUEUE_BLOCK_TIMEOUT`` env var, which applies in *all* contexts + so production can opt into a bound. Numeric seconds, or one of + ``none``/``inf``/``infinite``/``forever`` to wait forever. + 3. If running under pytest (``PYTEST_CURRENT_TEST`` is set): + ``CMD_QUEUE_PYTEST_BLOCK_TIMEOUT`` (default 300 seconds). + 4. Otherwise None (wait forever). + + Returns: + float | None: seconds to wait, or None to wait forever. + """ + import os + + if explicit is not None and explicit != 'auto': + return None if float(explicit) in (0.0, float('inf')) else float(explicit) + + env_val = os.environ.get('CMD_QUEUE_BLOCK_TIMEOUT', '').strip() + if env_val: + if env_val.lower() in ('none', 'inf', 'infinite', 'forever'): + return None + return float(env_val) + + if os.environ.get('PYTEST_CURRENT_TEST'): + return float(os.environ.get('CMD_QUEUE_PYTEST_BLOCK_TIMEOUT', '300')) + + return None + + +def block_deadline(label: str = 'queue', explicit: Any = None) -> Any: + """ + Build a ``check()`` callable that enforces :func:`resolve_block_timeout`. + + The returned callable is a no-op when the resolved timeout is None (the + production default), and otherwise raises ``TimeoutError`` once the + deadline has passed. Call it once per iteration of a blocking poll loop. + """ + import time + + timeout = resolve_block_timeout(explicit) + deadline = None if timeout is None else time.monotonic() + timeout + + def check() -> None: + if deadline is not None and time.monotonic() > deadline: + raise TimeoutError( + f'cmd_queue: timed out after {timeout}s waiting for {label} ' + 'to finish. This bound only applies under pytest or when ' + 'CMD_QUEUE_BLOCK_TIMEOUT is set; production waits forever by ' + 'default.' + ) + + return check + + class tmux: """ TODO: @@ -171,8 +235,11 @@ def block_with_attach_prompt( import sys import time + check_deadline = block_deadline(label=label) + if not sys.stdin.isatty(): while not is_finished_fn(): + check_deadline() time.sleep(refresh_rate) return @@ -204,6 +271,7 @@ def block_with_attach_prompt( while True: if is_finished_fn(): return + check_deadline() ready, _, _ = select.select([sys.stdin], [], [], refresh_rate) if not ready: continue diff --git a/tests/test_block_timeout.py b/tests/test_block_timeout.py new file mode 100644 index 0000000..055806c --- /dev/null +++ b/tests/test_block_timeout.py @@ -0,0 +1,114 @@ +""" +Regression tests for the blocking-monitor timeout policy. + +Production queues can legitimately run for days or weeks, so the blocking +monitor loops must wait forever by default. To keep a stuck worker from +hanging the test suite (or slowly growing pytest's captured output until +the runner is OOM-killed), a finite bound is imposed *only* when running +under pytest, detected via the ``PYTEST_CURRENT_TEST`` env var. + +These tests exercise :func:`cmd_queue.util.util_tmux.resolve_block_timeout` +and :func:`cmd_queue.util.util_tmux.block_deadline` directly. Note that +because the suite itself runs under pytest, ``PYTEST_CURRENT_TEST`` is +already set in the environment; the "production" cases delete it via +``monkeypatch`` (which restores it after each test). +""" +from __future__ import annotations + +import time + +import pytest + +from cmd_queue.util.util_tmux import block_deadline, resolve_block_timeout + + +def _clear_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Start each case from a known-clean env.""" + monkeypatch.delenv('PYTEST_CURRENT_TEST', raising=False) + monkeypatch.delenv('CMD_QUEUE_BLOCK_TIMEOUT', raising=False) + monkeypatch.delenv('CMD_QUEUE_PYTEST_BLOCK_TIMEOUT', raising=False) + + +def test_production_default_is_infinite(monkeypatch): + # No pytest marker and no override => wait forever. + _clear_env(monkeypatch) + assert resolve_block_timeout() is None + + +def test_pytest_default_is_bounded(monkeypatch): + _clear_env(monkeypatch) + monkeypatch.setenv('PYTEST_CURRENT_TEST', 'test_x.py::test_y (call)') + assert resolve_block_timeout() == 300.0 + + +def test_pytest_default_is_configurable(monkeypatch): + _clear_env(monkeypatch) + monkeypatch.setenv('PYTEST_CURRENT_TEST', 'test_x.py::test_y (call)') + monkeypatch.setenv('CMD_QUEUE_PYTEST_BLOCK_TIMEOUT', '42') + assert resolve_block_timeout() == 42.0 + + +def test_env_var_overrides_in_production(monkeypatch): + # A production user can opt into a bound without pytest involved. + _clear_env(monkeypatch) + monkeypatch.setenv('CMD_QUEUE_BLOCK_TIMEOUT', '5') + assert resolve_block_timeout() == 5.0 + + +def test_env_var_overrides_pytest_default(monkeypatch): + # The explicit env var beats the pytest auto-default, in both directions. + _clear_env(monkeypatch) + monkeypatch.setenv('PYTEST_CURRENT_TEST', 'test_x.py::test_y (call)') + monkeypatch.setenv('CMD_QUEUE_BLOCK_TIMEOUT', '5') + assert resolve_block_timeout() == 5.0 + + +@pytest.mark.parametrize('word', ['none', 'inf', 'infinite', 'forever', 'NONE']) +def test_env_var_can_force_infinite_under_pytest(monkeypatch, word): + _clear_env(monkeypatch) + monkeypatch.setenv('PYTEST_CURRENT_TEST', 'test_x.py::test_y (call)') + monkeypatch.setenv('CMD_QUEUE_BLOCK_TIMEOUT', word) + assert resolve_block_timeout() is None + + +def test_explicit_arg_wins(monkeypatch): + _clear_env(monkeypatch) + monkeypatch.setenv('CMD_QUEUE_BLOCK_TIMEOUT', '5') + assert resolve_block_timeout(12) == 12.0 + # 0 and inf mean "wait forever". + assert resolve_block_timeout(0) is None + assert resolve_block_timeout(float('inf')) is None + + +def test_explicit_auto_falls_through(monkeypatch): + # 'auto' is treated like None so callers can pass it as a sentinel. + _clear_env(monkeypatch) + assert resolve_block_timeout('auto') is None + monkeypatch.setenv('PYTEST_CURRENT_TEST', 'test_x.py::test_y (call)') + assert resolve_block_timeout('auto') == 300.0 + + +def test_block_deadline_is_noop_when_infinite(monkeypatch): + _clear_env(monkeypatch) + check = block_deadline(label='queue forever') + # Even after time passes, an infinite deadline never raises. + for _ in range(3): + time.sleep(0.001) + assert check() is None + + +def test_block_deadline_raises_after_expiry(monkeypatch): + _clear_env(monkeypatch) + monkeypatch.setenv('CMD_QUEUE_BLOCK_TIMEOUT', '0.01') + check = block_deadline(label='queue stuck') + # Not yet expired right after construction. + check() + time.sleep(0.05) + with pytest.raises(TimeoutError, match='queue stuck'): + check() + + +if __name__ == '__main__': + import sys + + sys.exit(pytest.main([__file__, '-v'])) From 70fe2bee68947e40c5e0991f5ff1c0ff3266eadb Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 11:57:08 -0400 Subject: [PATCH 15/31] Update deps --- pyproject.toml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 284c2ba..9fb8042 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,11 +5,6 @@ requires = [ "wheel>=0.37.1", ] -[tool.uv] -# Supply-chain guard: never resolve packages published after this date. -# Bump manually when intentionally pulling in newer upstream releases. -exclude-newer = "2026-05-22" - [project] name = "cmd_queue" description = "The cmd_queue module for a DAG of bash commands" @@ -49,9 +44,16 @@ dynamic.dependencies.file = [ dynamic.optional-dependencies.airflow.file = [ "requirements/airflow.txt", ] +dynamic.optional-dependencies.all.file = [ + "requirements/optional.txt", + "requirements/airflow.txt", +] dynamic.optional-dependencies.docs.file = [ "requirements/docs.txt", ] +dynamic.optional-dependencies.linting.file = [ + "requirements/linting.txt", +] dynamic.optional-dependencies.optional.file = [ "requirements/optional.txt", ] @@ -73,6 +75,7 @@ packages.find.include = [ "cmd_queue*", ] + [tool.ruff] target-version = "py310" line-length = 80 From 8b53bd276d1f20088dc47dce8575c5d21fbc645f Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 13:03:10 -0400 Subject: [PATCH 16/31] Make airflow backend robust to Airflow 3.3+ breaking changes Airflow 3.3 (currently 3.3.0b2 on PyPI) removed the ``include_examples`` kwarg from ``DagBag`` and now requires ``api_auth/jwt_secret`` to be set for local ``dag.test`` runs. CI's loose/prerelease job pulls this beta and was failing with ``TypeError: DagBag.__init__() got an unexpected keyword argument 'include_examples'``. - Introspect the ``DagBag`` signature and only pass ``include_examples`` when supported (examples stay disabled via AIRFLOW__CORE__LOAD_EXAMPLES). - Prefer the canonical ``airflow.dag_processing.dagbag`` import (3.2+), falling back to the deprecated ``airflow.models.dagbag``. - Provide a deterministic AIRFLOW__API_AUTH__JWT_SECRET (ignored by older versions) so 3.3+ embedded runs work out of the box. Verified against apache-airflow 3.2.1 and apache-airflow-core 3.3.0b2. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/airflow.py | 41 ++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/cmd_queue/backends/airflow.py b/cmd_queue/backends/airflow.py index cd4ef09..3315efa 100644 --- a/cmd_queue/backends/airflow.py +++ b/cmd_queue/backends/airflow.py @@ -199,6 +199,14 @@ def _airflow_env(self): 'AIRFLOW__DATABASE__SQL_ALCHEMY_CONN', f'sqlite:///{self.airflow_home / "airflow.db"}', ) + # Airflow 3.3+ runs tasks through the Task SDK execution API, which + # requires a JWT secret to be configured even for local ``dag.test`` + # runs. Provide a deterministic per-queue secret so the embedded run + # works out of the box. Ignored by older Airflow versions. + env.setdefault( + 'AIRFLOW__API_AUTH__JWT_SECRET', + ub.hash_data(self.queue_id)[0:32], + ) return env @contextlib.contextmanager @@ -227,10 +235,17 @@ def run(self, block: bool = True, system: bool = False) -> None: ) with self._patched_env(env): import contextlib + import inspect import sys from airflow.models.dag import DagModel - from airflow.models.dagbag import DagBag + try: + # Canonical location since Airflow 3.2 (AIP-66). Importing it + # directly avoids the DeprecationWarning emitted by the + # ``airflow.models.dagbag`` compatibility shim. + from airflow.dag_processing.dagbag import DagBag + except ImportError: + from airflow.models.dagbag import DagBag from airflow.models.dagbundle import DagBundleModel from airflow.models.serialized_dag import DagVersion from airflow.utils import db @@ -244,11 +259,27 @@ def run(self, block: bool = True, system: bool = False) -> None: db.upgradedb() else: db.initdb() - dag_bag = DagBag( - dag_folder=os.fspath(self.dags_dpath), - include_examples=False, - safe_mode=False, + # Build kwargs defensively: ``include_examples`` was removed from + # ``DagBag`` in Airflow 3.3 (example loading is now controlled only + # by the ``core.load_examples`` config, which we already disable via + # ``AIRFLOW__CORE__LOAD_EXAMPLES`` in ``_airflow_env``). Only pass + # kwargs that the installed signature actually accepts so we stay + # compatible across Airflow 3.1 - 3.3+. + dagbag_kwargs = { + 'dag_folder': os.fspath(self.dags_dpath), + 'include_examples': False, + 'safe_mode': False, + } + params = inspect.signature(DagBag.__init__).parameters + accepts_varkw = any( + p.kind == inspect.Parameter.VAR_KEYWORD + for p in params.values() ) + if not accepts_varkw: + dagbag_kwargs = { + k: v for k, v in dagbag_kwargs.items() if k in params + } + dag_bag = DagBag(**dagbag_kwargs) dag = dag_bag.get_dag(self.name) if dag is None: raise RuntimeError( From e61402ad17723a5212d81410daa2a8fd0b2fa02c Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 13:13:20 -0400 Subject: [PATCH 17/31] Fix ty no-matching-overload on ndarray.argmin and type DagBag kwargs - ty could not match the zero-argument ``bin_sums.argmin()`` overload; use the function form ``int(np.argmin(bin_sums))`` which type-checks and is used directly as a list index. - Annotate the dynamically-built ``DagBag`` kwargs as ``Dict[str, Any]`` so unpacking it does not trip ty's invalid-argument-type check when airflow stubs are resolvable. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/airflow.py | 2 +- cmd_queue/util/util_algo.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd_queue/backends/airflow.py b/cmd_queue/backends/airflow.py index 3315efa..3d5bf17 100644 --- a/cmd_queue/backends/airflow.py +++ b/cmd_queue/backends/airflow.py @@ -265,7 +265,7 @@ def run(self, block: bool = True, system: bool = False) -> None: # ``AIRFLOW__CORE__LOAD_EXAMPLES`` in ``_airflow_env``). Only pass # kwargs that the installed signature actually accepts so we stay # compatible across Airflow 3.1 - 3.3+. - dagbag_kwargs = { + dagbag_kwargs: Dict[str, Any] = { 'dag_folder': os.fspath(self.dags_dpath), 'include_examples': False, 'safe_mode': False, diff --git a/cmd_queue/util/util_algo.py b/cmd_queue/util/util_algo.py index b32ff20..f459705 100644 --- a/cmd_queue/util/util_algo.py +++ b/cmd_queue/util/util_algo.py @@ -49,7 +49,7 @@ def balanced_number_partitioning( for item_index in sortx: # Assign item to the smallest bin item_weight = item_weights[item_index] - bin_index = bin_sums.argmin() + bin_index = int(np.argmin(bin_sums)) bin_assignments[bin_index].append(item_index) bin_sums[bin_index] += item_weight From e5d07ffc78d432c4bb2bc83bcce5f8ead5ec1960 Mon Sep 17 00:00:00 2001 From: joncrall Date: Thu, 25 Jun 2026 13:20:41 -0400 Subject: [PATCH 18/31] [skip ci] Start branch for 0.3.2 --- CHANGELOG.md | 5 ++++- cmd_queue/__init__.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2285d40..b013e45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,10 @@ We are currently working on porting this changelog to the specifications in This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## Version 0.3.1 - Unreleased +## Version 0.3.2 - Unreleased + + +## Version 0.3.1 - Released 2026-06-25 ### Added: * First-class job `setup` / `teardown` lifecycle on `BashJob` (serial/tmux) and `SlurmJob`. `setup` is a gating precondition (shares the preamble's `PREAMBLE_OK` gating; a failing setup skips the command and marks the job failed). `teardown` always runs after the command — on success, failure, and SIGINT/SIGTERM — provided setup succeeded. It is rendered as a per-job, signal-safe cleanup (a scoped subshell trap for serial/tmux so it cannot leak across the many jobs in one script; an in-`--wrap` trap for slurm). The main command's exit code stays authoritative (a teardown failure does not flip the job result). A hard SIGKILL cannot be trapped — an out-of-band reclaim (e.g. a lease TTL) is the only backstop for that. This is the job-level try/finally for bracketing an external resource (e.g. acquire/release a GPU lease). diff --git a/cmd_queue/__init__.py b/cmd_queue/__init__.py index ad86b97..c3b7912 100644 --- a/cmd_queue/__init__.py +++ b/cmd_queue/__init__.py @@ -306,7 +306,7 @@ __mkinit__ = """ mkinit -m cmd_queue """ -__version__ = '0.3.1' +__version__ = '0.3.2' __submodules__ = { From 3af217213b4534e026323c5f7191cfbc4ba99dfb Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 13:55:40 -0400 Subject: [PATCH 19/31] Fix monitor='none' smartcast in CMDQueueConfig Add type=str to the monitor Value. Without it, scriptconfig smartcasts the string 'none' into Python None, which then fails the field's own choices validation, breaking --monitor=none on the CLI. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/cli_boilerplate.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd_queue/cli_boilerplate.py b/cmd_queue/cli_boilerplate.py index c828403..1c8b5c0 100644 --- a/cmd_queue/cli_boilerplate.py +++ b/cmd_queue/cli_boilerplate.py @@ -168,6 +168,9 @@ class CMDQueueConfig(scfg.DataConfig): monitor = scfg.Value( 'inline', + # NOTE: type=str is important. Without it scriptconfig smartcasts the + # string 'none' to Python None, which then fails choices validation. + type=str, help=ub.paragraph( """ Where the live status UI runs while jobs execute. From 2c69ee7fe01735a71b2f49353e18390d9b73fa04 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 14:09:12 -0400 Subject: [PATCH 20/31] Improve type checking of Job.depends and named_jobs registration Replace blanket type: ignore comments with structural fixes on the queue internals, giving better type coverage without suppression: - Introduce base_queue.JobDepends alias and coerce_job_depends() helper. Job constructors only ever receive a Job or an iterable of Jobs (string refs are resolved at the queue level before construction), so the param was mistyped as Optional[Iterable[Job]]. The helper normalizes via isinstance (which the type checker narrows, unlike ub.iterable), so self.depends is a clean List[Job]. Removes 4 depends-related ignores across Job/BashJob/SlurmJob/AirflowJob. - Add Queue._register_named_job() to narrow Optional[str] job names to str once before indexing named_jobs (Dict[str, Job]), replacing 3 duplicated invalid-assignment ignores in the base + slurm + airflow submit paths. - Clean up change_backend's two invalid-argument-type ignores via explicit narrowing, and drop the unreachable dead code after its return. ty passes; no behavior change (verified by tests + multi-backend doctests). Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/airflow.py | 12 +++--- cmd_queue/backends/serial.py | 9 ++--- cmd_queue/backends/slurm.py | 12 +++--- cmd_queue/base_queue.py | 73 +++++++++++++++++++++++++---------- 4 files changed, 67 insertions(+), 39 deletions(-) diff --git a/cmd_queue/backends/airflow.py b/cmd_queue/backends/airflow.py index 3d5bf17..0c34390 100644 --- a/cmd_queue/backends/airflow.py +++ b/cmd_queue/backends/airflow.py @@ -26,11 +26,12 @@ True """ from __future__ import annotations + import contextlib import os import time import uuid -from typing import Any, Dict, Iterable, List, Optional +from typing import Any, Dict, List, Optional import ubelt as ub @@ -47,7 +48,7 @@ def __init__( command: str, name: Optional[str] = None, output_fpath: Optional[Any] = None, - depends: Optional[Iterable[base_queue.Job]] = None, + depends: base_queue.JobDepends = None, partition: Optional[Any] = None, cpus: Optional[Any] = None, gpus: Optional[Any] = None, @@ -59,13 +60,11 @@ def __init__( super().__init__() if name is None: name = 'job-' + str(uuid.uuid4()) - if depends is not None and not ub.iterable(depends): - depends = [depends] # type: ignore self.unused_kwargs = kwargs self.command = command self.name = name self.output_fpath = output_fpath - self.depends = depends + self.depends: List[base_queue.Job] = base_queue.coerce_job_depends(depends) self.cpus = cpus self.gpus = gpus self.mem = mem @@ -466,8 +465,7 @@ def submit(self, command: str, **kwargs: Any) -> AirflowJob: # ty: ignore[inval job = AirflowJob(command, depends=depends, **kwargs) self.jobs.append(job) self.num_real_jobs += 1 - # job.name is set above before this line, but ty sees Optional. - self.named_jobs[job.name] = job # ty: ignore[invalid-assignment] + self._register_named_job(job) return job def print_commands( diff --git a/cmd_queue/backends/serial.py b/cmd_queue/backends/serial.py index 1772d24..e927901 100644 --- a/cmd_queue/backends/serial.py +++ b/cmd_queue/backends/serial.py @@ -4,8 +4,9 @@ https://stackoverflow.com/questions/13195655/bash-set-x-without-it-being-printed """ from __future__ import annotations + import uuid -from typing import Any, Dict, Iterable, List, Optional +from typing import Any, Dict, List, Optional import ubelt as ub @@ -103,7 +104,7 @@ def __init__( self, command: str, name: Optional[str] = None, - depends: Optional[Iterable[base_queue.Job]] = None, + depends: base_queue.JobDepends = None, gpus: Optional[Any] = None, cpus: Optional[Any] = None, mem: Optional[Any] = None, @@ -118,8 +119,6 @@ def __init__( teardown: List[str] | str | None = None, **kwargs: Any, ) -> None: - if depends is not None and not ub.iterable(depends): - depends = [depends] # type: ignore self.name = name assert self.name is not None self.pathid = self.name + '_' + ub.hash_data(uuid.uuid4())[0:8] @@ -128,7 +127,7 @@ def __init__( # The base ``Job`` types ``command`` as ``str | None``; a BashJob always # has a concrete command, so narrow it (keeps ``'\n'.join`` well-typed). self.command: str = command - self.depends: List[base_queue.Job] = list(depends) if depends else [] + self.depends: List[base_queue.Job] = base_queue.coerce_job_depends(depends) self.bookkeeper = bookkeeper self.log = log if info_dpath is None: diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index 3c1810e..fdc7cf8 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -38,7 +38,8 @@ >>> print('output does not exist') """ from __future__ import annotations -from typing import Any, Dict, Iterable, List, Optional, Union + +from typing import Any, Dict, List, Optional, Union import ubelt as ub @@ -264,7 +265,7 @@ def __init__( command: str, name: Optional[str] = None, output_fpath: Optional[Any] = None, - depends: Optional[Iterable[base_queue.Job]] = None, + depends: base_queue.JobDepends = None, cpus: Optional[Any] = None, gpus: Optional[Any] = None, mem: Optional[Any] = None, @@ -281,8 +282,6 @@ def __init__( import uuid name = 'job-' + str(uuid.uuid4()) - if depends is not None and not ub.iterable(depends): - depends = [depends] # type: ignore self.unused_kwargs = kwargs # The base ``Job`` types ``command`` as ``str | None``; a SlurmJob always # has a concrete command, so narrow it (keeps the ``--wrap`` join and @@ -292,7 +291,7 @@ def __init__( # concrete ``str`` here (the base ``Job`` types it as ``str | None``). self.name: str = name self.output_fpath = output_fpath - self.depends = depends + self.depends: List[base_queue.Job] = base_queue.coerce_job_depends(depends) self.cpus = cpus self.gpus = gpus self.mem = mem @@ -753,8 +752,7 @@ def submit( # ty: ignore[invalid-method-override] job = SlurmJob(command, depends=depends, preamble=preamble, **_kwargs) self.jobs.append(job) self.num_real_jobs += 1 - # job.name is always populated above, but ty sees ``str | None``. - self.named_jobs[job.name] = job # ty: ignore[invalid-assignment] + self._register_named_job(job) return job def order_jobs(self) -> List[SlurmJob]: diff --git a/cmd_queue/base_queue.py b/cmd_queue/base_queue.py index 8c244c2..a3fb196 100644 --- a/cmd_queue/base_queue.py +++ b/cmd_queue/base_queue.py @@ -1,5 +1,6 @@ from __future__ import annotations -from typing import Any, Dict, Iterable, List, Optional, Union + +from typing import Any, Dict, Iterable, List, Optional, TypeAlias, Union import ubelt as ub @@ -35,15 +36,13 @@ def __init__( self, command: Optional[str] = None, name: Optional[str] = None, - depends: Optional[Iterable[Job]] = None, + depends: JobDepends = None, **kwargs: Any, ) -> None: # This is unused, should the slurm and bash job reuse this? - if depends is not None and not ub.iterable(depends): - depends = [depends] # type: ignore self.name = name self.command = command - self.depends = depends + self.depends: List[Job] = coerce_job_depends(depends) self.kwargs = kwargs def __nice__(self) -> str: @@ -54,6 +53,30 @@ def finalize_text(self, *args: Any, **kwargs: Any) -> str: raise NotImplementedError +# The dependencies accepted by a *Job constructor*. String references are +# resolved to concrete ``Job`` objects at the queue level (see +# ``Queue.submit`` and the backend ``submit`` overrides) before any Job is +# constructed, so the constructors only ever see a single ``Job`` or an +# iterable of ``Job``. Contrast with :data:`cmd_queue._types.DependencyRefs`, +# which additionally allows the unresolved string form accepted by ``submit``. +JobDepends: TypeAlias = Union[Job, Iterable[Job], None] + + +def coerce_job_depends(depends: JobDepends) -> List[Job]: + """Normalize a job-constructor ``depends`` argument to a ``list``. + + Accepts a single :class:`Job`, an iterable of jobs, or ``None`` and always + returns a (possibly empty) ``list`` of jobs. Using ``isinstance`` (rather + than ``ubelt.iterable``) lets the type checker narrow the union, so callers + get a precise ``List[Job]`` without a ``type: ignore``. + """ + if depends is None: + return [] + if isinstance(depends, Job): + return [depends] + return list(depends) + + class Queue(ub.NiceRepr): """ Base class for a queue. @@ -122,20 +145,19 @@ def change_backend(self, backend: str, **kwargs: Any) -> Queue: new = Queue.create(backend=backend, **kwargs) for job_name, job in self.named_jobs.items(): new_depends = [] - if job.depends: - for dep in job.depends: - # named_jobs only contains non-None-named jobs by - # construction, but ``Job.name`` is typed Optional. - new_dep = new.named_jobs[dep.name] # ty: ignore[invalid-argument-type] - new_depends.append(new_dep) - # TODO: carry over metadata - new.submit(job.command, depends=new_depends, name=job.name) # ty: ignore[invalid-argument-type] + for dep in job.depends: + # ``Job.name`` is typed Optional, but every dependency was + # registered under a concrete name (see ``_register_named_job``). + dep_name = dep.name + if dep_name is not None: + new_depends.append(new.named_jobs[dep_name]) + # TODO: carry over metadata. ``job_name`` is the (non-None) dict + # key, and a registered job always has a concrete command. + command = job.command + if command is not None: + new.submit(command, depends=new_depends, name=job_name) return new - for job in self.jobs: - new.submit(job.commands) - pass - def __len__(self) -> int: return self.num_real_jobs @@ -242,14 +264,25 @@ def submit(self, command: Union[str, Job], **kwargs: Any) -> Job: except Exception: raise - # job.name is set by submit() above before this line, but ty - # only sees ``Optional[str]`` from the Job base class. - self.named_jobs[job.name] = job # ty: ignore[invalid-assignment] + self._register_named_job(job) if not job.bookkeeper: self.num_real_jobs += 1 return job + def _register_named_job(self, job: Job) -> None: + """Index ``job`` in :attr:`named_jobs` by its name. + + ``Job.name`` is typed ``Optional[str]`` on the base class, but a job is + always given a concrete name by the time it is submitted (each backend's + ``submit`` fills one in). The explicit check narrows that to ``str`` for + the ``Dict[str, Job]`` key and fails loudly if the invariant is broken. + """ + name = job.name + if name is None: + raise ValueError('submitted jobs must have a name') + self.named_jobs[name] = job + @classmethod def is_available(cls) -> bool: """ From 5642cc7b75fe0e2c5a43ef247104b4cfbf792172 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 14:09:23 -0400 Subject: [PATCH 21/31] Migrate CLIs to kwconf; deprecate scriptconfig cli_boilerplate Adopt kwconf (the successor to scriptconfig) as a runtime dependency and move the internal CLIs onto it, while keeping the public cli_boilerplate API backwards compatible. - requirements/runtime.txt: add kwconf >= 0.10.0 (scriptconfig stays for the deprecated boilerplate class). - main.py and slurmify.py: direct switch to kwconf (Config, ModalCLI, Flag, parser= instead of type=). kwconf no longer auto-splits comma strings, so --depends and --gpus now use parser='csv' to preserve the documented "comma separated" behavior. The argv=1 idiom is coerced to a bool for kwconf's cli(), and special_options=True preserves the --config/--dump CLI surface. Queue path helpers became methods because kwconf collects class-level data attributes (incl. properties) as fields. - cli_boilerplate.py: add kwconf-based CmdQueueConfigMixin with the same fields and create_queue/run_queue API (fully type-clean, no ignores). The original scriptconfig CMDQueueConfig is unchanged in behavior but now emits a deprecation warning pointing users to the new class and noting the comma-split change. ty + ruff clean on touched files; tests and doctests pass; CLIs exercised end-to-end (new/submit/show/run/list/cleanup and slurmify). Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/cli_boilerplate.py | 258 ++++++++++++++++++++++++++++++++++- cmd_queue/main.py | 97 +++++++------ cmd_queue/slurmify.py | 41 +++--- requirements/runtime.txt | 1 + 4 files changed, 337 insertions(+), 60 deletions(-) diff --git a/cmd_queue/cli_boilerplate.py b/cmd_queue/cli_boilerplate.py index 1c8b5c0..fa6897c 100644 --- a/cmd_queue/cli_boilerplate.py +++ b/cmd_queue/cli_boilerplate.py @@ -1,7 +1,18 @@ """ -This file defines a helper scriptconfig base config that can be used to help -make cmd_queue CLIs so cmd_queue options are standardized and present at the -top level. +This file defines a helper base config that can be used to help make cmd_queue +CLIs so cmd_queue options are standardized and present at the top level. + +There are two flavors: + +* :class:`CmdQueueConfigMixin` -- the current, :mod:`kwconf`-based class. Use + this for new code. + +* :class:`CMDQueueConfig` -- the original :mod:`scriptconfig`-based class. It is + **deprecated** but kept for backwards compatibility with downstream code. It + emits a :class:`DeprecationWarning` when used. + +The example below uses the deprecated scriptconfig class; see +:class:`CmdQueueConfigMixin` for the equivalent kwconf usage. CommandLine: xdoctest -m cmd_queue.cli_boilerplate __doc__:0 @@ -96,9 +107,11 @@ >>> my_cli_main(cmdline=0, run=1, print_queue=0, print_commands=0) """ from __future__ import annotations + import typing from typing import Any, Dict, Optional +import kwconf as kw import scriptconfig as scfg import ubelt as ub @@ -245,6 +258,21 @@ class CMDQueueConfig(scfg.DataConfig): def __post_init__(self) -> None: from cmd_queue.util.util_yaml import Yaml + ub.schedule_deprecation( + modname='cmd_queue', + name='CMDQueueConfig', + type='class', + migration=ub.paragraph( + """ + CMDQueueConfig is built on scriptconfig. Switch to the + kwconf-based :class:`CmdQueueConfigMixin`, which has the same + fields and ``create_queue`` / ``run_queue`` API. Note kwconf + no longer auto-splits comma strings into lists (use + ``parser='csv'`` or ``nargs='+'`` on such fields). + """ + ), + deprecate='0.3.2', + ) # scriptconfig descriptors return the underlying value at runtime; # ty sees the descriptor type and flags the assignment. self.slurm_options = Yaml.coerce(self.slurm_options) or {} # ty: ignore[invalid-assignment] @@ -347,3 +375,227 @@ def run_queue( monitor=config.monitor, **kwargs, ) + + +class CmdQueueConfigMixin(kw.Config): + """ + The kwconf-based successor to :class:`CMDQueueConfig`. + + Carries the common boilerplate for cmd-queue CLIs. Inherit from this class + and add the config options your CLI cares about (they must not clash with + the options defined here). Use :func:`create_queue` to build a queue from + these options and :func:`run_queue` to execute / report it. + + This has the same fields and methods as the deprecated + :class:`CMDQueueConfig`, but is built on :mod:`kwconf` instead of + :mod:`scriptconfig`. The most visible behavior change inherited from kwconf + is that comma-separated CLI strings are *not* auto-split into lists; declare + such fields with ``parser='csv'`` or ``nargs='+'`` if you need that. + + It is a good idea to override the default ``queue_name`` when inheriting: + + .. code:: python + + import kwconf as kw + queue_name = kw.Value('your_default_name', help='...', group='cmd-queue') + + Example: + >>> from cmd_queue.cli_boilerplate import CmdQueueConfigMixin + >>> import kwconf as kw + >>> import ubelt as ub + >>> class MyQueueCLI(CmdQueueConfigMixin): + >>> my_num_steps = kw.Value(2, help='a custom param') + >>> def my_cli_main(argv=False, **kwargs): + >>> config = MyQueueCLI.cli(argv=argv, data=kwargs) + >>> queue = config.create_queue() + >>> job0 = queue.submit('echo "root job"', name='ROOT') + >>> for idx in range(config.my_num_steps): + >>> queue.submit(f'echo "step {idx}"', depends=[job0], name=f'step{idx}') + >>> config.run_queue(queue) + >>> my_cli_main(argv=False, run=0, print_queue=1, print_commands=1, backend='serial') + """ + + run: bool = kw.Flag( + False, + # ``validate=False`` keeps the ``bool`` static type while staying lenient + # about the common ``run=0`` / ``run=1`` integer idiom at runtime. + validate=False, + help='if False, only prints the commands, otherwise executes them', + group='cmd-queue', + ) + + backend: str = kw.Value( + 'tmux', + help=('The cmd_queue backend. Can be tmux, slurm, or serial'), + group='cmd-queue', + ) + + monitor: str = kw.Value( + 'inline', + help=ub.paragraph( + """ + Where the live status UI runs while jobs execute. + hybrid = inline monitor + attachable tmux session (best for + interactive use); inline = inline only (default); tmux = + detached tmux session only (survives the calling shell); none + = headless (reattach hint still printed). + """ + ), + group='cmd-queue', + choices=['hybrid', 'inline', 'tmux', 'none'], + ) + + queue_name: Optional[str] = kw.Value( + None, help='overwrite the default queue name', group='cmd-queue' + ) + + print_commands: Any = kw.Value( + 'auto', + isflag=True, + help='enable / disable rprint before exec', + group='cmd-queue', + ) + + print_queue: Any = kw.Value( + 'auto', isflag=True, help='print the cmd queue DAG', group='cmd-queue' + ) + + with_textual: Any = kw.Value( + 'auto', + isflag=True, + help='setting for cmd-queue monitoring', + group='cmd-queue', + ) + + other_session_handler: str = kw.Value( + 'ask', + help='for tmux backend only. How to handle conflicting sessions. Can be ask, kill, or ignore, or auto', + group='cmd-queue', + ) + + virtualenv_cmd: Optional[str] = kw.Value( + None, + parser=str, + help=ub.paragraph( + """ + Command to start the appropriate virtual environment if your bashrc + does not start it by default.""" + ), + group='cmd-queue', + ) + + tmux_workers: int = kw.Value( + 8, + help='number of tmux workers in the queue for the tmux backend', + group='cmd-queue', + ) + + slurm_options: Any = kw.Value( + None, + help=ub.paragraph( + """ + if the backend is slurm, provide a YAML dictionary for things like + partition / etc... + """ + ), + group='cmd-queue', + ) + + def __post_init__(self) -> None: + from cmd_queue.util.util_yaml import Yaml + + self.slurm_options = Yaml.coerce(self.slurm_options) or {} + + def create_queue(config, **kwargs: Any) -> 'cmd_queue.Queue': + """ + Create an empty queue based on options specified in this config + + Args: + **kwargs: extra args passed to cmd_queue.Queue.create + + Returns: + cmd_queue.Queue + """ + import cmd_queue + + queuekw: Dict[str, Any] = {} + if config.backend == 'slurm': + queuekw.update(config.slurm_options) + elif config.backend == 'tmux': + queuekw.update( + { + 'size': config.tmux_workers, + } + ) + queuekw.update(kwargs) + if 'name' not in queuekw: + queuekw['name'] = config.queue_name + queue = cmd_queue.Queue.create(backend=config.backend, **queuekw) + if config.virtualenv_cmd: + # Experimental feature to automatically activate virtual + # environments + virtualenv_cmd: Optional[str] = config.virtualenv_cmd + if virtualenv_cmd == 'auto': + import os + import shlex + + venv_path = os.environ.get('VIRTUAL_ENV', '') + if venv_path: + virtualenv_cmd = 'source ' + shlex.quote( + str(ub.Path(venv_path) / 'bin/activate') + ) + else: + virtualenv_cmd = None + if virtualenv_cmd: + queue.add_preamble_command(virtualenv_cmd) + return queue + + def run_queue( + config, + queue: 'cmd_queue.Queue', + print_kwargs: Optional[Dict[str, Any]] = None, + **kwargs: Any, + ) -> None: + """ + Execute a queue with options based on this config. + + Args: + queue (cmd_queue.Queue): queue to run / report + print_kwargs (None | Dict): + """ + print_thresh = 30 + if config['print_commands'] == 'auto': + if len(queue) < print_thresh: + config['print_commands'] = 1 + else: + print( + f'More than {print_thresh} jobs, skip queue.print_commands. ' + 'If you want to see them explicitly specify print_commands=1' + ) + config['print_commands'] = 0 + + if config['print_queue'] == 'auto': + if len(queue) < print_thresh: + config['print_queue'] = 1 + else: + print( + f'More than {print_thresh} jobs, skip queue.print_graph. ' + 'If you want to see them explicitly specify print_queue=1' + ) + config['print_queue'] = 0 + + if config.print_commands: + if print_kwargs is None: + print_kwargs = {} + queue.print_commands(**print_kwargs) + + if config.print_queue: + queue.print_graph(vertical_chains=True) + + if config.run: + queue.run( + with_textual=config.with_textual, + other_session_handler=config.other_session_handler, + monitor=config.monitor, + **kwargs, + ) diff --git a/cmd_queue/main.py b/cmd_queue/main.py index 66ca29c..37e000e 100644 --- a/cmd_queue/main.py +++ b/cmd_queue/main.py @@ -11,10 +11,11 @@ """ from __future__ import annotations -from typing import TYPE_CHECKING, Any, Callable +from typing import TYPE_CHECKING, Any, Callable, Sequence + +import kwconf as kw import rich -import scriptconfig as scfg import ubelt as ub __todo__ = """ @@ -91,10 +92,10 @@ def _cmd_queue_tmux_session_ids(sessions: list[dict[str, str]]) -> list[str]: ] -class CommonConfig(scfg.DataConfig): - qname = scfg.Value(None, position=1, help='name of the CLI queue') +class CommonConfig(kw.Config): + qname = kw.Value(None, position=1, help='name of the CLI queue') - dpath = scfg.Value( + dpath = kw.Value( 'auto', help=ub.paragraph( """ @@ -103,44 +104,58 @@ class CommonConfig(scfg.DataConfig): ), ) - verbose = scfg.Value(1, help='verbosity level') + verbose = kw.Value(1, help='verbosity level') def __post_init__(config) -> None: if config['dpath'] == 'auto': config['dpath'] = str(ub.Path.appdir('cmd_queue/cli')) + def cli_queue_dpath(config) -> ub.Path: + """Directory holding this CLI's intermediate queue files. + + Defined as a method rather than an attribute/property because kwconf's + metaclass collects class-level data attributes as config fields. + """ + return ub.Path(config['dpath']) + + def cli_queue_fpath(config) -> ub.Path: + """Path to this queue's ``*.cmd_queue.json`` state file.""" + return config.cli_queue_dpath() / ( + str(config['qname']) + '.cmd_queue.json' + ) + @classmethod - def main(cls, argv: int = 1, **kwargs: Any) -> None: - # scriptconfig ``argv`` accepts True/None/list[str]; the integer - # idiom (``1`` => use sys.argv) is undocumented but in use here. - config = cls.cli(argv=argv, data=kwargs, strict=True) # ty: ignore[invalid-argument-type] + def main( + cls, argv: Sequence[str] | str | int | None = 1, **kwargs: Any + ) -> None: + # ``argv=1``/True reads sys.argv; ``argv=0``/False uses only ``data``. + # kwconf's ``cli`` takes a bool (not int) for the sys.argv toggle. + if isinstance(argv, int): + argv = bool(argv) + config = cls.cli( + argv=argv, data=kwargs, strict=True, special_options=True + ) if config.verbose: # ub.urepr's return type is unioned with a tuple form for # the json branch; the str cast is always-safe here. rich.print('config = ' + str(ub.urepr(config, nl=1))) - cli_queue_name = config['qname'] - # scriptconfig allows attaching arbitrary attributes to a Config - # instance at runtime. - config.cli_queue_dpath = ub.Path(config['dpath']) # ty: ignore[unresolved-attribute] - config.cli_queue_fpath = config.cli_queue_dpath / ( # ty: ignore[unresolved-attribute] - str(cli_queue_name) + '.cmd_queue.json' - ) config.run() class CommonShowRun(CommonConfig): - workers = scfg.Value( + workers = kw.Value( 1, help='number of concurrent queues for the tmux backend.' ) - backend = scfg.Value( + backend = kw.Value( 'tmux', help='the execution backend to use', choices=['tmux', 'slurm', 'serial', 'airflow'], ) - gpus = scfg.Value( + gpus = kw.Value( None, + parser='csv', help='a comma separated list of the gpu numbers to spread across. tmux backend only.', ) @@ -156,7 +171,7 @@ def _build_queue(config) -> 'cmd_queue.Queue': gpus=config['gpus'], ) # Run a new CLI queue - data = json.loads(config.cli_queue_fpath.read_text()) + data = json.loads(config.cli_queue_fpath().read_text()) print('data = {}'.format(ub.urepr(data, nl=1))) row = None try: @@ -198,7 +213,7 @@ def _build_queue(config) -> 'cmd_queue.Queue': return queue -class CmdQueueCLI(scfg.ModalCLI): +class CmdQueueCLI(kw.ModalCLI): r""" The cmd_queue CLI for building, executing, and managing queues from bash. @@ -308,9 +323,8 @@ class cleanup(CommonConfig): cleanup tmux sessions """ - yes = scfg.Value( + yes = kw.Flag( False, - isflag=True, help='if True say yes to prompts', short_alias=['y'], ) @@ -358,7 +372,7 @@ class monitor(CommonConfig): __command__ = 'monitor' - manifest = scfg.Value( + manifest = kw.Value( None, help=ub.paragraph( """ @@ -368,7 +382,7 @@ class monitor(CommonConfig): ), ) - onfail = scfg.Value( + onfail = kw.Value( '', choices=['', 'kill'], help=ub.paragraph( @@ -379,7 +393,7 @@ class monitor(CommonConfig): ), ) - onexit = scfg.Value( + onexit = kw.Value( '', choices=['', 'capture'], help=ub.paragraph( @@ -390,9 +404,9 @@ class monitor(CommonConfig): ), ) - refresh_rate = scfg.Value(0.4, help='monitor refresh rate, seconds') + refresh_rate = kw.Value(0.4, help='monitor refresh rate, seconds') - with_textual = scfg.Value( + with_textual = kw.Value( 'auto', help='use textual UI if available (tmux backend only)' ) @@ -400,8 +414,7 @@ def run(config) -> None: from cmd_queue import monitor_manifest as mm if config.manifest: - # scriptconfig descriptor narrows to str at runtime. - manifest_path = ub.Path(config.manifest).expand().absolute() # ty: ignore[invalid-argument-type] + manifest_path = ub.Path(config.manifest).expand().absolute() if not manifest_path.exists(): raise FileNotFoundError(manifest_path) else: @@ -452,14 +465,16 @@ class submit(CommonConfig): __command__ = 'submit' - jobname = scfg.Value( + jobname = kw.Value( None, help='for submit, this is the name of the new job' ) - depends = scfg.Value(None, help='comma separated jobnames to depend on') + depends = kw.Value( + None, parser='csv', help='comma separated jobnames to depend on' + ) - command = scfg.Value( + command = kw.Value( None, - type=str, + parser=str, position=2, nargs='*', help=ub.paragraph( @@ -523,14 +538,14 @@ def run(config) -> None: import json # Run a new CLI queue - data = json.loads(config.cli_queue_fpath.read_text()) + data = json.loads(config.cli_queue_fpath().read_text()) row = {'type': 'command', 'command': config['command']} if config.jobname: row['name'] = config.jobname if config.depends: row['depends'] = config.depends data.append(row) - config.cli_queue_fpath.write_text(json.dumps(data)) + config.cli_queue_fpath().write_text(json.dumps(data)) class new(CommonConfig): """ @@ -538,7 +553,7 @@ class new(CommonConfig): """ __command__ = 'new' - header = scfg.Value( + header = kw.Value( None, help='a header command to execute in every session (e.g. activating a virtualenv). Only used when action is new', ) @@ -549,12 +564,12 @@ def run(config) -> None: # Start a new CLI queue data = [] config = config - config.cli_queue_fpath.parent.ensuredir() + config.cli_queue_fpath().parent.ensuredir() if config.header is not None: data.append({'type': 'header', 'header': config.header}) - config.cli_queue_fpath.write_text(json.dumps(data)) + config.cli_queue_fpath().write_text(json.dumps(data)) class list(CommonConfig): """ @@ -565,7 +580,7 @@ class list(CommonConfig): def run(config) -> None: print( - ub.urepr(list(config.cli_queue_dpath.glob('*.cmd_queue.json'))) + ub.urepr(list(config.cli_queue_dpath().glob('*.cmd_queue.json'))) ) diff --git a/cmd_queue/slurmify.py b/cmd_queue/slurmify.py index d6fd878..4dd4bc9 100644 --- a/cmd_queue/slurmify.py +++ b/cmd_queue/slurmify.py @@ -16,21 +16,25 @@ -- \ python -c 'import sys; print("hello world"); sys.exit(0)' """ -import scriptconfig as scfg +from typing import Any, Sequence + +import kwconf as kw import ubelt as ub -class SlurmifyCLI(scfg.DataConfig): +class SlurmifyCLI(kw.Config): __command__ = 'slurmify' - jobname = scfg.Value( + jobname = kw.Value( None, help='for submit, this is the name of the new job' ) - depends = scfg.Value(None, help='comma separated jobnames to depend on') + depends = kw.Value( + None, parser='csv', help='comma separated jobnames to depend on' + ) - command = scfg.Value( + command = kw.Value( None, - type=str, + parser=str, position=1, nargs='*', help=ub.paragraph( @@ -45,23 +49,24 @@ class SlurmifyCLI(scfg.DataConfig): ), ) - gpus = scfg.Value( + gpus = kw.Value( None, + parser='csv', help='a comma separated list of the gpu numbers to spread across. tmux backend only.', ) - workers = scfg.Value( + workers = kw.Value( 1, help='number of concurrent queues for the tmux backend.' ) - mem = scfg.Value(None, help='') - partition = scfg.Value(1, help='slurm partition') + mem = kw.Value(None, help='') + partition = kw.Value(1, help='slurm partition') - ntasks = scfg.Value(None, help='') - ntasks_per_node = scfg.Value(None, help='') - cpus_per_task = scfg.Value(None, help='') + ntasks = kw.Value(None, help='') + ntasks_per_node = kw.Value(None, help='') + cpus_per_task = kw.Value(None, help='') @classmethod - def main(cls, argv=1, **kwargs): + def main(cls, argv: Sequence[str] | str | int | None = 1, **kwargs: Any): """ Example: >>> # xdoctest: +SKIP @@ -74,8 +79,12 @@ def main(cls, argv=1, **kwargs): import rich from rich.markup import escape - # See main.py: ``argv=1`` is the scriptconfig idiom for sys.argv. - config = cls.cli(argv=argv, data=kwargs, strict=True) # ty: ignore[invalid-argument-type] + # ``argv=1``/True reads sys.argv; kwconf's ``cli`` takes a bool toggle. + if isinstance(argv, int): + argv = bool(argv) + config = cls.cli( + argv=argv, data=kwargs, strict=True, special_options=True + ) # ub.urepr unions with a tuple form for the json branch; cast to str. rich.print('config = ' + escape(str(ub.urepr(config, nl=1)))) diff --git a/requirements/runtime.txt b/requirements/runtime.txt index 40b160d..61db659 100644 --- a/requirements/runtime.txt +++ b/requirements/runtime.txt @@ -31,6 +31,7 @@ pandas>=1.3.5 ; python_version < '3.8' and python_version >= '3.7' # Python pandas>=1.1.5 ; python_version < '3.7' and python_version >= '3.6' # Python 3.6 scriptconfig >= 0.8.4 +kwconf >= 0.10.0 psutil>=6.1.0 ; python_version < '4.0' and python_version >= '3.9' # Python 3.13+ psutil>=5.9.1 ; python_version < '3.9' and python_version >= '3.7' From 16bf122e591acf889083aeb76cc6d665660b9d82 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Thu, 25 Jun 2026 14:37:06 -0400 Subject: [PATCH 22/31] Fix --run=0 flag parsing in CmdQueueConfigMixin The run flag was declared as ``run: bool = kw.Flag(False, validate=False)``. A ``: bool`` annotation makes kwconf coerce the flag value with bool(), so a CLI ``--run=0`` became bool('0') == True and the queue executed when the user asked it not to. Dropping the annotation restores the intended semantics: ``--run=0`` is falsy, ``--run=1`` truthy, bare ``--run`` True, and programmatic run=0/1 still works without a validation warning. ty still infers bool from the kw.Flag(False) return type. Found while migrating kwdagger to kwconf (CLI flag exercised end-to-end there). Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/cli_boilerplate.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cmd_queue/cli_boilerplate.py b/cmd_queue/cli_boilerplate.py index fa6897c..88fa699 100644 --- a/cmd_queue/cli_boilerplate.py +++ b/cmd_queue/cli_boilerplate.py @@ -415,11 +415,12 @@ class CmdQueueConfigMixin(kw.Config): >>> my_cli_main(argv=False, run=0, print_queue=1, print_commands=1, backend='serial') """ - run: bool = kw.Flag( + # NOTE: do NOT add a ``: bool`` annotation here. kwconf coerces a + # bool-annotated flag, so ``--run=0`` becomes ``bool('0')`` -> True. + # Leaving it unannotated keeps the historical semantics: ``--run=0`` is + # falsy, ``--run=1`` truthy, and bare ``--run`` True. + run = kw.Flag( False, - # ``validate=False`` keeps the ``bool`` static type while staying lenient - # about the common ``run=0`` / ``run=1`` integer idiom at runtime. - validate=False, help='if False, only prints the commands, otherwise executes them', group='cmd-queue', ) From 6c430c3ddb0cbc96b7543b5ba06b10f28ee3a0c7 Mon Sep 17 00:00:00 2001 From: Jon Crall Date: Fri, 26 Jun 2026 19:06:28 -0400 Subject: [PATCH 23/31] slurm: make monitor='none' block headlessly (consistent with tmux + docstring) The slurm backend's run(block=True, monitor='none') printed "Queue running detached" and returned immediately, while the tmux backend's none blocks via a headless poll and this backend's own docstring promised none "still blocks when block=True". A scripted run would then act on results before any job finished. Add a headless mode to monitor() (poll until every job is terminal, no live table; per-job pass/fail/skip lines still print) and route none through it. The genuinely non-blocking case is block=False, which now carries the reattach hint. Regression test: test_slurm_monitor_none_blocks. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/slurm.py | 46 ++++++++++++++++++++++++++------- tests/test_backend_execution.py | 24 +++++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index fdc7cf8..a94250b 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -862,6 +862,12 @@ def run( manifest_path = self._write_monitor_manifest() ub.cmd(f'bash {self.fpath}', verbose=3, check=True, system=system) if not block: + from rich import print as rich_print + + rich_print( + '[bold]Queue submitted (not blocking).[/bold] ' + f'Reattach with: cmd_queue monitor --manifest={manifest_path}' + ) return None if monitor == 'inline': return self.monitor(onfail=onfail, onexit=onexit) @@ -908,11 +914,15 @@ def run( if monitor == 'none': from rich import print as rich_print + # Reached only when block=True (the not-block case returned above). + # No live UI, but still block until every job is terminal — matching + # this backend's docstring and the tmux backend's headless `none`. + # The hint lets you attach a live view in another shell meanwhile. rich_print( - '[bold]Queue running detached.[/bold] ' - f'Reattach with: cmd_queue monitor --manifest={manifest_path}' + '[bold]Queue running (headless).[/bold] ' + f'Attach a live view with: cmd_queue monitor --manifest={manifest_path}' ) - return None + return self.monitor(onfail=onfail, onexit=onexit, headless=True) if monitor == 'tmux': if not ub.find_exe('tmux'): import warnings @@ -976,6 +986,7 @@ def monitor( onfail: str = '', onexit: str = '', side_session: Optional[str] = None, + headless: bool = False, ) -> Optional[Any]: """ Monitor progress until the jobs are done. @@ -992,6 +1003,12 @@ def monitor( onexit (str): currently unused for slurm (kept for API parity with the tmux backend). + headless (bool): if set, block until every job is terminal + without rendering the live status table (per-job + pass/fail/skip lines still print). This is how + ``run(block=True, monitor='none')`` blocks, matching the + tmux backend's headless ``none`` mode. + CommandLine: xdoctest -m cmd_queue.slurm_queue SlurmQueue.monitor --dev --run @@ -1215,6 +1232,7 @@ def _update_agg_state() -> None: try: import sys + import time as _time from rich.console import Group @@ -1231,12 +1249,22 @@ def _build_renderable() -> Any: return renderable, finished, None refresh_rate = 0.4 - use_keys = side_session is not None and sys.stdin.isatty() - _run_live_with_attach( - build_renderable=_build_renderable, - refresh_rate=refresh_rate, - side_session=side_session if use_keys else None, - ) + if headless: + # Block without a live display: poll until every job is + # terminal. The per-job pass/fail/skip lines still print + # (update_jobid_status runs inside update_status_table). + while True: + _table, finished = update_status_table() + if finished: + break + _time.sleep(refresh_rate) + else: + use_keys = side_session is not None and sys.stdin.isatty() + _run_live_with_attach( + build_renderable=_build_renderable, + refresh_rate=refresh_rate, + side_session=side_session if use_keys else None, + ) _update_agg_state() except KeyboardInterrupt: from rich.prompt import Confirm diff --git a/tests/test_backend_execution.py b/tests/test_backend_execution.py index 39dc19f..584add9 100644 --- a/tests/test_backend_execution.py +++ b/tests/test_backend_execution.py @@ -176,3 +176,27 @@ def test_backend_setup_failure_fails_job_and_skips_command(backend): if backend != 'slurm': assert job.fail_fpath.exists(), 'a failing setup must fail the job' assert not job.pass_fpath.exists(), 'a failed job must not pass' + + +def test_slurm_monitor_none_blocks(): + """slurm ``run(block=True, monitor='none')`` must BLOCK until every job is + terminal (headless) — matching the tmux backend and this backend's own + docstring — not return immediately and leave jobs running. Regression for the + fix where slurm's ``none`` printed "detached" and returned. + """ + import time + if 'slurm' not in _AVAILABLE: + pytest.skip('slurm backend is not available on this machine') + dpath = _work_dpath('slurm-none-blocks') + queue = _make_queue('slurm', 'cmdq-none-blocks', dpath / 'qdir') + marker = dpath / 'done.marker' + queue.submit(f'sleep 3 && echo done > "{marker}"', name='blockjob') + + t0 = time.time() + queue.run(block=True, monitor='none', with_textual=False) + dt = time.time() - t0 + + assert marker.exists(), ( + 'run() returned before the job finished -- monitor=none did not block' + ) + assert dt >= 2.5, f'run() returned too fast ({dt:.1f}s); monitor=none did not block' From a4ca3c6b4426a1d0eb7f8929d3ece59d4fcad6c1 Mon Sep 17 00:00:00 2001 From: Jon Crall Date: Fri, 26 Jun 2026 19:22:19 -0400 Subject: [PATCH 24/31] slurm: parse_scontrol_output tolerates bare tokens (varied scontrol formats) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `key, value = part.split('=', 1)` assumed every whitespace token is key=value, but a space-containing value for a key not in special_keys (or an empty token) yields a token with no '=' and raised "not enough values to unpack" — seen on some slurm versions (aiq-gpu) but not others. Split on whitespace and skip bare tokens; the keys the monitor needs (JobState, ExitCode, and the per-line special JobName/StdErr/StdOut) still parse. Regression test added. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/slurm.py | 12 ++++++++++-- tests/test_slurm_variants.py | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index a94250b..811b071 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -1448,11 +1448,19 @@ def parse_scontrol_output(output: str) -> dict: parsed_data[key] = value.strip() line = leading_part - # Now, handle the general case: split by spaces and then by "=" + # Now, handle the general case: split on whitespace and then by "=". line = line.strip() if line: - parts = line.split(' ') + parts = line.split() for part in parts: + if '=' not in part: + # A bare token: a fragment of a space-containing value whose + # key wasn't in ``special_keys`` (those are extracted per-line + # above), or an empty token. Skip rather than crash -- the + # keys the monitor needs (JobState, ExitCode, ...) are plain + # ``key=value`` and parse fine. scontrol output varies across + # slurm versions, so be lenient here. + continue key, value = part.split('=', 1) parsed_data[key] = value diff --git a/tests/test_slurm_variants.py b/tests/test_slurm_variants.py index ee6e2f0..e74629a 100644 --- a/tests/test_slurm_variants.py +++ b/tests/test_slurm_variants.py @@ -156,3 +156,22 @@ def test_slurm_teardown_executes_as_documented(): assert r.returncode != 0, 'setup failure fails the job' assert 'CMD' not in r.stdout, 'command should not run if setup fails' assert 'TD' not in r.stdout, 'teardown should not run if setup fails' + + +def test_parse_scontrol_output_tolerates_bare_tokens(): + """scontrol output varies across slurm versions: a value with a space for a + key not in special_keys (or an empty/bare token) must not crash the parser. + Regression for `ValueError: not enough values to unpack` on aiq-gpu. + """ + from cmd_queue.backends.slurm import parse_scontrol_output + + sample = '\n'.join([ + 'JobId=123 JobState=COMPLETED ExitCode=0:0', + 'JobName=smol_135_01_abc', + 'Reason=Memory Required Not Available BareToken', # space value, unknown key + 'TRES=cpu=4,mem=16G,gres/gpu=2', + ]) + info = parse_scontrol_output(sample) + assert info['JobState'] == 'COMPLETED' + assert info['ExitCode'] == '0:0' + assert info['JobName'] == 'smol_135_01_abc' From 101850135884d144ca59ea3f4d146945483426c5 Mon Sep 17 00:00:00 2001 From: Jon Crall Date: Fri, 26 Jun 2026 20:09:03 -0400 Subject: [PATCH 25/31] slurm: monitor survives purged/unknown jobs (KeyError JobState) + more states `scontrol show job ` returns no JobState once a job is purged past MinJobAge (or for an invalid id), so `info['JobState']` raised KeyError when re-attaching a monitor after a run (and could mid-run for a fast job). Recover the final state from accounting (_sacct_job_state via sacct), fall back to a terminal state, and read all fields with .get() so missing JobName/StdErr/StdOut never crash. Also treat TIMEOUT/OUT_OF_MEMORY/NODE_FAIL/etc. as terminal-failed (was 'unknown', which hung the completion check) and known transient states as still-pending. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd_queue/backends/slurm.py | 80 ++++++++++++++++++++++++++++-------- tests/test_slurm_variants.py | 13 ++++++ 2 files changed, 77 insertions(+), 16 deletions(-) diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index 811b071..4024b84 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -1070,6 +1070,18 @@ def update_jobid_status(): import rich assert job_status_table is not None + # Terminal slurm states (besides COMPLETED/CANCELLED) that mean the + # job is done and failed -- treat them as terminal so the monitor + # neither hangs waiting nor mislabels them. ``str.startswith`` takes a + # tuple of prefixes. + TERMINAL_FAIL = ( + 'FAILED', 'TIMEOUT', 'NODE_FAIL', 'OUT_OF_MEMORY', + 'BOOT_FAIL', 'DEADLINE', 'PREEMPTED', 'SPECIAL_EXIT', + ) + TRANSIENT = ( + 'PENDING', 'CONFIGURING', 'COMPLETING', 'SUSPENDED', + 'RESIZING', 'REQUEUED', 'SIGNALING', 'STAGE_OUT', 'STOPPED', + ) for row in job_status_table: if row['needs_update']: job_id = row['job_id'] @@ -1077,29 +1089,40 @@ def update_jobid_status(): # ub.cmd().stdout is typed ``str | bytes | None`` but # is always a str here. info = parse_scontrol_output(out.stdout) # ty: ignore[invalid-argument-type] - row['JobState'] = info['JobState'] + state = info.get('JobState') + if not state: + # The controller no longer knows this job (completed and + # purged past MinJobAge, or an invalid id). Recover the + # final state from accounting; if that's unavailable, + # assume terminal so the monitor neither KeyErrors nor + # hangs polling a job that will never reappear. + state = _sacct_job_state(job_id) or 'COMPLETED' + name = info.get('JobName', row['job_varname']) + row['JobState'] = state row['ExitCode'] = info.get('ExitCode', None) # https://slurm.schedmd.com/job_state_codes.html - if info['JobState'].startswith('FAILED'): - row['status'] = 'failed' - rich.print(f'[red] Failed job: {info["JobName"]}') - if info['StdErr'] == info['StdOut']: - rich.print(f'[red] * Logs: {info["StdErr"]}') - else: - rich.print(f'[red] StdErr: {info["StdErr"]}') - rich.print(f'[red] StdOut: {info["StdOut"]}') + if state.startswith('COMPLETED'): + rich.print(f'[green] Completed job: {name}') + row['status'] = 'passed' row['needs_update'] = False - elif info['JobState'].startswith('CANCELLED'): - rich.print(f'[yellow] Skip job: {info["JobName"]}') + elif state.startswith('CANCELLED'): + rich.print(f'[yellow] Skip job: {name}') row['status'] = 'skipped' row['needs_update'] = False - elif info['JobState'].startswith('COMPLETED'): - rich.print(f'[green] Completed job: {info["JobName"]}') - row['status'] = 'passed' + elif state.startswith(TERMINAL_FAIL): + row['status'] = 'failed' + rich.print(f'[red] Failed job ({state}): {name}') + stderr = info.get('StdErr') + stdout = info.get('StdOut') + if stderr and stderr == stdout: + rich.print(f'[red] * Logs: {stderr}') + elif stderr or stdout: + rich.print(f'[red] StdErr: {stderr}') + rich.print(f'[red] StdOut: {stdout}') row['needs_update'] = False - elif info['JobState'].startswith('RUNNING'): + elif state.startswith('RUNNING'): row['status'] = 'running' - elif info['JobState'].startswith('PENDING'): + elif state.startswith(TRANSIENT): row['status'] = 'pending' else: row['status'] = 'unknown' @@ -1467,6 +1490,31 @@ def parse_scontrol_output(output: str) -> dict: return parsed_data +def _sacct_job_state(job_id: Any) -> str: + """Final state of a job from slurm accounting, for jobs that ``scontrol show + job`` no longer knows (completed + purged past MinJobAge, or invalid id). + + Returns a slurm state string (e.g. ``'COMPLETED'``, ``'FAILED'``, + ``'TIMEOUT'``) for the primary job record, or ``''`` if accounting is + unavailable / the job is unknown. Best-effort: never raises. + """ + try: + out = ub.cmd( + f'sacct -j "{job_id}" --noheader --parsable2 --format=State' + ) + except Exception: + return '' + if getattr(out, 'returncode', 1) != 0: + return '' + for line in (out.stdout or '').splitlines(): + line = line.strip() + if line: + # First (primary) record; drop any trailing reason like + # "CANCELLED by 1234" so the caller's startswith() checks still work. + return line.split('|', 1)[0].strip() + return '' + + SLURM_NOTES = r""" This shows a few things you can do with slurm diff --git a/tests/test_slurm_variants.py b/tests/test_slurm_variants.py index e74629a..b44bedc 100644 --- a/tests/test_slurm_variants.py +++ b/tests/test_slurm_variants.py @@ -175,3 +175,16 @@ def test_parse_scontrol_output_tolerates_bare_tokens(): assert info['JobState'] == 'COMPLETED' assert info['ExitCode'] == '0:0' assert info['JobName'] == 'smol_135_01_abc' + + +def test_sacct_job_state_is_best_effort(): + """_sacct_job_state never raises; returns '' for an unknown/bogus job id.""" + from cmd_queue.backends.slurm import _sacct_job_state + assert _sacct_job_state('not-a-real-jobid-zzz') == '' + + +def test_parse_scontrol_missing_jobstate(): + """A purged/invalid job yields no JobState -> the monitor must use .get().""" + from cmd_queue.backends.slurm import parse_scontrol_output + assert parse_scontrol_output('').get('JobState') is None + assert parse_scontrol_output('slurm_load_jobs error: Invalid job id specified').get('JobState') is None From 0635f9cd098293c1083711a9aeac479c7002c8bf Mon Sep 17 00:00:00 2001 From: agent Date: Thu, 2 Jul 2026 15:34:43 +0000 Subject: [PATCH 26/31] fix(backends): popd guard syntax, defined HUP semantics, sbatch flag quote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs found auditing cmd_queue as the executor of infer-stack lease-bracketed jobs: - serial/tmux: the cwd restore was rendered as `[["$CHDIR_OK" == "1"]]` (no space after `[[`) — bash resolves that as an unknown command, so popd never ran and every job after a cwd job executed in the wrong working directory. - serial/tmux teardown subshell: trap HUP explicitly (exit 129 -> EXIT trap). tmux kill-session delivers SIGHUP, and the release-on-kill behavior of the lease bracket was riding on bash's undocumented run-EXIT-trap-on-unhandled- group-HUP nicety. - slurm: boolean sbatch flags rendered as `--hold"` (stray trailing quote from f'--{key}"') — a malformed sbatch line on any SLURM_SBATCH_FLAGS boolean. Co-Authored-By: Claude Fable 5 --- cmd_queue/backends/serial.py | 7 +++++- cmd_queue/backends/slurm.py | 2 +- tests/test_bash_variants.py | 46 ++++++++++++++++++++++++++++++++++++ tests/test_slurm_variants.py | 14 +++++++++++ 4 files changed, 67 insertions(+), 2 deletions(-) diff --git a/cmd_queue/backends/serial.py b/cmd_queue/backends/serial.py index e927901..f20985f 100644 --- a/cmd_queue/backends/serial.py +++ b/cmd_queue/backends/serial.py @@ -314,6 +314,11 @@ def finalize_text( ' trap __cmdq_teardown EXIT', " trap 'exit 143' TERM", " trap 'exit 130' INT", + # tmux kill-session delivers SIGHUP (never TERM). Bash happens + # to run the EXIT trap on an unhandled group-HUP, but that is + # an undocumented nicety -- trap it explicitly so teardown + # semantics on kill are defined, not lucky. + " trap 'exit 129' HUP", f' {self.command}', ')', ] @@ -354,7 +359,7 @@ def finalize_text( script.append('RETURN_CODE=$?') if self.cwd is not None: - script.append('[["$CHDIR_OK" == "1"]] && popd') + script.append('[[ "$CHDIR_OK" == "1" ]] && popd') if internal_conditionals: # Use exit code 3 for error in preamble / chdir. diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index 4024b84..742c506 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -402,7 +402,7 @@ def _coerce_gres(gpus): for key, flag in self._sbatch_flags.items(): if flag: key = key.replace('_', '-') - sbatch_args.append(f'--{key}"') + sbatch_args.append(f'--{key}') if self.depends: # TODO: other depends parts diff --git a/tests/test_bash_variants.py b/tests/test_bash_variants.py index 505e3d0..fbc97cc 100644 --- a/tests/test_bash_variants.py +++ b/tests/test_bash_variants.py @@ -715,3 +715,49 @@ def test_bashjob_exec_teardown_runs_on_sigterm(): break time.sleep(0.1) assert td_marker.exists(), 'teardown should run on SIGTERM' + + +def test_bashjob_exec_cwd_restores_worker_dir(): + """Regression: the popd guard was rendered as `[["$CHDIR_OK" ...` (no space + after `[[`), which bash resolves as an unknown command — popd never ran, so + every job AFTER a cwd job executed in the wrong working directory.""" + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + workdir = tmp_path / 'workdir' + workdir.mkdir() + + job = BashJob('true', name='job_cwd', cwd=str(workdir)) + job.log = False + job.stat_fpath = tmp_path / 'job.status.json' + job.pass_fpath = tmp_path / 'job.pass' + job.fail_fpath = tmp_path / 'job.fail' + + text = job.finalize_text(with_status=True, with_gaurds=True) + # the guard must be real bash syntax, not a `[["...` command lookup + assert '[[ "$CHDIR_OK" == "1" ]] && popd' in text + + # behavioral: after the job, the worker shell is back where it started + probe = tmp_path / 'pwd_after.txt' + script = text + f'\npwd -P > "{probe}"\n' + subprocess.run( + ['bash'], input=script, text=True, cwd=str(tmp_path), + capture_output=True, check=False, + ) + assert job.pass_fpath.exists() + assert probe.read_text().strip() == str(tmp_path.resolve()), ( + 'the worker shell must popd back after a cwd job' + ) + + +def test_bashjob_teardown_traps_hup(): + """tmux kill-session delivers SIGHUP; the teardown subshell must give it + defined exit semantics rather than relying on bash's undocumented + run-EXIT-trap-on-unhandled-group-HUP behavior.""" + with tempfile.TemporaryDirectory() as tmp_path: + tmp_path = ub.Path(tmp_path) + job, _ = _make_teardown_job( + tmp_path, 'true', teardown='echo done' + ) + text = job.finalize_text(with_status=True, with_gaurds=True) + assert "trap 'exit 129' HUP" in text + subprocess.run(['bash', '-n'], input=text, text=True, check=True) diff --git a/tests/test_slurm_variants.py b/tests/test_slurm_variants.py index b44bedc..a30f65c 100644 --- a/tests/test_slurm_variants.py +++ b/tests/test_slurm_variants.py @@ -188,3 +188,17 @@ def test_parse_scontrol_missing_jobstate(): from cmd_queue.backends.slurm import parse_scontrol_output assert parse_scontrol_output('').get('JobState') is None assert parse_scontrol_output('slurm_load_jobs error: Invalid job id specified').get('JobState') is None + + +def test_sbatch_boolean_flags_render_cleanly(): + """Regression: boolean sbatch flags rendered as `--hold"` (stray trailing + double-quote, f'--{key}"') — a malformed sbatch line that errors on submit + or quote-pairs with a later argument.""" + queue = SlurmQueue(preamble=None) + job = queue.submit('echo CMD', hold=True, requeue=True) + sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) + assert '--hold' in sbatch_args + assert '--requeue' in sbatch_args + assert not any('"' in a and not a.startswith(('--wrap', '--job-name', '--output')) + for a in sbatch_args if a in ('--hold"', '--requeue"')), sbatch_args + assert '--hold"' not in sbatch_args and '--requeue"' not in sbatch_args From 034feb12206e8ef8dcdcdf308c4221e648a9f48c Mon Sep 17 00:00:00 2001 From: agent Date: Thu, 9 Jul 2026 18:46:23 +0000 Subject: [PATCH 27/31] Add audit-driven quality improvement plan to docs/planning Nine ordered planning docs from a full-repo audit at 0635f9c: hygiene, shell-quoting hardening, core/tmux/slurm/airflow correctness, test suite, packaging/CI/docs, and dead-code cleanup. Co-Authored-By: Claude Fable 5 --- docs/planning/00-README.md | 78 +++++++ docs/planning/01-repo-hygiene-and-tooling.md | 114 +++++++++++ .../02-shell-quoting-and-name-validation.md | 121 +++++++++++ docs/planning/03-core-correctness.md | 150 ++++++++++++++ docs/planning/04-tmux-correctness.md | 145 +++++++++++++ docs/planning/05-slurm-correctness.md | 191 ++++++++++++++++++ .../06-airflow-monitor-and-cli-boilerplate.md | 121 +++++++++++ docs/planning/07-test-suite-strengthening.md | 103 ++++++++++ docs/planning/08-packaging-ci-and-docs.md | 81 ++++++++ .../planning/09-code-quality-and-dead-code.md | 107 ++++++++++ 10 files changed, 1211 insertions(+) create mode 100644 docs/planning/00-README.md create mode 100644 docs/planning/01-repo-hygiene-and-tooling.md create mode 100644 docs/planning/02-shell-quoting-and-name-validation.md create mode 100644 docs/planning/03-core-correctness.md create mode 100644 docs/planning/04-tmux-correctness.md create mode 100644 docs/planning/05-slurm-correctness.md create mode 100644 docs/planning/06-airflow-monitor-and-cli-boilerplate.md create mode 100644 docs/planning/07-test-suite-strengthening.md create mode 100644 docs/planning/08-packaging-ci-and-docs.md create mode 100644 docs/planning/09-code-quality-and-dead-code.md diff --git a/docs/planning/00-README.md b/docs/planning/00-README.md new file mode 100644 index 0000000..59e25a5 --- /dev/null +++ b/docs/planning/00-README.md @@ -0,0 +1,78 @@ +# cmd_queue quality-improvement plan + +Full-repo audit performed 2026-07-09 on branch `dev/0.3.2` at commit `0635f9c`. +Five parallel deep-audit passes (core, tmux, slurm, airflow/monitor/CLI, +utils/tests/packaging/hygiene) produced ~45 verified bugs and ~60 improvement +items; the highest-severity findings were reproduced by executing code, and the +plans note which. This directory turns those findings into an ordered, +executable plan. + +## How to execute this plan + +- **Work the phases in order.** Ordering is load-bearing: Phase 1 gives you a + green baseline to verify against; Phase 2 fixes the *defect class* (shell + quoting / name validation) that many Phase 3-5 bugs are instances of, so + doing it first avoids fixing the same lines twice. +- **One commit per numbered task (or tight cluster), regression test in the + same commit.** Every bug entry states file:line, the defect, a concrete + failure scenario, a suggested fix, and (usually) a test recipe. Line numbers + reference commit `0635f9c` — locate by symbol/content if drifted. +- **Verify each phase before moving on** using the phase's Verification + section. The universal gate: `python3 -m pytest tests/ -q`, + `./run_doctests.sh`, `ruff check cmd_queue tests`, `ty check cmd_queue tests` + all green. +- **Suggested-fix humility:** the fixes were written from careful reading and + reproduction, but if the surrounding code contradicts a suggestion, trust the + code and the stated failure scenario over the suggested patch — the scenario + is the spec. +- **Keep `CHANGELOG.md` 0.3.2 current** as you land user-visible changes + (Phase 1.6 backfills; later phases append). +- Environment note: tmux-dependent and slurm-dependent tests are skip-gated on + availability (`dev/slurm` has a test-cluster toolkit). airflow and textual + are optional extras. The audit machine lacked `kwconf` initially — it IS + declared in `requirements/runtime.txt`; `pip install -e .` first. + +## Phases + +| # | Doc | Theme | Severity of contents | Size | +|---|-----|-------|----------------------|------| +| 1 | [01-repo-hygiene-and-tooling.md](01-repo-hygiene-and-tooling.md) | Junk removal, real linting, CHANGELOG backfill | low, but unblocks everything | S | +| 2 | [02-shell-quoting-and-name-validation.md](02-shell-quoting-and-name-validation.md) | The top defect class: unescaped interpolation into generated bash; incl. command-injection paths | **critical** | M-L | +| 3 | [03-core-correctness.md](03-core-correctness.md) | base_queue/serial/CLI bugs: tee masks failures, duplicate-name corruption, broken CLI quickstart | high | M | +| 4 | [04-tmux-correctness.md](04-tmux-correctness.md) | Monitor kills running queues, uninitialized `workers`, wrong-queue kills, stale re-run state | high | M-L | +| 5 | [05-slurm-correctness.md](05-slurm-correctness.md) | squeue-parse crashes, malformed sbatch flags, DAG-integrity holes, slurmify broken OOTB | high | M-L | +| 6 | [06-airflow-monitor-and-cli-boilerplate.md](06-airflow-monitor-and-cli-boilerplate.md) | **airflow can wipe an external metadata DB**, textual kill-key crash, backend `run()` signature mismatch | critical/high | M | +| 7 | [07-test-suite-strengthening.md](07-test-suite-strengthening.md) | Tests that can't fail, uncollected files, untested modules, isolation | medium | M | +| 8 | [08-packaging-ci-and-docs.md](08-packaging-ci-and-docs.md) | Inverted airflow pins, honest CI, stale Sphinx autodoc tree | medium | S-M | +| 9 | [09-code-quality-and-dead-code.md](09-code-quality-and-dead-code.md) | Dead code, lying annotations, API consistency, perf | low | M | + +## Top 10 most severe findings (cross-reference) + +1. **airflow `run()` can drop a production Airflow DB** — ambient + `AIRFLOW__DATABASE__SQL_ALCHEMY_CONN` + unconditional `db.resetdb()` (6.1) +2. **environ export command injection** — `export K="{v}"` unescaped (2.2.3) +3. **tmux monitor kills a running queue after promising it keeps running** — + `q`-detach and Ctrl-C+decline paths both hit unconditional kill (4.1) +4. **CLI Quickstart is broken** — single-arg `shlex.quote` hack turns + `'a && b'` into one quoted word, exit 127 (2.2.7 / 3.3.1) +5. **Failing job reported PASSED** — `log=True` without pipefail captures tee's + exit code; dependents run on failed deps (3.1.1) +6. **slurm monitor dies on any multi-word squeue REASON** — cluster-wide + pending jobs crash `pd.read_csv` mid-run (5.1.1) +7. **`kill_other_queues` kills other people's queues** — ambiguous lazy-parse + of session names + headless auto-kill default (4.3) +8. **slurm duplicate names silently mis-wire the DAG** — SlurmQueue.submit + bypasses the base duplicate check (5.3.2) +9. **textual monitor crashes on the kill key** — `Screen.bind` doesn't exist in + textual >= 4 (6.2) +10. **tmux re-run reuses stale semaphores/state** — dependency order violated, + or fresh sessions instantly killed (4.4) + +## Meta: what NOT to change + +The audit also confirmed strengths worth preserving: the newer test suites +(`test_bash_variants`, `test_slurm_variants`, `test_backend_execution`) are +well-designed; CI's build-wheel-then-test-from-sandbox flow, strict/loose lock +lanes, and xdoctest-over-installed-module are all good practice; README +examples match the current API (except the CLI quoting bug above). Don't churn +these while executing the phases. diff --git a/docs/planning/01-repo-hygiene-and-tooling.md b/docs/planning/01-repo-hygiene-and-tooling.md new file mode 100644 index 0000000..2944fe5 --- /dev/null +++ b/docs/planning/01-repo-hygiene-and-tooling.md @@ -0,0 +1,114 @@ +# Phase 1: Repo hygiene and tooling baseline + +**Goal:** Remove accumulated junk, make the linter meaningful, and establish a clean +baseline so every later phase can verify itself against green checks. + +**Prerequisites:** none. **Estimated scope:** small (~1 session). + +> Line numbers throughout these planning docs reference commit `0635f9c` +> (branch `dev/0.3.2`). Locate code by symbol/content if lines have drifted. + +## 1.1 Delete untracked junk at the repo root + +These are confirmed leftovers, not project files. Delete (do NOT commit them): + +- `cmd_queue-source-2026-05-15T193853-5-6c345ab182cf.tar.gz` — source snapshot tarball +- `emissions.csv` — codecarbon log from 2024 +- `foo`, `foo.log`, `out`, `out.json`, `err.log` — scratch output (out.json is `{}`, + likely from executing a `util_bash` doctest) +- `git-of-theseus/` — repo-stats artifacts from 2025 +- `queue_root/`, `airflow_home/` — leftovers from an older airflow test that wrote + into the repo root (the current `tests/test_airflow_queue.py` correctly uses + `ub.Path.appdir`) +- `htmlcov/`, `.coverage`, `cmd_queue.egg-info/` — build/test artifacts (already + gitignored; just clean them locally if convenient) + +Decide-and-act items (default to deleting; note the decision in the commit message): + +- `dev/poc/` — two proof-of-concept scripts from 2025. Move under `dev/` tracked + content only if they still run; otherwise delete. +- `examples/slurm_example2.py` — WIP; note it calls + `Queue.create(backend='serial', partition=..., account=...)` despite the slurm + name. Either fix it to actually use the slurm backend and commit it, or delete it. + +## 1.2 Extend .gitignore for artifacts this project generates + +Add to `.gitignore`: + +``` +queue_root/ +airflow_home/ +emissions.csv +out +out.json +.mypy_cache/ +.ruff_cache/ +``` + +(`.mypy_cache/`/`.ruff_cache/` are currently invisible only because of one +machine's global excludes; other contributors will see them.) + +## 1.3 Remove dead config files + +- `appveyor.yml` — tests Python 2.7/3.5 against a package requiring `>=3.10` and + `setuptools>=77`; any real run would fail at `pip install -e .`. Delete it. +- `.rules.yml` — GitLab rules template not `include:`d by `.gitlab-ci.yml`. Delete + (or wire it in if xcookie expects it — check `git log .rules.yml` first). +- `.coveragerc` — duplicates `[tool.coverage]` in `pyproject.toml`. CI passes + `--cov-config ../pyproject.toml` and `run_tests.py` passes `pyproject.toml`, so + `.coveragerc` is unused. Delete it. +- `docs/requirements.txt` — diverges from `requirements/docs.txt` (unpinned, + includes `six`, missing `myst_parser`/`sphinx-reredirects`); `.readthedocs.yml` + uses `requirements/docs.txt`. Replace the file's contents with a single line + `-r ../requirements/docs.txt`, or delete it if nothing references it. + +## 1.4 Make lint real + +Current state: the GitLab `lint` job is `allow_failure: true`; `run_linter.sh` runs +two flake8 invocations with no `set -e` (so only the second command's exit code +counts — E9/F82 errors in `cmd_queue/` pass as long as `tests/` is clean); ruff is +fully configured in `pyproject.toml` (`[tool.ruff]`) but nothing ever runs it. + +1. Rewrite `run_linter.sh` to use ruff, with strict shell settings: + ```bash + #!/usr/bin/env bash + set -euo pipefail + ruff check cmd_queue tests + ``` + (Keep a flake8 fallback only if some workflow depends on it; ruff's default + rules cover the E9/F63/F7/F82 subset.) +2. Add `ruff` to `requirements/linting.txt`. +3. Fix the existing 11 `ruff check` findings (unused variables, unsorted imports — + 6 are `ruff check --fix`-able; review the unsafe ones manually). +4. In `.gitlab-ci.yml`, remove `allow_failure: true` from the lint job once the + tree is clean. Note the CI file is xcookie-generated; if regenerating, make the + change in the xcookie config instead of hand-editing, and record that in the + commit message. + +## 1.5 Fix misplaced module docstring + +`cmd_queue/util/util_tmux.py:1-5` — the `"""Generic tmux helpers"""` docstring sits +*after* `from __future__ import annotations`, so it is a discarded expression and +`__doc__` is `None`. Move the docstring above the import. + +## 1.6 Update CHANGELOG for 0.3.2 + +`CHANGELOG.md` has an empty `## Version 0.3.2 - Unreleased` section while the +branch already contains user-visible changes. Backfill from `git log e5d07ff..HEAD`: + +- `--run=0` flag parsing fix in `CmdQueueConfigMixin` +- CLI migration to kwconf; `cli_boilerplate` (scriptconfig) deprecated +- slurm monitor survives purged/unknown jobs; more terminal states recognized +- `parse_scontrol_output` tolerates bare tokens +- `monitor='none'` blocks headlessly (consistent with tmux) +- popd guard syntax, defined HUP semantics, sbatch flag quoting fixes + +Keep this section updated as later phases land fixes. + +## Verification + +- `git status` shows a clean tree (no untracked junk). +- `./run_linter.sh && echo OK` prints OK; introduce a deliberate unused import in + `cmd_queue/__init__.py`, confirm the script now FAILS, then revert it. +- `python3 -m pytest tests/ -q` still passes (76+ passed / few env-dependent skips). +- `ty check cmd_queue tests` passes. diff --git a/docs/planning/02-shell-quoting-and-name-validation.md b/docs/planning/02-shell-quoting-and-name-validation.md new file mode 100644 index 0000000..32349f8 --- /dev/null +++ b/docs/planning/02-shell-quoting-and-name-validation.md @@ -0,0 +1,121 @@ +# Phase 2: Shell quoting and name validation (cross-cutting hardening) + +**Goal:** Eliminate the single largest defect class in the codebase — unescaped +interpolation of user-controlled strings into generated bash — by introducing +shared helpers and tight name validation, then applying them everywhere. + +**Prerequisites:** Phase 1 (green baseline). **Estimated scope:** medium-large. +This phase should land BEFORE the per-backend bug phases because many of their +bugs are instances of this class; fixing the class first avoids double work. + +> Line numbers reference commit `0635f9c`. Locate by symbol if drifted. + +## Why this is the top defect class + +The audit found unquoted/unescaped interpolation in every generator: + +- **serial**: `export {k}="{v}"` (environ → command injection), unquoted + `mkdir -p {path}` / `printf "pass" > {path}` / `[ -f {dep.pass_fpath} ]` / + `tee {log_fpath}` / `cd {self.cwd}` +- **slurm**: `--job-name="{name}"`, `--output="{path}"`, `--{key}="{value}"`, + `scancel --name="{name}"` (values with `"` or `$(...)` corrupt argv or execute + at submit time — only `--wrap` uses `shlex.quote` today) +- **tmux**: unquoted session names and `[ ! -f {flag_path} ]` semaphore tests; + f-string tmux commands in `util_tmux` +- **util_bash**: `bash_json_dump` interpolates values into a JSON template with no + escaping and concatenates the output path unquoted +- **main.py CLI**: the single-arg `shlex.quote` "hack" that breaks compound commands + +A name like `my job`, a value like `say "hi" $(whoami)`, or a dpath under +`"My Drive"` silently corrupts status tracking or executes code. + +## 2.1 Add shared helpers + +Create `cmd_queue/util/util_quote.py` (or extend `util_bash.py`) with: + +```python +import re +import shlex + +_NAME_PAT = re.compile(r'^[A-Za-z0-9][A-Za-z0-9_.\-]*$') + +def validate_job_name(name: str) -> str: + """Raise ValueError naming the offending character if `name` is not safe + for filenames, tmux session names, and unquoted bash words. + tmux additionally forbids '.' and ':' in session names — since job/queue + names feed session names, forbid '.' and ':' too (tighten _NAME_PAT + accordingly or strip dots at the tmux layer; pick one and document it).""" + +def shquote(value) -> str: + """shlex.quote(str(value)) — one obvious spelling used by all generators.""" +``` + +Decide the exact allowed character class once, document it in the `Queue.submit` +docstring, and mention it in the CHANGELOG (it is a behavior change: names that +previously "worked" by luck may now be rejected — that is the point). + +## 2.2 Apply at the choke points + +1. **Name validation** — `base_queue.py:229-230` currently only rejects `':'`. + Replace with `validate_job_name`. Also apply to: + - `Queue.__init__` / backend `__init__`s for the queue `name` (a space in the + queue name currently breaks `TMUXMultiQueue` session creation — the driver + runs `tmux new-session -d -s {pathid} "bash"` unquoted — and breaks + `SlurmQueue.run()`'s unquoted `ub.cmd(f'bash {self.fpath}')`). + - `SlurmQueue.submit` (slurm.py:694-756), which bypasses the base check today. +2. **serial.py generators** — quote every interpolated path: + `serial.py:197-209, 223, 232, 327, 333, 736` (`cd {self.cwd}` is unquoted while + the job-level `pushd "{self.cwd}"` at :274 is quoted). Since paths are literal, + plain `"..."` wrapping is sufficient, but `shquote` is uniform and safer. +3. **serial.py environ export** — `serial.py:727-729`: + ```python + script.extend(f'export {k}={shquote(v)}' for k, v in self.environ.items()) + ``` + and validate `k` as a bash identifier (`^[A-Za-z_][A-Za-z0-9_]*$`). +4. **util_bash.bash_json_dump** (`util_bash.py:41-47`) — quote the redirect target + and JSON-escape interpolated string values (`json.dumps(value)` produces a + correctly escaped JSON string; embed that). Fix the docstring example, whose + documented `%s` usage produces invalid unquoted JSON. +5. **slurm.py sbatch lines** — `slurm.py:358, 395, 400` and the scancel calls at + `:1164, 1309`: build every flag as `'--flag=' + shquote(value)` instead of + `--flag="{value}"`. `--wrap` already does this; make the rest match. +6. **tmux semaphore + driver** — `tmux.py:335` (`'[ ! -f {} ]'.format(p)` → + quote), `tmux.py:638-642` (driver script session names), and convert the + f-string commands in `util_tmux.py:99-105, 131-133` (`_kill_session_command`, + `_capture_pane_command`, `kill_pane`) to argv lists like the rest of that class. +7. **main.py CLI single-arg hack** — `main.py:187-194`. The current code applies + `shlex.quote` to a single-element command that "needs quoting", turning the + documented `cmd_queue submit q -- 'cowsay MOO && sleep 1'` into a single quoted + word → exit 127. A one-element list is already a complete bash command line the + user quoted once in their shell: use it as-is (`bash_command = bash_command[0]` + in both branches). Only the multi-token case needs per-token `shlex.quote`+join. + +## 2.3 Regression tests + +Add `tests/test_shell_quoting.py`: + +- Job/queue names with a space, `"`, `$`, `.`, `:` → `ValueError` from `submit` + (and from queue constructors). +- `SerialQueue(environ={'V': 'say "hi" $(true)'})` → rendered script contains a + correctly quoted export; run the queue and assert the env var round-trips + verbatim (execute with `bash -c`, echo `$V` to a file, compare). +- dpath containing a space (use `tmp_path / 'has space'`) → serial queue runs, + `.pass` files land in the right place, `read_state()` reports passed. +- `bash_json_dump` with a value containing `"` and `\` → output parses with + `json.loads`. +- CLI: `cmd_queue new t && cmd_queue submit t -- 'true && true' && cmd_queue run t + --backend=serial` (drive via `main()` in-process like `tests/test_cli.py` does) + → exit 0, state shows passed. This is the Quickstart's own documented usage and + is broken today. +- Slurm (no cluster needed — text-level): `SlurmJob('echo hi', 'n', + comment='say "hi" $(whoami)').finalize_text()` contains no bare `$(` outside + single quotes; parse the sbatch line with `shlex.split` and assert the comment + survives as one argv element. + +## Verification + +- New test file passes; full suite passes; `ruff` and `ty` clean. +- `grep -n '="{' cmd_queue/backends/*.py` returns no sbatch/export-style + interpolations (spot-check any remaining hits are safe). +- Doctests still pass: `./run_doctests.sh` (quoting changes alter generated text; + update any doctest expected output deliberately, not by blind copy). diff --git a/docs/planning/03-core-correctness.md b/docs/planning/03-core-correctness.md new file mode 100644 index 0000000..62947e2 --- /dev/null +++ b/docs/planning/03-core-correctness.md @@ -0,0 +1,150 @@ +# Phase 3: Core correctness (base_queue, serial backend, CLI) + +**Goal:** Fix verified logic bugs in the queue core and serial backend. Every fix +gets a regression test in the same commit. + +**Prerequisites:** Phase 2 (several fixes below assume the quoting helpers exist). + +> Line numbers reference commit `0635f9c`. Bugs marked [verified] were reproduced +> during the audit. Ordered by severity within each file. + +## 3.1 serial backend (`cmd_queue/backends/serial.py`) + +### 3.1.1 [HIGH, verified] `log=True` without guards masks failures via tee +`serial.py:330-336, 357-359`. With `log=True` the command renders as +`(cmd) 2>&1 | tee logfile`, but `set -o pipefail` is only emitted when +`with_gaurds` is true, so with `with_status=True, with_gaurds=False` the +`RETURN_CODE=$?` captures **tee's** exit status: a failing job writes +`{"ret": 0}` and creates its `.pass` file, and dependents run on a failed dep. +**Fix:** emit `set -o pipefail`/`set +o pipefail` whenever `self.log` is set, +independent of `with_gaurds` — or capture `${PIPESTATUS[0]}` in the no-guards +status branch. +**Test:** render `BashJob('false', name='j', log=True).finalize_text( +with_status=True, with_gaurds=False)`, execute with bash, assert the status JSON +has nonzero `ret` and no `.pass` file. + +### 3.1.2 [MEDIUM, verified] `run(mode='source')` always fails where /bin/sh is dash +`serial.py:905-914`. `ub.cmd(f'source {self.fpath}', shell=True)` runs under +`/bin/sh`; `source` is a bashism → exit 127 on Debian/Ubuntu. Also note sourcing +in a child process can never affect the caller's environment, so the mode's +implied purpose is unachievable. +**Fix:** run via `bash -c 'source '` (or `executable='/bin/bash'`), or +remove the mode. Additionally `run()`'s `mode` parameter accepts any string and +executes `f'{mode} {self.fpath}'` (the `raise KeyError` is commented out at +`serial.py:882-925`) — validate against known modes. +**Test:** `SerialQueue` with one job, `run(mode='source')` → passes; +`run(mode='bogus')` → raises. + +### 3.1.3 [LOW] `exclude_tags` desynchronizes numbering and totals +`serial.py:655, 668, 753-757, 779`. `total = self.num_real_jobs` is computed +before tag filtering, so a queue of 8 with 2 excluded renders +`### Command 1 / 8` … `6 / 8` and status `{"passed": 6, "total": 8}` — reads as a +partial failure. **Fix:** count post-filter jobs for both banner and +`_CMD_QUEUE_TOTAL`. **Test:** finalize with `exclude_tags`, execute, assert +`read_state()['total']` equals the included-job count. + +### 3.1.4 [LOW] `job_details` crashes on jobs that never ran +`serial.py:927-937`. `job.stat_fpath.read_text()` is unguarded; a skipped or +never-run job raises `FileNotFoundError` mid-printout. Guard on existence like the +adjacent `log_fpath` handling. + +## 3.2 base_queue (`cmd_queue/base_queue.py`) + +### 3.2.1 [MEDIUM, verified] Duplicate-name check runs after `jobs.append` +`base_queue.py:259-267`. On `DuplicateJobError` the duplicate job is already in +`self.jobs`; a caller that catches the error (it subclasses `KeyError`; "already +submitted, skip" is a natural pattern) leaves the queue permanently corrupted — +`order_jobs`/`finalize_text`/`print_graph` all raise `Job names must be unique`. +**Fix:** check (and `_register_named_job`) before appending. While here, delete +the no-op `try: ... raise ... except Exception: raise` wrapper (`:261-265`). +**Test:** submit dup name, catch `DuplicateJobError`, assert `len(q.jobs) == 1` +and `q.finalize_text()` succeeds. + +### 3.2.2 [MEDIUM, verified] `coerce_job_depends` explodes strings into characters +`base_queue.py:65-77`. `list(depends)` on a str yields characters (historical +`ub.iterable` treated str as scalar). `Queue.submit` resolves strings first, so +this bites only direct construction: `BashJob('echo hi', name='a', +depends='job1')` → `['j','o','b','1']`, later an opaque +`AttributeError: 'str' object has no attribute 'pass_fpath'`. +**Fix:** raise `TypeError` on str input with a message pointing at +`Queue.submit(depends='name')` as the supported spelling. +**Test:** direct `BashJob(..., depends='x')` raises `TypeError`. + +### 3.2.3 [LOW-MEDIUM, verified] `change_backend` KeyError + dropped preamble +`base_queue.py:145-159`. A dependency job never submitted to the source queue → +bare `KeyError`; jobs with `command=None` are silently dropped; `self.preamble` +is not carried to the new queue. **Fix:** raise a descriptive error for +unregistered deps; copy `preamble`; document (or warn on) dropped command-less +jobs. **Test:** `change_backend` on a queue with a preamble → preamble present in +output text of the new backend. + +### 3.2.4 [LOW, verified] Auto-generated names can collide with explicit names +`base_queue.py:222-226`. Auto-name is `f'{name}-job-{num_real_jobs}'` with no +collision check: `submit(name='q-job-1')` then two anonymous submits → spurious +`DuplicateJobError` (and pre-3.2.1, a corrupted queue). **Fix:** increment until +unused. **Test:** the exact scenario above succeeds. + +### 3.2.5 [LOW] `print_commands` drops **kwargs, contradicting its docstring +`base_queue.py:368-435`. Docstring promises forwarding to `finalize_text`; the +call at `:430-435` passes only four fixed kwargs. **Fix:** pop handled keys +(`colors`, `with_rich`) and forward the rest. Note `BashJob.print_commands` +(serial.py:472) already forwards correctly. + +### 3.2.6 [LOW, verified] `style='auto'` resolves inconsistently +`base_queue.py:401, 423-424` yields `'plain'` (because +`colors = kwargs.get('colors', None)` is falsy) while `_rendering.coerce_style` +(`_rendering.py:25-27`) defaults to `'colors'`. Same argument, opposite result +between `Queue.print_commands` and `BashJob.print_commands`. +**Fix:** consolidate both the `colors` and `with_rich` deprecations plus the +`'auto'` rule inside `_rendering.coerce_style`, delete the inline copy in +`base_queue`, and have `BashJob.print_commands` call the free function directly +instead of `base_queue.Queue._coerce_style(self, ...)` with a Job as `self` +(serial.py:468-470, currently `# ty: ignore`d). + +## 3.3 CLI (`cmd_queue/main.py`) + +### 3.3.1 [HIGH, verified] Single-arg quoting hack breaks documented usage +`main.py:187-194` — covered by Phase 2 (task 2.2.7). If Phase 2 landed, just +confirm the regression test exists; otherwise fix here. + +### 3.3.2 Debug prints pollute every `show`/`run` +`main.py:175` (`print('data = ...')`), `:203-208` (`print('\n\n\n')`, +`print(f'submitkw=...')`). Gate on `config.verbose > 1` or delete. +Also `cmd_queue/util/util_yaml.py:127-129` — two live debug prints inside the +ruamel `!include` constructor pollute stdout for any CLI config using `!include` +(the commented-out sibling at `:37` shows the intent). Delete both. + +### 3.3.3 [LOW] `qname=None` writes `None.cmd_queue.json` +`main.py:121-125`. `cmd_queue new` with no positional silently creates a queue +named `None`. **Fix:** in `cli_queue_fpath`, `SystemExit('a queue name is +required')` when falsy. **Test:** drive `main()` with `new` and no name → error. + +### 3.3.4 [LOW] CLI `submit` accepts a missing command +`main.py:538-548`. `command=None` is appended to the queue file; the error only +surfaces at `run` time as `TypeError()`. Validate at submit. + +### 3.3.5 Robustness cleanups (do together) +- `main.py:434-446` — capability sniffing via `__code__.co_varnames` includes + locals; use `inspect.signature(queue.monitor).parameters`. Remove the + `try/except Exception: pass` around a plain attribute read. +- `main.py:183` — CLI calls deprecated `add_header_command`, warning on every + run; call `add_preamble_command`. +- `main.py:341-347` — `cleanup` prompts "kill these?" for an empty list; + short-circuit. `submit`/`show`/`run` on a nonexistent queue surface a raw + `FileNotFoundError`; catch and suggest `cmd_queue new `. + +## 3.4 Deferred design issues (document, don't fix here) + +- `SerialQueue.run()` returns `None` and never fails on job failure (script ends + `set +e`, exits 0) — callers must call `read_state()`. Consider returning final + state and/or a `check=True` raise-on-failed option. API change → Phase 8. +- CLI queue JSON file has no locking; concurrent `submit`s lose rows + (acknowledged in module `__todo__`). An `flock` around read-modify-write is a + cheap interim fix. + +## Verification + +Each numbered fix lands with its regression test. After the phase: +`python3 -m pytest tests/ -q` green, `./run_doctests.sh` green, `ruff`/`ty` clean, +and `python3 -m cmd_queue --help` plus the README Quickstart CLI flow work +end-to-end in a scratch directory. diff --git a/docs/planning/04-tmux-correctness.md b/docs/planning/04-tmux-correctness.md new file mode 100644 index 0000000..e012880 --- /dev/null +++ b/docs/planning/04-tmux-correctness.md @@ -0,0 +1,145 @@ +# Phase 4: tmux backend correctness + +**Goal:** Fix verified defects in `cmd_queue/backends/tmux.py` and +`cmd_queue/util/util_tmux.py`. The theme: the monitor's lifecycle decisions +(kill/keep/finished) are made from incomplete state, and re-running a queue +reuses stale on-disk state. + +**Prerequisites:** Phases 1-2 (quoting/name validation removes several tmux +failure modes: unquoted session names, `.`/`:` in names, unquoted semaphore +paths). **Testing note:** tmux is available in CI's test lanes and skip-gated +locally; write tests with the existing `is_available` skip pattern used in +`tests/test_backend_execution.py`. + +> Line numbers reference commit `0635f9c`. + +## 4.1 [HIGH] Early monitor exit kills a running queue it promised to keep alive +`tmux.py:1090-1095`. `monitor()` ends with +`if onfail == 'kill' and not agg_state.get('failed'): self.kill()` with no check +that the queue actually finished. `run()` defaults to `onfail='kill'`, and two +early-exit paths return while jobs still run: + +- pressing `q` in `_run_live_with_attach` (`:1653-1654`) — whose on-screen hint + says "[q] to stop watching (queue keeps running)" (`:1594-1596`); +- Ctrl-C in `_simple_rich_monitor` then answering **No** to "kill the procs?" + (`:1291-1296`) — the user explicitly declines, and we kill anyway. + +Both then hit the unconditional kill, destroying all worker sessions mid-run. +`_print_done_summary` also prints "Queue complete: PASSED" with partial counts. +**Fix:** thread the `finished` flag from `_build_status_table()` out of the +monitor loops; gate the `onexit`/`onfail` cleanup AND the done-summary on it. +Early exits return a distinct "detached/aborted" state. +**Test:** monkeypatch/simulate: run a 2-job queue where job 2 sleeps; drive the +monitor loop to the early-exit path; assert sessions still alive, then clean up. +(A unit-level test of the gating logic with a fake `agg_state`/`finished` is +acceptable if driving the interactive loop is impractical.) + +## 4.2 [HIGH] `self.workers` never initialized +`tmux.py:257`. `__init__` calls `self._new_workers()` and discards the result +(the method is pure). `self.workers` only exists after `order_jobs()`. +`kill()`, `read_state()`, `current_output()`, `monitor()`, and +`_write_monitor_manifest()` on a not-yet-run queue raise `AttributeError`. +**Fix:** `self.workers = self._new_workers()`. +**Test:** `q = TMUXMultiQueue(1, 'x'); q.submit('echo hi'); q.kill()` does not +raise (skip-gated on tmux availability; `kill` of never-started sessions must +also tolerate missing sessions). + +## 4.3 [HIGH] `kill_other_queues` parses session names ambiguously +`tmux.py:664-673`. `parse.Parser('cmdq_{name}_{rootid}')` uses lazy groups, but +worker names embed `_{idx:03d}_` and rootids contain underscores +(`YYYY-MM-DD_`), so `name` always matches up to the first underscore. +Consequences: (a) headless default `other_session_handler='auto'` → `'kill'` +(`:693-704`) makes queue `train` kill queue `train_v2`'s live sessions; +(b) a queue named `my_queue` never matches its own conflicting sessions. +**Fix:** match `f'{self._tmux_session_prefix}{self.name}_' + r'\d{3}_' + rootid +pattern` exactly (regex, not `parse` with lazy groups). +**Test:** pure-function test on the matcher with the four name/session +combinations above. + +## 4.4 [MEDIUM-HIGH] No stale-state cleanup on re-run +`tmux.py:574-611, 638-642, 770-777`. Semaphore flags +(`rank_flag_*.done`), per-job `.pass`/`.fail` files, and worker `state_fpath`s +are deterministic for a fixed `rootid` and never cleared. A second `run()` on +the same object: stale rank flags let later ranks start before earlier ranks +finish (dependency order violated); stale `.pass` files satisfy dep checks for +jobs still re-running; a stale `status: done` state file read in the window +before bash writes `init` makes the monitor declare finished instantly and (with +default `onfail='kill'`) kill the fresh sessions. The driver's +`tmux new-session -d -s {pathid}` also fails on the duplicate name (driver has +no `set -e`) and then `tmux send` types into the *old* session. +**Fix:** in `run()`/`write()`: clear the semaphore dir, remove per-job status +flag files and worker state files before launching; fail loudly (or kill first) +if the target session already exists. +**Test:** run a small queue twice on the same object; second run passes with +correct ordering (job with a dep on a sleeping job must not start early — encode +via timestamps written by each job). + +## 4.5 [MEDIUM] Dead worker session blocks the monitor forever +`tmux.py:955-969, 1298-1358`. Completion is judged purely from worker state +files; worker scripts are *sourced* into the session's interactive bash +(`:639-641`), so a job script that calls `exit`, or an externally killed/OOM'd +session, leaves `status: run` forever and `run(block=True)` never returns +(production default has no timeout, `util_tmux.py:47`); later-rank sessions spin +in the `sleep 1` semaphore loop as orphans. +**Fix:** each poll tick, cross-check `tmux.has_session(worker.pathid)`; session +gone + state != done → mark worker failed/aborted, stop blocking, and reflect it +in `agg_state`. +**Test:** start a queue whose job is `sleep 60`, kill the worker session +out-of-band, assert `run(block=True)` returns with a failure within a few ticks. + +## 4.6 [MEDIUM] `capture-pane` hardcodes `:0.0` +`util_tmux.py:103-105`. Users with `base-index 1`/`pane-base-index 1` (very +common tmux config) have no window 0 → every `onexit='capture'` / +`current_output()` call errors. +**Fix:** target just the session (`-t ` captures its active pane) or +resolve the first pane via `list-panes -F '#{window_index}.#{pane_index}'`. + +## 4.7 [MEDIUM] Ctrl-C in `block_with_attach_prompt` masquerades as completion +`util_tmux.py:298-301` + `tmux.py:941-949`. The enclosing +`except KeyboardInterrupt: return` makes an abort indistinguishable from +"finished"; the caller prints "Queue complete: PASSED passed=3 ... total=40" +while 37 jobs still run. +**Fix:** propagate KeyboardInterrupt or return an "aborted" sentinel; only print +the completion summary when `is_finished_fn()` returned true. (Coordinate with +4.1's finished-flag threading.) + +## 4.8 [LOW-MEDIUM] Explicit `with_textual=True` without textual → TypeError +`tmux.py:1079-1087, 1113`. Only `'auto'` is mapped through the availability +check; explicit `True` with the import failed (`:1680-1687`, +`CmdQueueMonitorApp = None`) crashes after jobs launched. +**Fix:** coerce truthy `with_textual` to False with a warning when the app class +is None. **Test:** monkeypatch `CmdQueueMonitorApp = None`, call with +`with_textual=True`, assert no crash + warning. + +## 4.9 [LOW-MEDIUM] Textual monitor has no "stop watching without killing" path +`tmux.py:1110-1140` + `monitor_app.py:241-242`. `q` exits the app with +`graceful_exit=False` → kill-confirm prompt; answering No re-launches the app. +The hybrid hint advertises "[q] to stop watching (queue keeps running)". +**Fix:** treat `q` without kill-confirmation as "stop watching" and return +(requires 4.1 so returning doesn't kill). Depends on Phase 6's monitor_app fix +for the `k` binding crash — coordinate. + +## 4.10 [LOW] Hybrid side-session name collision aborts `run()` mid-flight +`tmux.py:861-874` + `util_tmux.py:183`. `spawn_monitor_session` uses +`check=True` on `tmux new-session -s cmdq-monitor-{pathid}`; a stale session +from a hard-killed previous run makes `run()` raise after workers launched (no +monitoring, no cleanup, no summary). +**Fix:** kill or reuse the existing session first (`new-session -A` or +pre-`kill_session`). Also move the "Spawned attachable monitor..." message +(`:864-867`) to after the spawn succeeds. + +## 4.11 [LOW] `list_panes` crashes on missing session / empty output +`util_tmux.py:383-397`. Unchecked `ub.cmd`; empty stdout → +`json.loads('')` raises. Check the return code and skip empty lines. (Note: +`list_panes`/`kill_pane` currently have no in-repo callers — if 9.x dead-code +cleanup removes them, skip this.) + +## Verification + +- New/updated tests pass locally with tmux installed (`tmux -V`), and are + properly skip-gated where tmux is unavailable. +- Manual smoke: `python examples/tmux_example.py` (or the README tmux snippet) + through `run(monitor='hybrid')`; press `q`; verify with `tmux ls` that + `cmdq_*` worker sessions survive; re-attach monitor via `cmd_queue monitor`; + let it finish; verify cleanup per `onexit`. +- Full suite + doctests + ruff + ty green. diff --git a/docs/planning/05-slurm-correctness.md b/docs/planning/05-slurm-correctness.md new file mode 100644 index 0000000..e6ee0e7 --- /dev/null +++ b/docs/planning/05-slurm-correctness.md @@ -0,0 +1,191 @@ +# Phase 5: slurm backend correctness + +**Goal:** Fix verified defects in `cmd_queue/backends/slurm.py` and +`cmd_queue/slurmify.py`. Highest-impact clusters: (a) squeue scraping that +crashes real monitors, (b) out-of-the-box malformed sbatch lines, +(c) submission/DAG integrity holes. + +**Prerequisites:** Phases 1-2 (sbatch value quoting is Phase 2 task 2.2.5). +**Testing note:** most fixes are testable at the text level +(`finalize_text()` / rendered script assertions) without a cluster — extend +`tests/test_slurm_variants.py`. Live behavior is covered by the skip-gated +`tests/test_backend_execution.py` lane; `dev/slurm` contains a toolkit for +spinning up a test slurm. Recent commits already fixed: boolean-flag trailing +quote, scontrol bare-token parsing, KeyError on purged jobs, headless +`monitor='none'` — do not redo those. + +> Line numbers reference commit `0635f9c`. + +## 5.1 Monitor robustness (crashes observed in real runs) + +### 5.1.1 [HIGH] Multi-word squeue REASON kills the monitor +`slurm.py:1140-1142`. `update_status_table` parses +`squeue --format="%i %P %j %u %t %M %D %R"` with `pd.read_csv(stream, sep=' ')`. +`%R` (NODELIST/REASON) routinely contains spaces for pending jobs — e.g. +`(Nodes required for job are DOWN, DRAINED or reserved)` → `ParserError: +Expected 8 fields ... saw 16`. squeue output is not filtered to this queue, so +ANY job on the cluster in such a state kills the live monitor mid-run. Even when +pandas doesn't raise, columns silently misalign, breaking the NAME filter and +the `(DependencyNeverSatisfied)` auto-scancel. +**Fix (preferred):** use `squeue --json` when available (slurm >= 20.02; +`is_available` already version-gates `sinfo --json`, reuse that plumbing), with +a text fallback that splits each line manually via `line.split(' ', 7)` so the +REASON tail stays intact. Drop `pd.read_csv` for this entirely. + +### 5.1.2 [HIGH] Transient squeue failure crashes the monitor / fakes completion +`slurm.py:1140-1142` and `:960-966`. `ub.cmd('squeue ...')` return code is never +checked: a slurmctld hiccup gives empty stdout → `pd.read_csv` raises +`EmptyDataError` in the inline monitor; in `monitor='tmux'` mode `_is_finished` +treats the same empty output as "finished" and returns while jobs still run. +**Fix:** check `info['ret']`; on failure skip the tick (keep previous state); +in `_is_finished`, treat failed squeue as "not finished". + +### 5.1.3 [MEDIUM] `refresh_rate` parameter unconditionally discarded +`slurm.py:1274` overwrites the `refresh_rate: float = 0.4` argument from `:982` +with a local `= 0.4`. Callers cannot slow the poll, which currently runs one +`squeue` plus one `scontrol show job` **per unfinished job** per 0.4s tick — +hammering slurmctld on large queues. **Fix:** delete line 1274. Then (perf, +same area): replace per-job `scontrol show job` (`:1085-1099`) with one batched +`sacct -j id1,id2,... --format=JobID,State,ExitCode --parsable2` (or +`squeue --jobs=... --json`) per tick. + +### 5.1.4 [MEDIUM] Ctrl-C in non-interactive monitor raises; partial state lost +`slurm.py:1292-1298`. The `KeyboardInterrupt` handler calls +`rich.prompt.Confirm.ask` unconditionally (EOFError when stdin is not a tty, +e.g. nohup/CI) and returns `agg_state` without `_update_agg_state()` (empty +dict → callers checking `result['failed']` break; `onfail='kill'` skipped). +**Fix:** `_update_agg_state()` before returning; prompt only when +`sys.stdin.isatty()`, defaulting to "don't kill". Fix the prompt typo +("do you to kill the procs?"). + +### 5.1.5 [LOW] Unknown job state optimistically reported COMPLETED +`slurm.py:1099`. `_sacct_job_state(job_id) or 'COMPLETED'` marks jobs with no +accounting info as passed (e.g. bogus ids from a failed sbatch, see 5.3.1). +**Fix:** introduce a distinct terminal `'lost'` status counted separately +(still terminal for the finish check). + +## 5.2 sbatch flag construction + +### 5.2.1 [HIGH] `mem_per_cpu`/`container` misclassified as boolean flags +`slurm.py:219` (`mem_per_cpu` in `SLURM_SBATCH_FLAGS` while also in KVARGS at +`:182`) and `:206` (`container` in FLAGS only, though it takes a value). +`mem_per_cpu='4G'` renders `--mem-per-cpu="4G" --mem-per-cpu` (sbatch: option +requires an argument); `container='/img.sqfs'` renders bare `--container`, +silently discarding the path. **Fix:** remove `mem_per_cpu` from FLAGS; move +`container` to KVARGS. Then audit the whole FLAGS/KVARGS split against +`sbatch --help` (the lists were regex-generated; check for other misfilings). +**Test:** text-level render assertions for both kwargs. + +### 5.2.2 [MEDIUM] Queue-level boolean flags silently dropped +`slurm.py:751`. `submit` merges `_kwargs = self._sbatch_kvargs | kwargs`; +`self._sbatch_flags` (captured at `:583-584`) is never used, so +`SlurmQueue('t', requeue=True)` is a silent no-op. +**Fix:** `_kwargs = self._sbatch_kvargs | self._sbatch_flags | kwargs`. +**Test:** queue-level `requeue=True` appears as `--requeue` in every job line. + +### 5.2.3 [LOW] Typo'd kwargs silently swallowed +`slurm.py:285, 566`. Unrecognized kwargs land in `unused_kwargs` without a +whisper (`partion='gpu'` drops the resource request). **Fix:** warn (or raise) +when `unused_kwargs` is non-empty. + +## 5.3 Submission / DAG integrity + +### 5.3.1 [HIGH] Failed sbatch produces invalid jobids JSON → JSONDecodeError +`slurm.py:816-822` (unquoted `%s` in the jobids JSON printf) and `:1049-1066` +(only `UnableToMonitor` caught around `json.loads`). The generated script has no +`set -e`; a failed sbatch leaves `JOB_nnn` empty → +`printf '{"JOB_000": %s}' ""` writes `{"JOB_000": }` (invalid JSON); the script +still exits 0 (last printf succeeds) so `check=True` passes, then the monitor +raises `JSONDecodeError` instead of reporting the submission failure. With +`--clusters`, `sbatch --parsable` prints `jobid;cluster` — invalid both as JSON +and inside `--dependency=afterok:${JOB_nnn}`. +**Fix:** quote values in the printf (`'"%s"'`); post-parse, treat empty or +non-numeric ids as submission failures with a clear error; strip `;cluster` +suffixes both in the JSON and before interpolating into `--dependency`. +**Test:** simulate a jobids file with an empty value; monitor reports a +submission failure instead of raising JSONDecodeError. + +### 5.3.2 [HIGH] Duplicate job names accepted → dependencies wired to wrong job +`slurm.py:694-756` (no duplicate check; base class has one at +base_queue.py:262) and `:804-806` (`jobname_to_varname[name]` overwritten). +Two jobs named `dup` → string deps resolve to the second; even SlurmJob-object +deps on the first resolve to the second job's id. Wrong DAG, no error. +**Fix:** replicate the base-class duplicate check in `SlurmQueue.submit` (or +better: refactor so `SlurmQueue.submit` calls shared base logic — coordinate +with Phase 3.2.1's ordering fix). **Test:** duplicate submit raises +`DuplicateJobError`. + +### 5.3.3 [MEDIUM] `exclude_tags` + dependent job → invalid/hijackable dependency +`slurm.py:426` (fallback `$(squeue --noheader --format %i --name '')`) +with `:795-806` (excluded jobs get no varname). A dep on an excluded job renders +the squeue-by-name fallback; the job was never submitted, so it expands empty → +`sbatch: error: Job dependency problem`; the dependent and everything downstream +never submit. Worse: if ANY cluster user has a job with that name, the +dependency silently attaches to a stranger's job; multi-line output from +several same-name jobs also breaks the flag. +**Fix:** at `finalize_text` time, drop dependencies on excluded jobs with a +warning (matching serial/tmux semantics of "excluded means treat as satisfied") +or fail fast; if keeping the runtime fallback for cross-queue deps, add +`--user=$USER`, take the last line only, and guard empty expansion in bash. +**Test:** text-level — excluded dep produces no `--dependency` (or a guarded +form), and a warning is emitted. + +## 5.4 Availability probe + +### [MEDIUM] `is_available` can raise UnboundLocalError; dead branch +`slurm.py:663-679`. Within the `>= 21` path the `else: scontrol show nodes +--json` branch is unreachable, and JSON with neither `'nodes'` nor `'sinfo'` +keys leaves `nodes` unbound → availability *probe* crashes `run()` instead of +returning False. sinfo's JSON shape demonstrably changed across v21/22/23. +**Fix:** `nodes = out.get('nodes') or [i['node'] for i in out.get('sinfo', [])]`; +wrap the whole probe in try/except returning False; delete the dead branch; +also guard the `sinfo --version` parse (`:652-654`). +**Test:** feed the parser both known JSON shapes and a bogus shape. + +## 5.5 slurmify CLI (`cmd_queue/slurmify.py`) — broken out of the box + +1. **[HIGH] `partition` default is `1`** (`slurmify.py:62`) → every no-arg run + submits `--partition="1"` → `sbatch: error: invalid partition`. Fix: default + `None`. +2. **[HIGH] `--depends` always raises KeyError** (`slurmify.py:97,129-130` with + `slurm.py:746-749`): names are resolved via the fresh queue's `named_jobs`, + which is empty; even the docstring's own `--depends=None` csv-parses to + `['None']` and crashes. Fix: accept numeric job ids passed through as + `--dependency=afterok:`, and/or translate names via a guarded + squeue-by-name (`--user=$USER`); error clearly otherwise. Fix the docstring + example. +3. **[MEDIUM] `--gpus=1` requests zero GPUs**: `parser='csv'` yields a list; + `_coerce_gres` (`slurm.py:386-387`) maps any list to `'gpu:0'`. Fix: parse + as int / map 1-element lists to `gpu:`; also fix the help strings that say + "tmux backend only" in a slurm-only tool. +4. **[MEDIUM] single-token command double-quoted** (`slurmify.py:114-121`): + same class as main.py's hack (Phase 2.2.7) — a 1-element command is already a + complete command line; pass through unmodified. +5. **[LOW] `command=None`** (no positional) flows into `queue.submit(None)` and + fails opaquely later — validate up front. +6. Add `tests/test_slurmify.py` covering all of the above at the + rendered-text level (no cluster needed: build the queue, inspect + `finalize_text()`). + +## 5.6 Smaller items (fold into the above commits where convenient) + +- `kill()` cancels by `--name` only (`slurm.py:1306-1311`) — user-supplied + names collide across queues/users; prefer `scancel ` from the jobids + JSON, falling back to `--name` + `--user=$USER`. +- Job stdout written with `.sh` extension (`slurm.py:728`) — use `.log`/`.out`. +- `parse_scontrol_output` (`slurm.py:1401-1438`): example is not a doctest + (no `>>>`) so never runs; the first-special-key regex mis-splits lines like + `Partition=priority AllocNode:Sid=...`. Convert to a real doctest; match the + last special key per line. +- Queue `name` validation + `run()`'s unquoted `ub.cmd(f'bash {self.fpath}')` + — covered by Phase 2; verify here for slurm specifically. + +## Verification + +- `tests/test_slurm_variants.py` + new text-level tests green without a cluster. +- If feasible, spin up the `dev/slurm` test cluster and run + `tests/test_backend_execution.py` + `examples/slurm_example.py` end-to-end; + verify the monitor survives a pending job with a multi-word reason (submit + more jobs than the toy cluster has nodes to force `(Resources)`/priority + reasons). +- Full suite + doctests + ruff + ty green. diff --git a/docs/planning/06-airflow-monitor-and-cli-boilerplate.md b/docs/planning/06-airflow-monitor-and-cli-boilerplate.md new file mode 100644 index 0000000..3103df0 --- /dev/null +++ b/docs/planning/06-airflow-monitor-and-cli-boilerplate.md @@ -0,0 +1,121 @@ +# Phase 6: airflow backend, textual monitor, CLI boilerplate + +**Goal:** Fix a destructive airflow bug, a crash in the interactive monitor, and +assorted CLI-layer defects. + +**Prerequisites:** Phase 1. Independent of Phases 4-5 except where noted. +**Testing note:** airflow is an optional extra (`pip install cmd_queue[airflow]`, +pinned `apache-airflow>=3.1.3`); airflow tests use `importorskip`. The textual +fix is testable with textual's `run_test` harness (textual is in the optional +extras and was verified against 8.2.8). + +> Line numbers reference commit `0635f9c`. + +## 6.1 [CRITICAL] `AirflowQueue.run()` can wipe an external Airflow metadata DB +`backends/airflow.py:197-200, 253-254`. `_airflow_env` only **setdefault**s +`AIRFLOW__DATABASE__SQL_ALCHEMY_CONN`, so a value exported in the user's shell +(standard for anyone operating a real Airflow deployment) is kept — and `run()` +then unconditionally prefers `db.resetdb()` (drops and recreates ALL tables, +true on every supported Airflow 3.x since `hasattr(db, 'resetdb')`). A user with +a production connection string in their environment who runs a cmd_queue +airflow queue destroys their production metadata DB. Lesser variant: a shared +`airflow_home=` (as the module doctest at line 18 uses) erases all prior run +history on every `run()`. +**Fix:** force-set (not setdefault) the connection string to the per-queue +sqlite path; only `resetdb()` when that sqlite file does not yet exist, +preferring `db.migratedb()`/upgrade otherwise. Add a test asserting the env +override wins over an ambient `AIRFLOW__DATABASE__SQL_ALCHEMY_CONN`. + +## 6.2 [HIGH] Pressing `k` (kill) in the textual monitor crashes the UI +`monitor_app.py:118-120`. `ConfirmKillScreen.on_mount` calls `self.bind(...)`, +but textual defines `bind()` only on `App`, never on `Screen`/`ModalScreen` +(verified on textual 8.2.8; no 4.x+ version has it). Since +`TMUXMultiQueue._textual_monitor` always passes `kill_fn=self.kill` +(tmux.py:1113-1115), every interactive tmux-monitor user who presses `k` gets +an `AttributeError` crash instead of a confirm dialog. +**Fix:** replace the `on_mount` binds with a `BINDINGS` class variable on +`ConfirmKillScreen`: `[('y', 'confirm_kill', ...), ('n', 'cancel_kill', ...)]`. +The `q`/`k`/`a` binds in `CmdQueueMonitorApp.on_mount` are fine (`App.bind` +exists). +**Test:** textual `run_test` harness — press `k`, assert the modal appears; +press `n`, assert it dismisses without killing; press `k` then `y`, assert +`kill_fn` was called. (Coordinate with Phase 4.9, which adds a +stop-watching-without-kill path through this same app.) + +## 6.3 [HIGH] `--backend=airflow --run=1` via the CLI boilerplate crashes +`backends/airflow.py:226`. `AirflowQueue.run(block, system)` takes no +`**kwargs`, but both `CMDQueueConfig.run_queue` (cli_boilerplate.py:372-377) +and `CmdQueueConfigMixin.run_queue` (cli_boilerplate.py:596-602) always pass +`with_textual=`, `other_session_handler=`, `monitor=` → +`TypeError: run() got an unexpected keyword argument` after the queue is built. +**Fix:** add `**kwargs` to `AirflowQueue.run` matching the other backends. +**Test:** extend `tests/test_airflow_queue.py` (importorskip'd) to drive +`run_queue`-style kwargs; or at minimum an inspection test that every +registered backend's `run` accepts the boilerplate kwargs (this also protects +future backends — see Phase 7's backend-contract suite). + +## 6.4 [MEDIUM] `demo()` broken on every supported Airflow version +`backends/airflow.py:528-547`. Imports `airflow.operators.bash` (removed in +Airflow 3.0) and calls `dag.run()` (removed; `dag.test()` is the replacement +the real `run()` already uses). Port to +`airflow.providers.standard.operators.bash` + `dag.test()`, or delete `demo()` +and the `__main__` block. Also fix stale CommandLine pointers +(`airflow.py:6-7, 553`) that target the `cmd_queue.airflow_queue` shim (whose +docstring has no doctests), and refresh the `+SKIP`'d class doctest +(`:86-96`) that calls a pre-refactor `read_state()` flow. + +## 6.5 [MEDIUM] `onfail=kill` means opposite things in tmux vs slurm +`main.py:385-394` (help text: "`kill` cancels still-running workers" on +failure), vs tmux (`tmux.py:1023-1027, 1093`): kill on *clean exit*, keep on +failure; slurm (`slurm.py:998-1002`): kill on failure, as the help says. +**Fix (decide once, document in CHANGELOG):** reconcile to one semantic across +backends — recommended: `onfail='kill'` cancels remaining work when a failure +occurs (the plain reading, and slurm's behavior), and tmux's +keep-alive-for-debugging behavior moves to `onfail='keep'` (already the tmux +docstring's vocabulary). Update `main.py` help, both backends' docstrings, and +Phase 4.1's gating logic together. + +## 6.6 [LOW-MEDIUM] `resolve_manifest` lets a stray cwd file shadow a queue name +`monitor_manifest.py:88-94`. `candidate.is_file()` is checked before the +active-index lookup, so `cmd_queue monitor foo` with an unrelated `./foo` file +(this repo currently has one!) dies with `JSONDecodeError`. Validate that the +file parses as a manifest (or has the expected suffix) before accepting; +fall through to the index otherwise. **Test:** create a junk file matching a +registered name; monitor resolves the registered queue. + +## 6.7 [LOW] airflow dead parameters and duplication +- `AirflowJob.__init__` accepts `partition` (`airflow.py:52`) but never stores + it; `submit()` computes an `output_fpath` log path (`:442-443`) that + `finalize_text` never uses, so the promised per-job log is never written. + Wire the log through (redirect in `bash_command`) or drop both. +- `read_state` (`:332-345`): try/except bodies identical except the import — + deduplicate. +- BashOperator Jinja-templates `bash_command`: user commands containing + `{{ }}` are rewritten and commands ending in `.sh` trigger "template file not + found". Document, or render tasks with templating disabled + (`Template fields`/`render_template_as_native_obj` — investigate the + supported knob on Airflow 3.x). + +## 6.8 [LOW] cli_boilerplate cleanups +- `run_queue` duplicated verbatim between `CMDQueueConfig` and + `CmdQueueConfigMixin` (cli_boilerplate.py:327-377 vs 554-602), and it mutates + the caller's config in place (`config['print_commands'] = 1`). Factor into a + shared helper using locals. (The class is deprecated but still shipped — + keep it working until removal, see Phase 9.) +- `cli_boilerplate.py:324` calls deprecated `add_header_command` → use + `add_preamble_command` (the Mixin already does at `:551`). +- `monitor_app.py`: `JobTable.refresh_status` runs `table_fn` synchronously on + the UI loop every 0.5s; tmux's table_fn shells out — offload via + `run_worker`/thread so a slow call doesn't stall the UI. +- `main.py:146-148` `workers` help ("number of concurrent queues") vs + `tmux_workers` in cli_boilerplate — align wording. + +## Verification + +- With airflow installed (`pip install -e .[airflow]`): `pytest + tests/test_airflow_queue.py` green; run the module doctest flow with a + poisoned `AIRFLOW__DATABASE__SQL_ALCHEMY_CONN` env var pointing at a scratch + sqlite file and assert that file is untouched. +- With textual installed: monitor `run_test` tests green; manual smoke of the + tmux hybrid monitor (`k` → dialog → `n` keeps running; `y` kills). +- Full suite + doctests + ruff + ty green. diff --git a/docs/planning/07-test-suite-strengthening.md b/docs/planning/07-test-suite-strengthening.md new file mode 100644 index 0000000..8ba8c7f --- /dev/null +++ b/docs/planning/07-test-suite-strengthening.md @@ -0,0 +1,103 @@ +# Phase 7: Test suite strengthening + +**Goal:** Close the gaps that let the Phase 2-6 bugs survive: assertion-free +tests, uncollected files, untested modules, and shared-state hazards. + +**Prerequisites:** Phases 2-6 each added targeted regression tests; this phase +is the systematic sweep. Current baseline: 82 tests collect; 76 pass / 7 skip +locally in ~11s. The newer suites (`test_bash_variants`, `test_slurm_variants`, +`test_backend_execution`, `test_block_timeout`, `test_tmux_attach`) are strong — +behavioral, regression-documented, properly skip-gated. Match their style. + +## 7.1 Fix tests that cannot fail + +- `tests/tests_mixed_hardware_tmux.py` — **never collected** (pytest's default + pattern is `test_*.py`, this is `tests_*.py`), and even if renamed it has + zero assertions (only `print_commands()`/`print_graph()`). Rename to + `test_mixed_hardware_tmux.py` and assert something real: e.g. the rendered + worker scripts split `CUDA_VISIBLE_DEVICES` assignments across the two + `gres` entries. Or delete it if redundant with `test_backend_execution`. +- `tests/test_errors.py::test_failures_on_each_backend` — submits jobs named + "job2 never runs" etc., then only calls `run()` and discards `read_state()`. + Assert on `read_state()`: expected failed/skipped/passed counts per backend — + this is the test that should have caught the tee/pipefail bug (Phase 3.1.1). +- `tests/test_package_metadata.py` — unconditionally `@pytest.mark.skip` with a + `pass` body; inflates counts, tests nothing. Delete it (move its docstring + rationale to a comment elsewhere if worth keeping). + +## 7.2 Add unit tests for untested modules + +No direct tests exist anywhere for (line counts as of `0635f9c`): + +| Module | Priority | What to cover | +|---|---|---| +| `util/util_yaml.py` (443) | high | `Yaml.coerce/loads/dumps` round-trips, `!include` (no debug prints — regression for Phase 3.3.2), pyyaml-vs-ruamel backends | +| `slurmify.py` (149) | high | Phase 5.5 items — rendered-text level | +| `monitor_manifest.py` (131) | high | resolve by name/path, stray-file shadowing (Phase 6.6), stale-manifest handling | +| `util/util_bash.py` (48) | high | `bash_json_dump` output parses as JSON incl. quoting edge cases (Phase 2.2.4) | +| `util/richer.py` / `util/texter.py` | medium | one test that every name in `__all__` is importable — this alone would have caught both current bugs (see 7.5) | +| `cli_boilerplate.py` (602) | medium | deprecated but shipped: one smoke per config class through `create_queue`/`run_queue` with `--run=0` | +| `util/util_algo.py`, `util/util_tags.py`, `util/util_networkx.py` | low | doctest coverage may suffice — ensure doctests actually run in CI (they run via the xdoctest lane over the installed module; verify these modules are included) | +| `monitor_app.py` | medium | textual `run_test` coverage added in Phase 6.2 | + +## 7.3 Backend-contract suite + +`tests/test_backend_contract.py` exists — extend it so every registered backend +is checked for: + +- `run(**kw)` accepts the boilerplate kwargs (`with_textual`, + `other_session_handler`, `monitor`, `block`, `onfail`, `onexit`) — catches + Phase 6.3's airflow TypeError class permanently. +- `submit` rejects duplicate names (catches Phase 5.3.2's SlurmQueue bypass). +- `run()`/`monitor()` return shapes agree with annotations (tmux's currently + lie — Phase 9 fixes the annotations; the contract test keeps them honest). +- `read_state()` returns the agreed keys (`passed/failed/skipped/total`...) + for a trivially passing and a trivially failing queue (serial always; + tmux/slurm skip-gated). + +## 7.4 Test isolation + +- Add `tests/conftest.py`. `tests/test_cli.py:8` reuses a shared + `ub.Path.appdir('cmd_queue/tests/tests_cli')` and fixed queue names + `testqueue1/2` — collides under `pytest -n` or two concurrent checkouts. + Move CLI tests to `tmp_path` (point the CLI's queue dir there via its env + var/config knob; add one if none exists — check `cli_queue_fpath`). + `test_backend_execution.py` may keep appdir (documented reason: slurm needs a + shared filesystem) — leave it. +- Known timing-sensitive spots (acceptable, just don't regress): + `test_backend_execution.py:193-201` (`dt >= 2.5` around a `sleep 3` + slurm job); `test_bash_variants.py:705` (1s sleep before SIGTERM, mitigated + by a marker-poll loop). + +## 7.5 Two concrete util bugs to fix alongside their new tests + +- `util/richer.py:125-194` — `__all__` advertises `get_console`, `inspect`, + `print`, `reconfigure`, but `lazy_import` was generated with + `submod_attrs={}`, so those four raise AttributeError/ImportError (verified); + `from cmd_queue.util.richer import *` crashes. Regenerate with mkinit adding + the attrs, or drop them from `__all__`. +- `util/texter.py` — mirrors a ~2021 textual API (`background`, `layout_map`, + `page`, `view`, ... submodules) that does not exist in the required + `textual>=4.0.0`; `EAGER_IMPORT=1` makes importing it crash; nothing in the + package imports it. **Delete the module** (and its docs page) — preferred — + or regenerate against textual 4.x. +- Also from the same audit: `util/__init__.py:45-57` lazy-exports only + `util_algo`/`util_networkx`; attribute access to `cmd_queue.util.util_yaml` + etc. raises AttributeError. Regenerate the mkinit block to list all + submodules. `util_yaml.py:83`: restore the commented-out `@ub.memoize` on + `_custom_new_ruaml_yaml_obj` (every dumps/loads currently rebuilds classes + and re-registers representers). `util_yaml.py:267`: pyyaml backend uses + full `yaml.Loader` on user-supplied config strings — switch to `SafeLoader` + unless arbitrary-object loading is a documented feature. +- `util_tmux.py:35-36`: `resolve_block_timeout(explicit='none')` raises + `ValueError` while the env-var path accepts `'none'` — accept it in both. + +## Verification + +- `python3 -m pytest tests/ -q` — every test file collects (no `tests_*.py` + strays), no unconditional skips remain, count meaningfully higher than the + 82 baseline. +- `python3 -m pytest tests/ -n 4` (pytest-xdist, add to test requirements if + absent) passes — proves the isolation work. +- Coverage: `python3 run_tests.py`; target: every module in the 7.2 table has + >0% direct coverage; note overall % in the phase-completion commit message. diff --git a/docs/planning/08-packaging-ci-and-docs.md b/docs/planning/08-packaging-ci-and-docs.md new file mode 100644 index 0000000..92c030a --- /dev/null +++ b/docs/planning/08-packaging-ci-and-docs.md @@ -0,0 +1,81 @@ +# Phase 8: Packaging, CI, and documentation + +**Goal:** Make declared dependencies true, CI honest, and the Sphinx docs match +the post-refactor package layout. + +**Prerequisites:** Phase 1 (deleted dead config). Mostly independent of 2-7. + +## 8.1 Requirements fixes + +- `requirements/airflow.txt:2-5` — version floors are **inverted**: py3.11 + requires `apache-airflow>=3.2.1` while py3.12/3.13 require only `>=3.1.3`. + Almost certainly a typo — fix so newer pythons get the newer floor (verify + against airflow's actual python-support matrix before choosing values). +- Same file: the py3.14 row is commented out, so + `pip install cmd_queue[airflow]` on 3.14 silently installs no airflow, while + the CI `full-strict/cp314` job still passes `INSTALL_EXTRAS=...airflow` — + the extra resolves to nothing, airflow tests skip, job goes green as a + de-facto minimal job. Either add a 3.14 row (if airflow supports it now) or + make the cp314 full lanes explicitly non-airflow so nobody mistakes them for + airflow coverage. +- `requirements/runtime.txt` — prune the dead py3.6-3.9 marker rows + (numpy/pandas); the package requires >=3.10. +- `requirements/runtime.txt:21` — act on the existing + `# TODO: lets make pandas an optional dependency`. pandas is heavy for a + bash-DAG tool; its main use is the slurm monitor's squeue table, which + Phase 5.1.1 rewrites without pandas — after that lands, demote pandas to + the `optional` extra and guard imports. Same review for `pint` + (`slurm.py:74-100` uses it only to parse "4GB"-style strings — a 10-line + suffix parser removes the dependency and its doctest gating). +- `pyproject.toml` `package-data."*" = ["requirements/*.txt"]` is ineffective + (requirements/ is outside any package; sdist gets them via MANIFEST.in). + Remove the stanza or move requirements inside the package if runtime access + is actually needed (check whether anything reads them at runtime first — + xcookie-style `__requires__` loaders sometimes do). + +## 8.2 CI improvements (`.gitlab-ci.yml` — xcookie-generated; prefer changing +the xcookie config and regenerating; hand-edit only if regeneration is not set +up, and say so in the commit) + +- Lint job: drop `allow_failure: true` once Phase 1.4 lands; run + `ruff check cmd_queue tests`. +- The four ~120-line test job templates differ only in 3 variable lines + (`INSTALL_EXTRAS`, `USE_UV_LOCK`, `LOCK_REQUIREMENTS`); convert to one + template + per-job `variables:` (~400 lines removed, drift risk eliminated). +- Keep (do not regress): wheel-built-then-tested-from-sandbox flow, xdoctest + over the installed module, strict/loose lanes, sdist smoke lane, twine + check, GPG fingerprint verification. + +## 8.3 Sphinx docs + +- **Delete the stale page** `docs/source/auto/cmd_queue.util.util_network_text.rst` + (module no longer exists; also remove its entry from + `docs/source/auto/cmd_queue.util.rst:13`). +- **Regenerate the autodoc tree** — it predates the backends refactor: there + are no pages for `cmd_queue.backends` (serial/tmux/slurm/airflow — the real + implementations), `monitor_manifest`, `slurmify`, `_graph`, `_registry`, + `_rendering`, `_types`, or `util.util_bash`. Use sphinx-apidoc/xcookie's + generator, then build docs and fix warnings. +- If Phase 7.5 deleted `util/texter.py`, remove its docs page in the same pass. +- Build check: `cd docs && make html` with `-W` (warnings as errors) if the + warning count is manageable; otherwise record the count and ratchet down. + +## 8.4 README and CHANGELOG + +- README spot-checks passed in the audit (Quickstart API and CLI subcommands + match the code) — after Phase 3.3.1/2.2.7, re-verify the + `cmd_queue submit ... -- 'a && b'` example actually runs, since it is broken + today and the fix must keep the documented form working. +- Keep CHANGELOG 0.3.2 current as phases land (started in Phase 1.6). Items + worth explicit CHANGELOG entries because they change behavior: name + validation (Phase 2), `onfail` semantics reconciliation (Phase 6.5), pandas/ + pint demotion to optional (8.1), any removed modules (`texter.py`). + +## Verification + +- `pip install -e .[all]` in a fresh 3.10 venv and in the newest supported + python; `python -c "import cmd_queue; cmd_queue.Queue.create(backend='serial')"`. +- `python -m build` (or the CI equivalent) produces sdist+wheel; install the + wheel in a clean venv and run `pytest --pyargs` / the xdoctest lane against + the installed package. +- Docs build clean; the rendered API index lists the backends package. diff --git a/docs/planning/09-code-quality-and-dead-code.md b/docs/planning/09-code-quality-and-dead-code.md new file mode 100644 index 0000000..756baa3 --- /dev/null +++ b/docs/planning/09-code-quality-and-dead-code.md @@ -0,0 +1,107 @@ +# Phase 9: Code quality, dead code, and API consistency + +**Goal:** Remove vestigial code and resolve API inconsistencies. Lowest urgency, +highest reviewer-goodwill. Everything here is behavior-preserving unless marked. + +**Prerequisites:** Phases 2-6 (don't delete code a fix phase is about to touch). + +## 9.1 Dead code to delete + +- `slurm.py:594-632` — `_slurm_checks` never called, returns None. +- `slurm.py:571, 803, 1138` — vestigial `if 0:` / `if 1:` / `if True:` blocks. + Note `:810` unconditionally resets `_include_monitor_metadata = True`, making + the constructor/`_from_manifest` values meaningless — decide to honor the + flag or remove it. +- `tmux.py:990-1004` — `serial_run()`, self-described as deprecated. +- `tmux.py:1690-1785` — `if 0:` `__tmux_notes__` block, ~95 lines of scratch + notes in module body. Move anything worth keeping to `dev/notes/` or docs. +- `tmux.py:760-768` — deprecated `check_other_sessions` handling in `run()`; + it can double-process conflicting sessions since `handle_other_sessions` + already ran unconditionally at `:758`. Complete the deprecation. +- `util_tmux.py:328-397` — `list_panes`/`kill_pane` have no in-repo callers; + delete or test-and-keep deliberately (see Phase 4.11). +- `serial.py:850-878` — ~30 lines of commented-out legacy `print_commands` + after a `return`. +- `base_queue.py:234-237` — dead round-trip + (`kwargs['depends'] = depends` then immediately `pop`). +- `serial.py:585-586, 813-827` — `SerialQueue.__init__` re-initializes + `self.preamble`/`self.jobs` already set by `super().__init__()`, and + re-defines `add_header_command`/`add_preamble_command` identical to the base + class (one needs `# type: ignore` for its trouble). Delete the duplicates. +- `cli_boilerplate.py` — the module is deprecated (kwconf migration, commit + `5642cc7`). Set a removal version (e.g. 0.4.0), say it in the module + docstring's deprecation notice, and keep it working until then (Phase 6.8 + keeps it minimally healthy). + +## 9.2 API consistency + +- **Return annotations that lie:** `TMUXMultiQueue.run` (tmux.py:710-721, + `-> None`) returns `agg_state`/`_dispatch_monitor(...)`; `monitor()` + (tmux.py:1006-1014, `-> None`) returns `agg_state` (`:1096`). Fix + annotations to the real (documented, `_print_done_summary`-shaped) dict — + and add that shape to the Phase 7.3 contract test. +- **Base `Queue.monitor`** (base_queue.py:496-503) prints "monitor not + implemented" and returns, while `run`/`kill`/`finalize_text`/`read_state` + raise `NotImplementedError`. Make it raise for consistency. +- **`SerialQueue.run()` failure signaling** (deferred from Phase 3.4): script + ends `set +e` → exit 0 even when jobs failed; `run()` returns None. Decide: + return final state dict (matching tmux/slurm post-annotation-fix) and/or an + opt-in raise-on-failure. Behavior change → CHANGELOG. +- **`BashJob.__init__` silently discards `gpus`/`cpus`/`mem`** + (serial.py:103-125) — cross-backend acceptance is intentional, but stash + them (e.g. `self.kwargs`) so introspection and `change_backend` don't lose + resource requests. +- **`serial.py:124`** — `assert self.name is not None` vanishes under + `python -O`; raise `ValueError`. +- **`agg_state` totals** (tmux.py:1329-1344) omit workers in `unknown` state, + so the aggregate `total` fluctuates during startup; use + `self.num_real_jobs`. +- **Facade `__all__` re-exports private names**: `serial_queue.py` exports + `_check_bash_text_for_syntax_errors`; `tmux_queue.py` exports four + underscore names. Remove privates from the facades' `__all__` (they invite + external dependence on internals). +- **tmux.py:695** — `handle_other_sessions` imports `has_stdin` from the + `cmd_queue.tmux_queue` facade, which imports it back from this very file + (defined at `:1669`). Use the local name. +- **Error types:** `serial.py:993-1004` raises Python's builtin `SyntaxError` + for a *bash* syntax error and prints stderr instead of attaching it — define + `BashSyntaxError(Exception)` carrying stderr. `base_queue.py:352-360` + swallows all exceptions from `transitive_reduction` and prints `ex=...` — + catch `nx.NetworkXError` specifically and fall back to unreduced rendering + with a clear message. `_graph.py:60-67` — guard edge-only nodes (deps never + submitted) with a "dependency X was never submitted to this queue" error + instead of latent KeyErrors downstream. + +## 9.3 Performance / structure (optional, do last) + +- `tmux.py` — `order_jobs()` runs 2-3× per operation (`write()` → itself, then + `finalize_text()` again; `print_commands` triple-orders), rebuilding all + SerialQueue workers and regenerating bookkeeper jobs with fresh uuid + stat-paths each time (the written scripts and the monitor manifest can + reference *different* bookkeeper paths — currently benign only because + bookkeepers are excluded from failure accounting). Cache the ordering or + make `finalize_text` reuse current workers. This is the riskiest change in + this phase — do it with the Phase 4 tests already green. +- `tmux.py:511-512` — `for m in members: rankings[rank].update(members)` + repeats the identical set-update len(members) times; hoist out of the loop. +- `tmux.py:1489` — the monitor manifest serializes each worker's full + `environ` (the code comments at `:633-634` explicitly worry about logging + secrets in plaintext); the monitor never reads `environ` — drop it from the + manifest. +- `util_tmux.py:99-105, 131-133` — use tmux exact-match targets (`-t =name`) + so prefix matching can't kill/find a different session extending the name + (complements Phase 4.3). +- `tmux.py:74` — class doctest imports `from cmd_queue.serial_queue import *` + which doesn't export `TMUXMultiQueue` (works only via doctest global + seeding); use `tmux_queue`. +- `serial.py:969-990` — `indent`'s doctest lives inside `Returns:` (never + runs); move to an `Example:` block. Consider replacing the local `indent` + with `ub.indent` if equivalent. + +## Verification + +Behavior-preserving claims are backed by the (now-strengthened) suite: +full pytest + doctests + ruff + ty green after every deletion commit. +Grep for stragglers: `grep -rn "if 0:\|if False:" cmd_queue/` returns nothing +unexplained; `python -X dev -W error::DeprecationWarning -c "import cmd_queue"` +raises nothing from our own modules. From bf70fe80d3ad6459f69085f1bc46be0e51249a58 Mon Sep 17 00:00:00 2001 From: Jon Crall Date: Tue, 4 Aug 2026 15:55:38 -0400 Subject: [PATCH 28/31] Apply ruff format across the repo Formatting only, separated from the release fixes so that diff stays reviewable. Co-Authored-By: Claude Opus 5 (1M context) --- cmd_queue/_graph.py | 5 +- cmd_queue/_registry.py | 2 +- cmd_queue/_rendering.py | 1 + cmd_queue/_types.py | 19 ++++++-- cmd_queue/backends/airflow.py | 9 ++-- cmd_queue/backends/serial.py | 6 ++- cmd_queue/base_queue.py | 1 - cmd_queue/main.py | 5 +- cmd_queue/monitor_app.py | 4 +- cmd_queue/monitor_manifest.py | 2 + cmd_queue/slurmify.py | 5 +- cmd_queue/util/util_tmux.py | 4 +- tests/test_backend_contract.py | 2 + tests/test_backend_execution.py | 6 ++- tests/test_bash_variants.py | 68 ++++++++++++++++++--------- tests/test_block_timeout.py | 1 + tests/test_cleanup.py | 1 + tests/test_generated_output_compat.py | 11 ++++- tests/test_package_metadata.py | 4 +- tests/test_public_api_compat.py | 5 +- tests/test_slurm_variants.py | 57 ++++++++++++++-------- tests/test_submit_log_flag.py | 13 +++-- tests/test_tmux_attach.py | 19 ++++---- 23 files changed, 174 insertions(+), 76 deletions(-) diff --git a/cmd_queue/_graph.py b/cmd_queue/_graph.py index 20e4a3d..e4fabb4 100644 --- a/cmd_queue/_graph.py +++ b/cmd_queue/_graph.py @@ -4,6 +4,7 @@ logic that was historically implemented directly on ``Queue`` so later backend refactors can share one dependency model. """ + from __future__ import annotations from typing import Any, Dict, Iterable, List, Optional @@ -36,7 +37,9 @@ def merge_sync_depends(all_depends: Optional[List[Any]], depends: Any) -> Any: return depends -def resolve_dependency_refs(depends: Any, named_jobs: Dict[str, Any]) -> Optional[List[Any]]: +def resolve_dependency_refs( + depends: Any, named_jobs: Dict[str, Any] +) -> Optional[List[Any]]: """Resolve string dependency references against a queue's named jobs.""" coerced = coerce_depends(depends) diff --git a/cmd_queue/_registry.py b/cmd_queue/_registry.py index be3b030..f1ace2b 100644 --- a/cmd_queue/_registry.py +++ b/cmd_queue/_registry.py @@ -4,12 +4,12 @@ This registry only centralizes the backend lookup so future refactors can move backend internals without changing user-facing imports. """ + from __future__ import annotations import importlib from typing import Any, Dict, Tuple, Type - _BACKEND_SPECS: Dict[str, Tuple[str, str]] = { 'serial': ('cmd_queue.backends.serial', 'SerialQueue'), 'tmux': ('cmd_queue.backends.tmux', 'TMUXMultiQueue'), diff --git a/cmd_queue/_rendering.py b/cmd_queue/_rendering.py index e1a3ec5..bfde2ed 100644 --- a/cmd_queue/_rendering.py +++ b/cmd_queue/_rendering.py @@ -1,4 +1,5 @@ """Private rendering helpers shared by queue classes.""" + from __future__ import annotations from typing import Optional diff --git a/cmd_queue/_types.py b/cmd_queue/_types.py index 3a9e317..f6777b9 100644 --- a/cmd_queue/_types.py +++ b/cmd_queue/_types.py @@ -3,17 +3,28 @@ This module is intentionally private. It gives refactors a shared place for small typed data containers without changing the public API. """ + from __future__ import annotations from dataclasses import dataclass from os import PathLike -from typing import Any, Dict, Iterable, Optional, Protocol, TypeAlias, Union, runtime_checkable - +from typing import ( + Any, + Dict, + Iterable, + Optional, + Protocol, + TypeAlias, + Union, + runtime_checkable, +) Pathish: TypeAlias = Union[str, PathLike[str]] JobName: TypeAlias = str -DependencyRef: TypeAlias = Union[JobName, "JobProtocol"] -DependencyRefs: TypeAlias = Optional[Union[DependencyRef, Iterable[DependencyRef]]] +DependencyRef: TypeAlias = Union[JobName, 'JobProtocol'] +DependencyRefs: TypeAlias = Optional[ + Union[DependencyRef, Iterable[DependencyRef]] +] BackendName: TypeAlias = str diff --git a/cmd_queue/backends/airflow.py b/cmd_queue/backends/airflow.py index 0c34390..1daa338 100644 --- a/cmd_queue/backends/airflow.py +++ b/cmd_queue/backends/airflow.py @@ -25,6 +25,7 @@ >>> print((queue.dags_dpath / 'cmdq_airflow_mwe.py').exists()) True """ + from __future__ import annotations import contextlib @@ -64,7 +65,9 @@ def __init__( self.command = command self.name = name self.output_fpath = output_fpath - self.depends: List[base_queue.Job] = base_queue.coerce_job_depends(depends) + self.depends: List[base_queue.Job] = base_queue.coerce_job_depends( + depends + ) self.cpus = cpus self.gpus = gpus self.mem = mem @@ -238,6 +241,7 @@ def run(self, block: bool = True, system: bool = False) -> None: import sys from airflow.models.dag import DagModel + try: # Canonical location since Airflow 3.2 (AIP-66). Importing it # directly avoids the DeprecationWarning emitted by the @@ -271,8 +275,7 @@ def run(self, block: bool = True, system: bool = False) -> None: } params = inspect.signature(DagBag.__init__).parameters accepts_varkw = any( - p.kind == inspect.Parameter.VAR_KEYWORD - for p in params.values() + p.kind == inspect.Parameter.VAR_KEYWORD for p in params.values() ) if not accepts_varkw: dagbag_kwargs = { diff --git a/cmd_queue/backends/serial.py b/cmd_queue/backends/serial.py index f20985f..caa331c 100644 --- a/cmd_queue/backends/serial.py +++ b/cmd_queue/backends/serial.py @@ -3,6 +3,7 @@ https://jmmv.dev/2018/03/shell-readability-strict-mode.html https://stackoverflow.com/questions/13195655/bash-set-x-without-it-being-printed """ + from __future__ import annotations import uuid @@ -127,7 +128,9 @@ def __init__( # The base ``Job`` types ``command`` as ``str | None``; a BashJob always # has a concrete command, so narrow it (keeps ``'\n'.join`` well-typed). self.command: str = command - self.depends: List[base_queue.Job] = base_queue.coerce_job_depends(depends) + self.depends: List[base_queue.Job] = base_queue.coerce_job_depends( + depends + ) self.bookkeeper = bookkeeper self.log = log if info_dpath is None: @@ -1003,4 +1006,5 @@ def _check_bash_text_for_syntax_errors(bash_text: str) -> None: print(info.stderr) raise SyntaxError('bash syntax error') + # the implementation now lives under cmd_queue.backends. diff --git a/cmd_queue/base_queue.py b/cmd_queue/base_queue.py index a3fb196..bf81632 100644 --- a/cmd_queue/base_queue.py +++ b/cmd_queue/base_queue.py @@ -520,7 +520,6 @@ def kill(self, *args: Any, **kwargs: Any) -> Any: def read_state(self, *args: Any, **kwargs: Any) -> Any: raise NotImplementedError - def _coerce_style( self, style: str = 'auto', diff --git a/cmd_queue/main.py b/cmd_queue/main.py index 37e000e..fa3129a 100644 --- a/cmd_queue/main.py +++ b/cmd_queue/main.py @@ -10,6 +10,7 @@ cmd_queue --help """ + from __future__ import annotations from typing import TYPE_CHECKING, Any, Callable, Sequence @@ -580,7 +581,9 @@ class list(CommonConfig): def run(config) -> None: print( - ub.urepr(list(config.cli_queue_dpath().glob('*.cmd_queue.json'))) + ub.urepr( + list(config.cli_queue_dpath().glob('*.cmd_queue.json')) + ) ) diff --git a/cmd_queue/monitor_app.py b/cmd_queue/monitor_app.py index 77b760a..7525160 100644 --- a/cmd_queue/monitor_app.py +++ b/cmd_queue/monitor_app.py @@ -27,7 +27,9 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: pass def run(self, *args: Any, **kwargs: Any) -> None: - raise ImportError('The textual monitor requires the textual package') + raise ImportError( + 'The textual monitor requires the textual package' + ) MonitorTableFn = Callable[[], Tuple[Any, bool, Any]] diff --git a/cmd_queue/monitor_manifest.py b/cmd_queue/monitor_manifest.py index 56fd02d..8617337 100644 --- a/cmd_queue/monitor_manifest.py +++ b/cmd_queue/monitor_manifest.py @@ -16,7 +16,9 @@ a human queue name to the most recent manifest path so that ``cmd_queue monitor `` can find it. """ + from __future__ import annotations + import json from typing import Any, Dict, Optional diff --git a/cmd_queue/slurmify.py b/cmd_queue/slurmify.py index 4dd4bc9..8c1edc5 100644 --- a/cmd_queue/slurmify.py +++ b/cmd_queue/slurmify.py @@ -16,6 +16,7 @@ -- \ python -c 'import sys; print("hello world"); sys.exit(0)' """ + from typing import Any, Sequence import kwconf as kw @@ -25,9 +26,7 @@ class SlurmifyCLI(kw.Config): __command__ = 'slurmify' - jobname = kw.Value( - None, help='for submit, this is the name of the new job' - ) + jobname = kw.Value(None, help='for submit, this is the name of the new job') depends = kw.Value( None, parser='csv', help='comma separated jobnames to depend on' ) diff --git a/cmd_queue/util/util_tmux.py b/cmd_queue/util/util_tmux.py index 6a8f0a6..5925a37 100644 --- a/cmd_queue/util/util_tmux.py +++ b/cmd_queue/util/util_tmux.py @@ -33,7 +33,9 @@ def resolve_block_timeout(explicit: Any = None) -> float | None: import os if explicit is not None and explicit != 'auto': - return None if float(explicit) in (0.0, float('inf')) else float(explicit) + return ( + None if float(explicit) in (0.0, float('inf')) else float(explicit) + ) env_val = os.environ.get('CMD_QUEUE_BLOCK_TIMEOUT', '').strip() if env_val: diff --git a/tests/test_backend_contract.py b/tests/test_backend_contract.py index e8c5d90..eacbd42 100644 --- a/tests/test_backend_contract.py +++ b/tests/test_backend_contract.py @@ -36,7 +36,9 @@ def test_backend_classes_support_minimal_contract(backend, tmp_path): # per-worker scripts contain the actual job commands. ``workers`` is a # tmux-only attribute, so narrow the base ``Queue`` to the concrete type. from typing import cast + from cmd_queue.backends.tmux import TMUXMultiQueue + tmux_queue = cast(TMUXMultiQueue, queue) combined_text += '\n'.join( worker.finalize_text() for worker in tmux_queue.workers diff --git a/tests/test_backend_execution.py b/tests/test_backend_execution.py index 584add9..cd9d953 100644 --- a/tests/test_backend_execution.py +++ b/tests/test_backend_execution.py @@ -20,6 +20,7 @@ read them back. ``$HOME`` is shared across nodes on a typical cluster, whereas ``/tmp`` is often node-local. """ + from __future__ import annotations import pytest @@ -185,6 +186,7 @@ def test_slurm_monitor_none_blocks(): fix where slurm's ``none`` printed "detached" and returned. """ import time + if 'slurm' not in _AVAILABLE: pytest.skip('slurm backend is not available on this machine') dpath = _work_dpath('slurm-none-blocks') @@ -199,4 +201,6 @@ def test_slurm_monitor_none_blocks(): assert marker.exists(), ( 'run() returned before the job finished -- monitor=none did not block' ) - assert dt >= 2.5, f'run() returned too fast ({dt:.1f}s); monitor=none did not block' + assert dt >= 2.5, ( + f'run() returned too fast ({dt:.1f}s); monitor=none did not block' + ) diff --git a/tests/test_bash_variants.py b/tests/test_bash_variants.py index fbc97cc..e8102c7 100644 --- a/tests/test_bash_variants.py +++ b/tests/test_bash_variants.py @@ -548,8 +548,12 @@ def test_bashjob_exec_teardown_runs_on_success(): text = job.finalize_text(with_status=True, with_gaurds=True) subprocess.run(['bash', '-n'], input=text, text=True, check=True) subprocess.run( - ['bash'], input=text, text=True, cwd=str(tmp_path), - capture_output=True, check=False, + ['bash'], + input=text, + text=True, + cwd=str(tmp_path), + capture_output=True, + check=False, ) assert td_marker.exists(), 'teardown should run on success' @@ -574,8 +578,12 @@ def test_bashjob_exec_teardown_runs_on_command_failure(): text = job.finalize_text(with_status=True, with_gaurds=True) subprocess.run(['bash', '-n'], input=text, text=True, check=True) subprocess.run( - ['bash'], input=text, text=True, cwd=str(tmp_path), - capture_output=True, check=False, + ['bash'], + input=text, + text=True, + cwd=str(tmp_path), + capture_output=True, + check=False, ) assert td_marker.exists(), 'teardown should run even when command fails' @@ -601,8 +609,12 @@ def test_bashjob_exec_teardown_skipped_when_setup_fails(): text = job.finalize_text(with_status=True, with_gaurds=True) subprocess.run(['bash', '-n'], input=text, text=True, check=True) subprocess.run( - ['bash'], input=text, text=True, cwd=str(tmp_path), - capture_output=True, check=False, + ['bash'], + input=text, + text=True, + cwd=str(tmp_path), + capture_output=True, + check=False, ) assert not outfile.exists(), 'command should not run if setup fails' @@ -627,8 +639,12 @@ def test_bashjob_exec_teardown_failure_does_not_flip_result(): text = job.finalize_text(with_status=True, with_gaurds=True) subprocess.run(['bash', '-n'], input=text, text=True, check=True) subprocess.run( - ['bash'], input=text, text=True, cwd=str(tmp_path), - capture_output=True, check=False, + ['bash'], + input=text, + text=True, + cwd=str(tmp_path), + capture_output=True, + check=False, ) assert td_marker.exists(), 'teardown should run' @@ -645,8 +661,9 @@ def test_bashjob_teardown_trap_does_not_leak_across_jobs(): tmp_path = ub.Path(tmp_path) td_marker = tmp_path / 'teardown_ran.txt' - j1 = BashJob('echo J1', name='j1', - teardown=f'printf x >> "{td_marker}"') + j1 = BashJob( + 'echo J1', name='j1', teardown=f'printf x >> "{td_marker}"' + ) j1.log = False j1.stat_fpath = tmp_path / 'j1.status.json' j1.pass_fpath = tmp_path / 'j1.pass' @@ -658,14 +675,20 @@ def test_bashjob_teardown_trap_does_not_leak_across_jobs(): j2.pass_fpath = tmp_path / 'j2.pass' j2.fail_fpath = tmp_path / 'j2.fail' - text = '\n'.join([ - j1.finalize_text(with_status=True, with_gaurds=True), - j2.finalize_text(with_status=True, with_gaurds=True), - ]) + text = '\n'.join( + [ + j1.finalize_text(with_status=True, with_gaurds=True), + j2.finalize_text(with_status=True, with_gaurds=True), + ] + ) subprocess.run(['bash', '-n'], input=text, text=True, check=True) subprocess.run( - ['bash'], input=text, text=True, cwd=str(tmp_path), - capture_output=True, check=False, + ['bash'], + input=text, + text=True, + cwd=str(tmp_path), + capture_output=True, + check=False, ) # Exactly one teardown invocation -- the trap did not leak to j2. @@ -682,6 +705,7 @@ def test_bashjob_exec_teardown_runs_on_sigterm(): import os import signal import time + with tempfile.TemporaryDirectory() as tmp_path: tmp_path = ub.Path(tmp_path) td_marker = tmp_path / 'teardown_ran.txt' @@ -740,8 +764,12 @@ def test_bashjob_exec_cwd_restores_worker_dir(): probe = tmp_path / 'pwd_after.txt' script = text + f'\npwd -P > "{probe}"\n' subprocess.run( - ['bash'], input=script, text=True, cwd=str(tmp_path), - capture_output=True, check=False, + ['bash'], + input=script, + text=True, + cwd=str(tmp_path), + capture_output=True, + check=False, ) assert job.pass_fpath.exists() assert probe.read_text().strip() == str(tmp_path.resolve()), ( @@ -755,9 +783,7 @@ def test_bashjob_teardown_traps_hup(): run-EXIT-trap-on-unhandled-group-HUP behavior.""" with tempfile.TemporaryDirectory() as tmp_path: tmp_path = ub.Path(tmp_path) - job, _ = _make_teardown_job( - tmp_path, 'true', teardown='echo done' - ) + job, _ = _make_teardown_job(tmp_path, 'true', teardown='echo done') text = job.finalize_text(with_status=True, with_gaurds=True) assert "trap 'exit 129' HUP" in text subprocess.run(['bash', '-n'], input=text, text=True, check=True) diff --git a/tests/test_block_timeout.py b/tests/test_block_timeout.py index 055806c..670fb0b 100644 --- a/tests/test_block_timeout.py +++ b/tests/test_block_timeout.py @@ -13,6 +13,7 @@ already set in the environment; the "production" cases delete it via ``monkeypatch`` (which restores it after each test). """ + from __future__ import annotations import time diff --git a/tests/test_cleanup.py b/tests/test_cleanup.py index a4d6625..6a35848 100644 --- a/tests/test_cleanup.py +++ b/tests/test_cleanup.py @@ -1,4 +1,5 @@ """Tests for cmd_queue cleanup helpers.""" + from __future__ import annotations diff --git a/tests/test_generated_output_compat.py b/tests/test_generated_output_compat.py index a58a67e..789cfaa 100644 --- a/tests/test_generated_output_compat.py +++ b/tests/test_generated_output_compat.py @@ -24,16 +24,23 @@ def test_serial_generated_text_invariants(tmp_path): def test_tmux_generated_text_invariants(tmp_path): from typing import cast + from cmd_queue import Queue from cmd_queue.backends.tmux import TMUXMultiQueue queue = Queue.create( - backend='tmux', name='compat_tmux', rootid='root', dpath=tmp_path, size=1 + backend='tmux', + name='compat_tmux', + rootid='root', + dpath=tmp_path, + size=1, ) first = queue.submit('echo first', name='first') queue.submit('echo second', name='second', depends=first) - text = queue.finalize_text(with_status=False, with_gaurds=False, with_locks=False) + text = queue.finalize_text( + with_status=False, with_gaurds=False, with_locks=False + ) # ``workers`` is a tmux-only attribute; narrow to the concrete queue type. worker_text = '\n'.join( worker.finalize_text(with_status=False, with_gaurds=False) diff --git a/tests/test_package_metadata.py b/tests/test_package_metadata.py index c8bb929..0d999bc 100644 --- a/tests/test_package_metadata.py +++ b/tests/test_package_metadata.py @@ -7,6 +7,8 @@ import pytest -@pytest.mark.skip(reason='metadata checks belong in build/install CI smoke tests') +@pytest.mark.skip( + reason='metadata checks belong in build/install CI smoke tests' +) def test_package_metadata_checked_by_ci_smoke_test(): pass diff --git a/tests/test_public_api_compat.py b/tests/test_public_api_compat.py index 02778e8..7ddea95 100644 --- a/tests/test_public_api_compat.py +++ b/tests/test_public_api_compat.py @@ -45,7 +45,10 @@ def test_create_preserves_size_argument_compatibility(tmp_path): from cmd_queue.tmux_queue import TMUXMultiQueue serial = Queue.create( - backend='serial', size=999, name='compat_serial', dpath=tmp_path / 'serial' + backend='serial', + size=999, + name='compat_serial', + dpath=tmp_path / 'serial', ) tmux = Queue.create( backend='tmux', size=1, name='compat_tmux', dpath=tmp_path / 'tmux' diff --git a/tests/test_slurm_variants.py b/tests/test_slurm_variants.py index a30f65c..11ae5d8 100644 --- a/tests/test_slurm_variants.py +++ b/tests/test_slurm_variants.py @@ -114,7 +114,9 @@ def test_slurm_setup_gates_teardown(): # ``setup && { trap...; command; }``: when setup fails the whole group # (which installs the trap) never runs, so nothing is torn down. queue = SlurmQueue(preamble=None) - job = queue.submit('echo CMD', setup='acquire_lease', teardown='release_lease') + job = queue.submit( + 'echo CMD', setup='acquire_lease', teardown='release_lease' + ) sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) payload = _extract_wrap_payload(sbatch_args) @@ -127,31 +129,36 @@ def test_slurm_teardown_executes_as_documented(): # Execute the rendered wrap payload to confirm the runtime contract: # the command's exit code stays authoritative and teardown always runs. import subprocess + queue = SlurmQueue(preamble=None) # command fails -> exit code preserved, teardown still runs - job = queue.submit('echo CMD; exit 5', name='a', - setup='echo SETUP', teardown='echo TD') + job = queue.submit( + 'echo CMD; exit 5', name='a', setup='echo SETUP', teardown='echo TD' + ) payload = _extract_wrap_payload( - job._build_sbatch_args(global_preamble=queue.header_commands)) + job._build_sbatch_args(global_preamble=queue.header_commands) + ) r = subprocess.run(['bash', '-c', payload], capture_output=True, text=True) assert r.returncode == 5, 'command exit code stays authoritative' assert 'TD' in r.stdout, 'teardown runs even when command fails' # teardown fails -> does not flip a passing command - job = queue.submit('echo CMD', name='b', - setup='echo SETUP', teardown='echo TD; false') + job = queue.submit( + 'echo CMD', name='b', setup='echo SETUP', teardown='echo TD; false' + ) payload = _extract_wrap_payload( - job._build_sbatch_args(global_preamble=queue.header_commands)) + job._build_sbatch_args(global_preamble=queue.header_commands) + ) r = subprocess.run(['bash', '-c', payload], capture_output=True, text=True) assert r.returncode == 0, 'teardown failure must not flip the result' assert 'TD' in r.stdout # setup fails -> command skipped, teardown not run - job = queue.submit('echo CMD', name='c', - setup='false', teardown='echo TD') + job = queue.submit('echo CMD', name='c', setup='false', teardown='echo TD') payload = _extract_wrap_payload( - job._build_sbatch_args(global_preamble=queue.header_commands)) + job._build_sbatch_args(global_preamble=queue.header_commands) + ) r = subprocess.run(['bash', '-c', payload], capture_output=True, text=True) assert r.returncode != 0, 'setup failure fails the job' assert 'CMD' not in r.stdout, 'command should not run if setup fails' @@ -165,12 +172,14 @@ def test_parse_scontrol_output_tolerates_bare_tokens(): """ from cmd_queue.backends.slurm import parse_scontrol_output - sample = '\n'.join([ - 'JobId=123 JobState=COMPLETED ExitCode=0:0', - 'JobName=smol_135_01_abc', - 'Reason=Memory Required Not Available BareToken', # space value, unknown key - 'TRES=cpu=4,mem=16G,gres/gpu=2', - ]) + sample = '\n'.join( + [ + 'JobId=123 JobState=COMPLETED ExitCode=0:0', + 'JobName=smol_135_01_abc', + 'Reason=Memory Required Not Available BareToken', # space value, unknown key + 'TRES=cpu=4,mem=16G,gres/gpu=2', + ] + ) info = parse_scontrol_output(sample) assert info['JobState'] == 'COMPLETED' assert info['ExitCode'] == '0:0' @@ -180,14 +189,21 @@ def test_parse_scontrol_output_tolerates_bare_tokens(): def test_sacct_job_state_is_best_effort(): """_sacct_job_state never raises; returns '' for an unknown/bogus job id.""" from cmd_queue.backends.slurm import _sacct_job_state + assert _sacct_job_state('not-a-real-jobid-zzz') == '' def test_parse_scontrol_missing_jobstate(): """A purged/invalid job yields no JobState -> the monitor must use .get().""" from cmd_queue.backends.slurm import parse_scontrol_output + assert parse_scontrol_output('').get('JobState') is None - assert parse_scontrol_output('slurm_load_jobs error: Invalid job id specified').get('JobState') is None + assert ( + parse_scontrol_output( + 'slurm_load_jobs error: Invalid job id specified' + ).get('JobState') + is None + ) def test_sbatch_boolean_flags_render_cleanly(): @@ -199,6 +215,9 @@ def test_sbatch_boolean_flags_render_cleanly(): sbatch_args = job._build_sbatch_args(global_preamble=queue.header_commands) assert '--hold' in sbatch_args assert '--requeue' in sbatch_args - assert not any('"' in a and not a.startswith(('--wrap', '--job-name', '--output')) - for a in sbatch_args if a in ('--hold"', '--requeue"')), sbatch_args + assert not any( + '"' in a and not a.startswith(('--wrap', '--job-name', '--output')) + for a in sbatch_args + if a in ('--hold"', '--requeue"') + ), sbatch_args assert '--hold"' not in sbatch_args and '--requeue"' not in sbatch_args diff --git a/tests/test_submit_log_flag.py b/tests/test_submit_log_flag.py index 1658824..9366bdd 100644 --- a/tests/test_submit_log_flag.py +++ b/tests/test_submit_log_flag.py @@ -9,6 +9,7 @@ silently disable tee logging without any other test catching it, which is exactly the kind of thing this test is here to catch. """ + import cmd_queue @@ -26,7 +27,9 @@ def _command_section(text: str) -> str: def test_submit_with_log_true_produces_tee(): - queue = cmd_queue.Queue.create(backend='serial', name='log-flag-true', size=1) + queue = cmd_queue.Queue.create( + backend='serial', name='log-flag-true', size=1 + ) job = queue.submit('echo hi', name='job1', log=True) assert job.log is True, 'log=True should land on BashJob.log' @@ -45,7 +48,9 @@ def test_submit_with_log_true_produces_tee(): def test_submit_with_log_false_omits_tee(): - queue = cmd_queue.Queue.create(backend='serial', name='log-flag-false', size=1) + queue = cmd_queue.Queue.create( + backend='serial', name='log-flag-false', size=1 + ) job = queue.submit('echo hi', name='job1', log=False) assert job.log is False, 'log=False should land on BashJob.log' @@ -64,7 +69,9 @@ def test_submit_log_default_omits_tee(): compatibility. If a caller does not pass ``log``, no tee should appear. Tracked here so any default flip is caught explicitly. """ - queue = cmd_queue.Queue.create(backend='serial', name='log-flag-default', size=1) + queue = cmd_queue.Queue.create( + backend='serial', name='log-flag-default', size=1 + ) job = queue.submit('echo hi', name='job1') assert job.log is False, 'BashJob.log default is False' diff --git a/tests/test_tmux_attach.py b/tests/test_tmux_attach.py index 21f520d..20644ba 100644 --- a/tests/test_tmux_attach.py +++ b/tests/test_tmux_attach.py @@ -15,6 +15,7 @@ The tmux helpers are monkeypatched so the tests run without a tmux server. """ + from __future__ import annotations from typing import Any, Dict, List @@ -22,7 +23,9 @@ import pytest -def _patch_tmux_helpers(monkeypatch: pytest.MonkeyPatch) -> Dict[str, List[Any]]: +def _patch_tmux_helpers( + monkeypatch: pytest.MonkeyPatch, +) -> Dict[str, List[Any]]: """Replace the tmux helper static methods with recorders. Returns a dict of call-log lists keyed by helper name so each test @@ -69,12 +72,8 @@ def fake_attach(session_name: str) -> None: monkeypatch.setattr( util_tmux.tmux, 'spawn_monitor_session', staticmethod(fake_spawn) ) - monkeypatch.setattr( - util_tmux.tmux, 'kill_session', staticmethod(fake_kill) - ) - monkeypatch.setattr( - util_tmux.tmux, 'has_session', staticmethod(fake_has) - ) + monkeypatch.setattr(util_tmux.tmux, 'kill_session', staticmethod(fake_kill)) + monkeypatch.setattr(util_tmux.tmux, 'has_session', staticmethod(fake_has)) monkeypatch.setattr( util_tmux.tmux, 'attach_or_switch', staticmethod(fake_attach) ) @@ -159,7 +158,7 @@ def fake_monitor(self, **kwargs): with_textual='auto', ) - assert calls['spawn'] == [], "inline mode must not spawn a side session" + assert calls['spawn'] == [], 'inline mode must not spawn a side session' assert calls['kill'] == [], 'no kill if nothing was spawned' assert 'side_session' not in seen[0], ( 'inline path goes through the legacy monitor() signature, ' @@ -226,9 +225,7 @@ def test_textual_app_binds_a_only_when_attach_session_set(): def table_fn(): return None, True, {} - app_with = CmdQueueMonitorApp( - table_fn, attach_session='cmdq-monitor-x' - ) + app_with = CmdQueueMonitorApp(table_fn, attach_session='cmdq-monitor-x') app_without = CmdQueueMonitorApp(table_fn) assert app_with.attach_session == 'cmdq-monitor-x' From 6e02f52feed5c20a5f5d15b6ad66b4558257be08 Mon Sep 17 00:00:00 2001 From: Jon Crall Date: Tue, 4 Aug 2026 15:55:50 -0400 Subject: [PATCH 29/31] Close the 0.3.2 release gaps: parse, deprecation, changelog Three things that would have shipped broken, plus the ty errors. parse was imported by TMUXMultiQueue.kill_other_queues and declared in no requirements file, so the tmux backend raised ModuleNotFoundError on any install that did not happen to have it -- 8 tests and doctests failed on a clean sync. Rather than declare a dependency for one call site, drop it: the template it parsed was matching session ids, and a prefix test does that correctly. The parse version was also wrong in a way that mattered, since non-greedy fields made a queue named my_queue parse as my -- it missed its own sessions, and a queue actually named my matched my_queue's and offered to kill them. CMDQueueConfig was documented as emitting a DeprecationWarning and never emitted one. It does now, on subclassing rather than on import, because importing cli_boilerplate is also how a caller reaches the kwconf class. The 0.3.2 changelog section was empty. It now covers all ten commits, including the two call-signature changes downstream CLIs will hit: kwconf's cli() takes argv= rather than cmdline=, and a bool rather than an int. Also cast ub.cmd's stdout in _sacct_job_state, which returns str unless binary=True but is annotated str | bytes, and drop four unused DAG-leaf assignments in test_errors. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 76 ++++++++++++++++++++++++++++++++++++ cmd_queue/backends/slurm.py | 33 ++++++++++++---- cmd_queue/backends/tmux.py | 44 +++++++++++++-------- cmd_queue/cli_boilerplate.py | 19 +++++++++ tests/test_errors.py | 8 ++-- 5 files changed, 153 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b013e45..59dfa2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,82 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## Version 0.3.2 - Unreleased +### Added: +* `CmdQueueConfigMixin`, a :mod:`kwconf`-based equivalent of `CMDQueueConfig` + carrying the same fields and the same `create_queue` / `run_queue` API. Use + it for new code. +* A headless mode for the slurm backend's `monitor()`: it polls until every job + is terminal without rendering a live table, and still prints per-job + pass/fail/skip lines. +* Nine ordered planning documents under `docs/planning/`, from a full-repo + audit: hygiene, shell-quoting hardening, core/tmux/slurm/airflow + correctness, the test suite, packaging/CI/docs, and dead-code cleanup. + +### Changed: +* The internal CLIs (`main.py`, `slurmify.py`) are now kwconf-based. kwconf does + not auto-split comma strings, so `--depends` and `--gpus` use `parser='csv'` + to preserve the documented comma-separated behavior. + + **For downstream CLIs:** kwconf's `cli()` takes `argv=` rather than + `cmdline=`, and its sys.argv toggle is a `bool` rather than scriptconfig's + `int` -- so `main(argv=1)` becomes `main(argv=True)`. +* `Job.depends` is typed `List[Job]` rather than `Optional[Iterable[Job]]`, + which is what constructors actually receive: string references are resolved + at the queue level before construction. A new `base_queue.JobDepends` alias + and `coerce_job_depends()` helper normalize it, and `Queue._register_named_job()` + narrows optional job names. This replaced blanket `type: ignore` comments with + structural fixes. +* `TMUXMultiQueue.kill_other_queues` no longer requires the `parse` package, + which was imported but declared in no requirements file -- so the tmux + backend raised `ModuleNotFoundError` on any install that did not happen to + have it. + +### Deprecated: +* `CMDQueueConfig`, the scriptconfig-based boilerplate base. Subclassing it now + emits a `DeprecationWarning` naming `CmdQueueConfigMixin` as the replacement. + (The 0.3.2 migration documented this warning but never raised one.) The warning fires on + subclassing rather than on import, because importing `cli_boilerplate` is + also how a caller reaches the kwconf class. `scriptconfig` remains a + dependency for as long as this class exists. + +### Fixed: +* **serial/tmux: every job after a `cwd` job ran in the wrong directory.** The + cwd restore was rendered as `[["$CHDIR_OK" == "1"]]` -- no space after `[[`, + which bash resolves as an unknown command -- so `popd` never ran. +* **serial/tmux: a teardown could be skipped on `tmux kill-session`.** The + teardown subshell now traps `HUP` explicitly rather than relying on bash's + undocumented run-the-EXIT-trap-on-unhandled-group-HUP behavior, which the + release-on-kill path was riding on. +* **slurm: boolean `sbatch` flags rendered with a stray trailing quote** + (`--hold"`), malforming the sbatch line for any boolean in + `SLURM_SBATCH_FLAGS`. +* **slurm: the monitor crashed on purged or unknown jobs.** `scontrol show job` + returns no `JobState` once a job is past `MinJobAge`, so `info['JobState']` + raised `KeyError` when re-attaching after a run. The final state is now + recovered from accounting via `sacct`, all fields are read with `.get()`, and + `TIMEOUT` / `OUT_OF_MEMORY` / `NODE_FAIL` and friends are treated as + terminal-failed rather than `'unknown'`, which used to hang the completion + check. +* **slurm: `parse_scontrol_output` raised on some slurm versions.** It assumed + every whitespace token was `key=value`; a space-containing value for a + non-special key yielded a bare token and "not enough values to unpack". Bare + tokens are now skipped. +* **slurm: `run(block=True, monitor='none')` did not block.** It printed "Queue + running detached" and returned immediately, so a scripted run could act on + results before any job finished -- contradicting both the tmux backend and + this backend's own docstring. `block=False` is now the genuinely + non-blocking case, and carries the reattach hint. +* **`--run=0` executed the queue.** `run` was declared + `run: bool = kw.Flag(False, ...)`, and the `: bool` annotation made kwconf + coerce with `bool()`, so `'0'` became `True`. Dropping the annotation + restores `--run=0` falsy, `--run=1` truthy, bare `--run` true. +* `monitor='none'` was smartcast to `None` by scriptconfig in `CMDQueueConfig`. +* **`kill_other_queues` could offer to kill an unrelated queue's sessions.** It + matched session ids with a non-greedy `{name}_{rootid}` template, so a queue + named `my_queue` parsed as `my`: it missed its own sessions, and a queue + actually named `my` matched `my_queue`'s. Session ownership is now decided by + the full `_` prefix. + ## Version 0.3.1 - Released 2026-06-25 diff --git a/cmd_queue/backends/slurm.py b/cmd_queue/backends/slurm.py index 742c506..bbef26f 100644 --- a/cmd_queue/backends/slurm.py +++ b/cmd_queue/backends/slurm.py @@ -37,9 +37,10 @@ >>> else: >>> print('output does not exist') """ + from __future__ import annotations -from typing import Any, Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Union, cast import ubelt as ub @@ -291,7 +292,9 @@ def __init__( # concrete ``str`` here (the base ``Job`` types it as ``str | None``). self.name: str = name self.output_fpath = output_fpath - self.depends: List[base_queue.Job] = base_queue.coerce_job_depends(depends) + self.depends: List[base_queue.Job] = base_queue.coerce_job_depends( + depends + ) self.cpus = cpus self.gpus = gpus self.mem = mem @@ -1075,12 +1078,25 @@ def update_jobid_status(): # neither hangs waiting nor mislabels them. ``str.startswith`` takes a # tuple of prefixes. TERMINAL_FAIL = ( - 'FAILED', 'TIMEOUT', 'NODE_FAIL', 'OUT_OF_MEMORY', - 'BOOT_FAIL', 'DEADLINE', 'PREEMPTED', 'SPECIAL_EXIT', + 'FAILED', + 'TIMEOUT', + 'NODE_FAIL', + 'OUT_OF_MEMORY', + 'BOOT_FAIL', + 'DEADLINE', + 'PREEMPTED', + 'SPECIAL_EXIT', ) TRANSIENT = ( - 'PENDING', 'CONFIGURING', 'COMPLETING', 'SUSPENDED', - 'RESIZING', 'REQUEUED', 'SIGNALING', 'STAGE_OUT', 'STOPPED', + 'PENDING', + 'CONFIGURING', + 'COMPLETING', + 'SUSPENDED', + 'RESIZING', + 'REQUEUED', + 'SIGNALING', + 'STAGE_OUT', + 'STOPPED', ) for row in job_status_table: if row['needs_update']: @@ -1506,7 +1522,10 @@ def _sacct_job_state(job_id: Any) -> str: return '' if getattr(out, 'returncode', 1) != 0: return '' - for line in (out.stdout or '').splitlines(): + # ``ub.cmd`` returns text unless ``binary=True``; its annotation is the + # wider ``str | bytes``. + stdout = cast(str, out.stdout or '') + for line in stdout.splitlines(): line = line.strip() if line: # First (primary) record; drop any trailing reason like diff --git a/cmd_queue/backends/tmux.py b/cmd_queue/backends/tmux.py index f756a24..ce5ae49 100644 --- a/cmd_queue/backends/tmux.py +++ b/cmd_queue/backends/tmux.py @@ -48,7 +48,9 @@ >>> queue.run() """ + from __future__ import annotations + import os import uuid from typing import Any, Dict, Iterable, List, Optional @@ -58,8 +60,7 @@ # import itertools as it from cmd_queue import base_queue from cmd_queue.backends.serial import SerialQueue -from cmd_queue.util.util_tmux import tmux -from cmd_queue.util.util_tmux import block_deadline +from cmd_queue.util.util_tmux import block_deadline, tmux class TMUXMultiQueue(base_queue.Queue): @@ -659,18 +660,20 @@ def kill_other_queues(self, ask_first: bool = True) -> None: Find other tmux sessions that look like they were started with cmd_queue and kill them. """ - import parse - - queue_name_pattern = parse.Parser( - self._tmux_session_prefix + '{name}_{rootid}' - ) + # A session of this queue is named ``__`` + # (see the worker construction in ``_init_workers``), so the prefix is + # the whole test. This used to parse the id with a ``{name}_{rootid}`` + # template, whose fields are non-greedy: a queue named ``my_queue`` + # parsed as ``my``, so it missed its own sessions -- and a queue + # actually named ``my`` matched ``my_queue``'s and offered to kill + # them. That is a bad way to be wrong in a function that kills things. + session_prefix = f'{self._tmux_session_prefix}{self.name}_' current_sessions = self._tmux_current_sessions() - other_session_ids = [] - for info in current_sessions: - matched = queue_name_pattern.parse(info['id']) - if matched is not None: - if self.name == matched['name']: - other_session_ids.append(info['id']) + other_session_ids = [ + info['id'] + for info in current_sessions + if info['id'].startswith(session_prefix) + ] # print(f'other_session_ids={other_session_ids}') if other_session_ids: print( @@ -1084,9 +1087,15 @@ def monitor( with_textual = False if with_textual: - self._textual_monitor(side_session=side_session, manifest_path=manifest_path) + self._textual_monitor( + side_session=side_session, manifest_path=manifest_path + ) else: - self._simple_rich_monitor(refresh_rate, side_session=side_session, manifest_path=manifest_path) + self._simple_rich_monitor( + refresh_rate, + side_session=side_session, + manifest_path=manifest_path, + ) table, finished, agg_state = self._build_status_table() if onexit == 'capture': self.capture() @@ -1109,7 +1118,10 @@ def _textual_monitor( is_running = True while is_running: - table_fn = lambda: self._build_live_renderable(side_session=side_session) + + def table_fn(): + return self._build_live_renderable(side_session=side_session) + app = CmdQueueMonitorApp( table_fn, kill_fn=self.kill, attach_session=side_session ) diff --git a/cmd_queue/cli_boilerplate.py b/cmd_queue/cli_boilerplate.py index 88fa699..10f949f 100644 --- a/cmd_queue/cli_boilerplate.py +++ b/cmd_queue/cli_boilerplate.py @@ -106,9 +106,11 @@ >>> print('----------------') >>> my_cli_main(cmdline=0, run=1, print_queue=0, print_commands=0) """ + from __future__ import annotations import typing +import warnings from typing import Any, Dict, Optional import kwconf as kw @@ -121,6 +123,8 @@ class CMDQueueConfig(scfg.DataConfig): """ + DEPRECATED: use :class:`CmdQueueConfigMixin`, which is kwconf-based. + A helper to carry around the common boilerplate for cmd-queue CLI's. The general usage is that you will inherit from this class and define config options your CLI cares about, however they must not overload any of the @@ -166,6 +170,21 @@ class CMDQueueConfig(scfg.DataConfig): """ + def __init_subclass__(cls, **kwargs: Any) -> None: + # Warn on subclassing rather than at import: importing this module is + # also how a caller reaches ``CmdQueueConfigMixin``, and warning there + # would fire for people who never touch the scriptconfig class. + super().__init_subclass__(**kwargs) + warnings.warn( + f'{cls.__name__} inherits from cmd_queue CMDQueueConfig, which is ' + 'deprecated and will be removed in a future release. Inherit from ' + 'cmd_queue.cli_boilerplate.CmdQueueConfigMixin instead; it is the ' + 'kwconf equivalent. Note that kwconf takes cli(argv=...) rather ' + 'than cli(cmdline=...), and a bool rather than an int.', + DeprecationWarning, + stacklevel=2, + ) + run = scfg.Value( False, isflag=True, diff --git a/tests/test_errors.py b/tests/test_errors.py index cb2d1c3..9fdfea4 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -7,12 +7,12 @@ def test_failures_on_each_backend(): self = cmd_queue.Queue.create(backend=backend) job1 = self.submit('echo "job1 fails" && false') job2 = self.submit('echo "job2 never runs"', depends=[job1]) - job3 = self.submit('echo "job3 never runs"', depends=[job2]) + self.submit('echo "job3 never runs"', depends=[job2]) job4 = self.submit('echo "job4 passes" && true') job5 = self.submit('echo "job5 fails" && false', depends=[job4]) - job6 = self.submit('echo "job6 never runs"', depends=[job5]) - job7 = self.submit('echo "job7 never runs"', depends=[job4, job2]) - job8 = self.submit('echo "job8 never runs"', depends=[job4, job1]) + self.submit('echo "job6 never runs"', depends=[job5]) + self.submit('echo "job7 never runs"', depends=[job4, job2]) + self.submit('echo "job8 never runs"', depends=[job4, job1]) self.print_commands() self.run() self.read_state() From c73365febf0532281ea14eebba877d08ea5c76fc Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Tue, 4 Aug 2026 16:32:01 -0400 Subject: [PATCH 30/31] [skip ci] Start branch for 0.3.3 --- CHANGELOG.md | 5 ++++- cmd_queue/__init__.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59dfa2d..735ca20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,10 @@ We are currently working on porting this changelog to the specifications in This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## Version 0.3.2 - Unreleased +## Version 0.3.3 - Unreleased + + +## Version 0.3.2 - Released 2026-08-04 ### Added: * `CmdQueueConfigMixin`, a :mod:`kwconf`-based equivalent of `CMDQueueConfig` diff --git a/cmd_queue/__init__.py b/cmd_queue/__init__.py index c3b7912..8579eb9 100644 --- a/cmd_queue/__init__.py +++ b/cmd_queue/__init__.py @@ -306,7 +306,7 @@ __mkinit__ = """ mkinit -m cmd_queue """ -__version__ = '0.3.2' +__version__ = '0.3.3' __submodules__ = { From b1a85e809d98f68073c5fd85da00801f573ee609 Mon Sep 17 00:00:00 2001 From: "jon.crall" Date: Tue, 4 Aug 2026 16:53:14 -0400 Subject: [PATCH 31/31] Update xcookie --- .gitlab-ci.yml | 25 +++++++++++++++++++++++++ docs/source/conf.py | 1 + pyproject.toml | 2 +- 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e1efaeb..2863d0f 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -515,6 +515,14 @@ test/minimal-loose/cp314-linux-x86_64: image: python:3.14 needs: - build/cp314-linux-x86_64 +build/cp315-linux-x86_64: + <<: *build_wheel_template + image: python:3.15-rc +test/minimal-loose/cp315-linux-x86_64: + <<: *test_minimal-loose_template + image: python:3.15-rc + needs: + - build/cp315-linux-x86_64 test/full-loose/cp310-linux-x86_64: <<: *test_full-loose_template image: python:3.10 @@ -540,6 +548,11 @@ test/full-loose/cp314-linux-x86_64: image: python:3.14 needs: - build/cp314-linux-x86_64 +test/full-loose/cp315-linux-x86_64: + <<: *test_full-loose_template + image: python:3.15-rc + needs: + - build/cp315-linux-x86_64 test/minimal-strict/cp310-linux-x86_64: <<: *test_minimal-strict_template image: python:3.10 @@ -565,6 +578,11 @@ test/minimal-strict/cp314-linux-x86_64: image: python:3.14 needs: - build/cp314-linux-x86_64 +test/minimal-strict/cp315-linux-x86_64: + <<: *test_minimal-strict_template + image: python:3.15-rc + needs: + - build/cp315-linux-x86_64 test/full-strict/cp310-linux-x86_64: <<: *test_full-strict_template image: python:3.10 @@ -590,6 +608,11 @@ test/full-strict/cp314-linux-x86_64: image: python:3.14 needs: - build/cp314-linux-x86_64 +test/full-strict/cp315-linux-x86_64: + <<: *test_full-strict_template + image: python:3.15-rc + needs: + - build/cp315-linux-x86_64 lint: <<: *common_template image: python:3.14 @@ -667,6 +690,8 @@ gpgsign/wheels: artifacts: true - job: build/cp314-linux-x86_64 artifacts: true + - job: build/cp315-linux-x86_64 + artifacts: true deploy/wheels: <<: *common_template image: python:3.14 diff --git a/docs/source/conf.py b/docs/source/conf.py index 91dd17a..a82a06f 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -236,6 +236,7 @@ def visit_Assign(self, node): 'xdoctest': ('https://xdoctest.readthedocs.io/en/latest/', None), 'networkx': ('https://networkx.org/documentation/stable/', None), 'scriptconfig': ('https://scriptconfig.readthedocs.io/en/latest/', None), + 'kwconf': ('https://kwconf.readthedocs.io/en/latest/', None), 'rich': ('https://rich.readthedocs.io/en/latest/', None), 'numpy': ('https://numpy.org/doc/stable/', None), 'sympy': ('https://docs.sympy.org/latest/', None), diff --git a/pyproject.toml b/pyproject.toml index 9fb8042..2bbd38d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,6 +25,7 @@ classifiers = [ "Programming Language :: Python :: 3.12", "Programming Language :: Python :: 3.13", "Programming Language :: Python :: 3.14", + "Programming Language :: Python :: 3.15", "Topic :: Software Development :: Libraries :: Python Modules", "Topic :: Utilities", ] @@ -75,7 +76,6 @@ packages.find.include = [ "cmd_queue*", ] - [tool.ruff] target-version = "py310" line-length = 80