Let an app version declare its own sbatch parameters - #584
Draft
leoschwarz wants to merge 10 commits into
Draft
Conversation
An app version may now carry extra sbatch flags in its app.yml, which the slurm submitter merges between its own defaults and the per-workunit parameters. A null value removes a flag the submitter would otherwise pass. Reading the app spec at submit time never fails a submission: any problem falls back to the submitter defaults with a warning, since the job itself reports app spec errors with more context. Flags that carry the job's logging, working directory and environment are reserved, as are workunit variables, which are not in scope when an app spec is evaluated.
Adds a task-shaped section to the app guide (what to write, what a user can still override, how to unset a flag, which flags are refused, how to check it took effect) plus the field-level reference and a changelog entry.
Two silent-wrongness cases, both caught at spec validation: - YAML parses an unquoted 24:00:00 as the integer 86400, and sbatch reads a bare integer --time as minutes, so an app asking for a day would silently get 60. - Each flag renders as one #SBATCH line, so a newline in a value would append lines to the generated job script.
The merge of main brought #600's stricter ApiResponseDataType, which made Path(workunit.application.executable["program"]) a type error. Cast at the call site as the sibling WrapAppYamlTemplate already does.
A workunit only carries partition, nodelist, mem and time, and 49 of 60 app-runner workunits sampled on TEST name the memory field "memory", which SlurmWorkunitParams does not recognise and silently drops. "A user's own choice still wins" was therefore false for memory on most apps.
The "memory" submitter parameter belongs to the legacy yaml___Slurm_executable, which consumes it in bfabric.wrapper_creator.bfabric_submitter. Workunits served by the app-runner submitter always carry --mem/--nodelist/--partition/--time, so SlurmWorkunitParams never sees "memory" and nothing is silently dropped. The four-flag limit on what a workunit can override does hold, and stays.
The field holds sbatch flags and is validated against SLURM semantics (reserved #SBATCH flags, sexagesimal durations), so submitter_params promised a generality the schema never had: no non-SLURM submitter could consume the dict. Unreleased, so no app.yml carries the old key yet. SlurmParameters.submitter_params keeps its name -- that one really is the submitter deployment's own defaults from submitter.yml.
…g a single-version spec _evaluate_app_params re-derived what resolve_app already knew: the application_version parameter name and the spec lookup. Both now go through AppSpec.for_parameters, so the submitter and the runner cannot drift apart on which version a workunit resolves to. That shared method also drops the requirement for an application_version when the spec defines exactly one version, so an app that never had the parameter is a drop-in. An explicitly named version is still taken literally, so nothing is substituted silently -- which is the part #236 was closed over. The callers keep their own error policy: the runner raises with the available versions, the submitter warns and falls back to the submitter defaults.
Contributor
📝 "TODO" Changes DetectedSummary: ✅ 2 "TODO"s removed ✅ Removed "TODO"s (2)
This comment is automatically updated when "TODO" changes are detected. |
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
1 participant
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.
Every app currently gets whatever resources its submitter deployment was configured with, so there is no way to say "this app needs 24 cores and that one needs 1" short of running a submitter per app. An app version can now carry
slurm_params— plain sbatch flags — which the SLURM submitter merges into the job it generates.This restores the
paramshalf of theSubmitterReffield deleted in c778d65 ("Modernize submitter integration", #207), this time actually read by the submitter. Thenamehalf and theSubmittersSpecregistry are deliberately not restored: B-Fabric decides which submitter runs a workunit, so a name could only select a profile inside the submitter already running, and the app version is a better selector for the same job — adevelversion can carry different params than the released one, with no registry to keep in sync per environment.Key points:
nullvalue removes a flag the submitter would otherwise pass, which is the only way to unset one.application_versionall fall back to the submitter defaults with a warning. A submission must not fail over the app spec, since the job itself reports spec problems with far more context.validate app-speccatches them before deployment:--output/--error/--chdir/--export(the submitter owns the job's logging, working directory and environment),${workunit...}values (no workunit is in scope when a spec is evaluated), integer--time(YAML reads an unquoted24:00:00as86400, which sbatch reads as minutes), and multi-line values (each flag becomes one#SBATCHline).Trade-off worth a reviewer's opinion:
RESERVED_SBATCH_FLAGSlives in the spec module, sospecs/now encodes something the submitter owns. The alternative was dropping those flags at merge time with a warning; validation-time rejection was chosen because a warning in a submitter log is not seen by the person editing the app.yml.Out of scope here: adding
cpus_per_taskto the workunit whitelist, and translating a multi-nodenodelistinto--excludeof the partition complement.Verification: 79 tests in
tests/bfabric_app_runner/specsand.../bfabric_integration, basedpyright clean. The 6 failures indispatch/test_dispatch_resource_flow.pyare pre-existing on main (pandera on Python 3.14).🤖 Prepared with assistance from Claude Opus 5 via Claude Code.