refactor(nf-client): scheduler Submitter seam; de-duplicate submit/daemon#168
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…emon
The SLURM/PBS submission path (build the same Jinja context, render the
template, dispatch via sbatch/qsub with retries) was copied verbatim across
cli.py's submit() and daemon(). Extract it behind one seam in submission.py:
- build_submission_context(batch, cfg, sample_ids) — the context dict, once.
- SCHEDULER_SUBMITTERS registry {slurm, pbs} + submit_to_scheduler() entry
point; lsf is deliberately absent (raises UnwiredSchedulerError).
- Two named exceptions (TemplatePathMissingError, UnwiredSchedulerError) let
each caller keep its own control flow: submit() exits nonzero for both;
daemon() exits for a missing template but skips-and-continues for an unwired
mode. A genuine submission failure is NOT wrapped — it propagates as the
SubprocessError/OSError submit_with_retry already raised.
local mode is untouched — it's deliberately different per caller (non-blocking
Popen in submit(); blocking subprocess.run in daemon() to serialise runs).
Behaviour-preserving. One faithfulness fix over a naive extraction: the
daemon's submission-failure catch is narrowed from `except Exception` to
`except (SubprocessError, OSError)` so a template/render error (a config bug)
still stops the daemon as before, rather than being swallowed into an infinite
skip loop mislabelled "submission failed after retries".
New tests/test_submitters.py (10 tests): context keys, both dispatch commands,
export_none toggle, the two can't-try signals, and failure-propagates-as-is.
nf_client suite 60 passed; server suite 177 unaffected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the nf-client scheduler submission path (SLURM/PBS) by extracting the duplicated “build Jinja context → render template → submit with retries” logic from cli.py into a single seam in nf_client.submission. It keeps local mode behavior intentionally unchanged while making scheduler submission reusable and more testable.
Changes:
- Added a scheduler submitter registry +
submit_to_scheduler()entry point, with explicit “can’t even try” exceptions for missing template path and unwired scheduler modes. - Updated
cli.pysubmit()anddaemon()to use the extracted scheduler submission seam and preserve caller-specific control flow (exit vs skip/continue). - Added unit tests covering context-building, dispatch commands, export-none behavior, and failure propagation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/nf_client/src/nf_client/submission.py | Introduces the scheduler submitter seam (context builder, registry, dispatcher) and new exception types. |
| packages/nf_client/src/nf_client/cli.py | Replaces duplicated scheduler submission logic in submit() and daemon() with calls to submit_to_scheduler(). |
| packages/nf_client/tests/test_submitters.py | Adds unit tests for the new scheduler submission seam (context, dispatch, error signaling, failure propagation). |
| docs/roadmap.md | Marks roadmap item #4 as completed (references DONE #167). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+212
to
+216
| # unwired mode). A genuine submission failure (e.g. sbatch down after | ||
| # retries) is NOT wrapped here: it propagates as whatever submit_with_retry | ||
| # raises, exactly as it did before this refactor, so submit() still lets it | ||
| # crash uncaught while daemon()'s existing `except Exception: continue` | ||
| # still catches it. |
Comment on lines
+159
to
+162
| """A scheduler failure that exhausts retries propagates as-is (not wrapped) — | ||
| submit() lets it crash uncaught, daemon() catches bare Exception and continues. | ||
| """ | ||
| tmpl = tmp_path / "submit.sh.j2" |
Copilot review on #168: - Update the seam's module comment and the propagation test's docstring to reflect the daemon's narrowed `except (SubprocessError, OSError)` catch (they still described the old bare `except Exception`). - submit_to_scheduler: bind `template_path` locally and check `is None` so the Optional is narrowed to Path before render_submission_script (clearer for type-checkers than the truthiness check). nf_client suite still 60 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The SLURM/PBS submission path — build the same Jinja context, render the template, dispatch via
sbatch/qsubwith retries — was copied verbatim acrosscli.py'ssubmit()anddaemon(). This extracts it behind one seam insubmission.py. Roadmap "Architecture deepening" candidate #8.build_submission_context(batch, cfg, sample_ids)— the context dict, defined once.SCHEDULER_SUBMITTERSregistry (slurm,pbs) +submit_to_scheduler()entry point;lsfis deliberately absent →UnwiredSchedulerError.TemplatePathMissingError,UnwiredSchedulerError) let each caller keep its own control flow:submit()exits nonzero for both;daemon()exits for a missing template but skips-and-continues for an unwired mode. A genuine submission failure is not wrapped — it propagates as theSubprocessError/OSErrorsubmit_with_retryalready raised.localis untouched — on purposelocalmode is deliberately different per caller and left exactly as-is: non-blockingPopeninsubmit(), blockingsubprocess.runindaemon()(which serialises local runs — making it non-blocking would break the daemon). This refactor only touches the duplicated scheduler path.Behaviour
Behaviour-preserving. One faithfulness fix over a naive extraction: the daemon's submission-failure catch is narrowed from
except Exceptiontoexcept (SubprocessError, OSError), so a template/render error (a deterministic config bug) still stops the daemon as it did when rendering happened outside thetry— rather than being swallowed into an infinite skip loop mislabelled "submission failed after retries".Tests
New
tests/test_submitters.py(10 tests): context keys +defaultsmerge, both dispatch commands (sbatch --parsable --export=NONE/qsub), theexport_none=Falsetoggle, the two can't-try signals, and failure-propagates-as-CalledProcessError.just test): 177 passed (unaffected — nf_client has its own test config)🤖 Generated with Claude Code