Run legacy apps under app-runner without the wrapper creator - #603
Draft
leoschwarz wants to merge 4 commits into
Draft
Run legacy apps under app-runner without the wrapper creator#603leoschwarz wants to merge 4 commits into
leoschwarz wants to merge 4 commits into
Conversation
…t type Legacy applications expect the wrapper creator's `config.yaml` as their sole argument, which has kept the wrapper creator, its submitter and the external-job machinery alive purely as a YAML generator. Port the two section builders out of `BfabricWrapperCreator`, dropping every `client.save`, and expose them as a `legacy_wrapper_yaml` input type. Because nothing is written to B-Fabric, resolution stays side-effect free and can run on every `inputs prepare`/`list`/`check`, which is what lets this live in the input resolver rather than in dispatch. Four fields necessarily differ from the wrapper creator's output. The output path comes from the spec, since app-runner registers the result itself and there is no pre-created resource to address; and the output, stdout and stderr resource ids plus the external job id are `0`. Sentinel rather than null keeps an app that flattens the YAML into shell variables working under `set -u`. Diffed against the wrapper creator's own output for workunit 349972: those are the only differences. Adds `workunit` to the resolver's entity cache, since grouping input resources by their producing application reads one workunit per resource. Refs #262
…mands Complete the path from a B-Fabric workunit to a legacy application without the wrapper creator. `legacy dispatch` writes a single chunk holding only the legacy YAML -- a legacy app fetches its own inputs from the scp URLs inside it, so nothing else is staged, and no B-Fabric client is needed since the workunit id comes from `workunit_definition.yml`. `legacy run` invokes the app with the YAML path as its last argument. `legacy collect` writes the chunk's `outputs.yml`. Together they let an `app.yml` wrap a legacy app with no per-app Python. `legacy run` prepends a generated directory of shims to PATH. Six are no-ops: app-runner already sets the workunit status, the resource status and the workflow step, and the ids those commands would be handed are sentinels, so left alone they would at best duplicate work and at worst mark the wrong entity. `bfabric_upload_resource.py` is redirected rather than neutralised. It base64s the file over SOAP, which makes B-Fabric file the resource on its internal storage -- a maintenance burden for the B-Fabric developers -- instead of the application's. The shim records the path for `collect` to declare, so those resources land on the same storage as the main output. Recording rather than copying, because no legacy app removes its scratch directory before exiting and the files can be large. The shims are generated at runtime rather than shipped as package data: nothing preserves an executable bit through a wheel, and app-runner often runs from an ephemeral `uv run --with` environment, so there is no stable path to point at. `collect` tolerates a missing declared output when uploads were recorded, for the few apps that only ever upload extra resources. An app that should have written its output fails its own scp first, which fails the process step, so collect never runs in that case. Refs #262
…run` `legacy collect` could never have worked: app-runner invokes a collect command with two trailing arguments, `$workunit_ref $chunk_dir` (runner.py:53), while `cmd_legacy_collect` accepted only the chunk directory, so cyclopts rejected the second token as unused. The documented app.yml would have failed on its first real run. Rather than patch the arity, drop the command. `collect` is optional precisely because a process command can write `outputs.yml` itself, and it is not its own lifecycle step but the head of the outputs action -- execute.py has called it "the legacy collect step" since dc511e4. Writing the declaration at the end of `legacy run` keeps a legacy app spec to the same two commands a modern app uses. Two smaller wins fall out of the move: a `--read-only` rehearsal now produces an inspectable `outputs.yml` (execute_outputs returns before run_collect, so it previously produced none), and a hand-corrected `outputs.yml` survives a re-run of `make stage` instead of being regenerated over. The cost is that regenerating the declaration now needs the app re-run, which is acceptable because every way it can fail is either deterministic or already required re-running the app.
… layer Found by a review pass over the branch. `legacy run` appended to `legacy_uploads.txt` but never truncated it, so a retry of the process step inherited the previous run's paths: either `_require_file` killed an otherwise successful run, or stale files were silently registered as resources of this run. The manifest is now reset before the app starts. `cmd_legacy_dispatch` built `output_path` from an unresolved `work_dir`, so a relative work directory baked a relative path into `application.output`. Legacy apps `cd` before their `scp`, which sent the output somewhere neither the app nor output registration expected. It also wrote an absolute chunk path into `chunks.yml`, contrary to the contract `ChunksFile` documents and `Runner.infer_from_directory` produces; that only worked because `work_dir / absolute` discards the left operand. A `host:path` destination was rejected only while declaring outputs, i.e. after the app had run for hours -- it is now checked before the app starts, and the `output_path` docs no longer offer a remote URL as a workaround, since it never worked end-to-end. An app that uploads its own declared output put the same file in both the declared and uploaded lists, so the duplicate-name guard aborted a fully successful run. Declarations are now deduplicated by resolved path first. Also: state the `job_configuration.output` protocol/ssh_args divergence in `build_legacy_wrapper_yaml`'s docstring instead of denying it, raise an explicit error for an executable with no `program`, document dispatch's two positionals for `--help`, and avoid a trailing empty PATH element (which means the current directory) when PATH is unset. Two review findings were rejected after checking the real apps: shimming `..._failed.py` loses nothing, because every caller either exits non-zero straight after or fires it from an EXIT trap, and the write commands that are left unshimmed all take the real workunit id, so shimming them would drop a user-visible feature. Both are now documented.
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.
legacy_wrapper_yamlinput type that writes the legacy wrapper-creatorconfig.yamlfor a workunit, reading only from B-Fabric.legacy dispatchandlegacy runso anapp.ymlcan wrap a legacy app with no per-app Python;rundeclares the outputs itself, so nocollectcommand is needed.bfabric_upload_resource.pycalls to register on the application's storage instead of B-Fabric's internal storage.slurm_stdout/slurm_stderrresources, since app-runner owns that state itself.fgcz_slurm_maxquant_textfiles.bashandfgcz_slurm_SummarizedExperiment_A315.bashparse the output path assuming ahost:pathshape and need adapting before they can run this way.Closes #262
🤖 Prepared with assistance from Claude Opus 5 via Claude Code.