From 8c3d5c933c4d3a848326228131e58f2a0f006cf7 Mon Sep 17 00:00:00 2001 From: Leonardo Schwarz Date: Wed, 12 Aug 2026 12:30:30 +0200 Subject: [PATCH 1/2] refactor(app-runner): remove the legacy default-resource reuse from outputs Copied outputs always register a new resource now, instead of recycling the pending placeholder resource that the legacy wrapper creator pre-created on the workunit. The current submitter creates no such placeholder, so this was already a no-op for apps submitted through it. Closes #361 --- bfabric_app_runner/docs/changelog.md | 1 + .../docs/user_guides/cli_reference.md | 3 - .../docs/user_guides/working_with_outputs.md | 6 -- .../src/bfabric_app_runner/actions/execute.py | 1 - .../bfabric_app_runner/app_runner/runner.py | 1 - .../src/bfabric_app_runner/cli/outputs.py | 5 - .../output_registration/register.py | 38 +------- .../specs/app/app_version.py | 15 --- .../app_runner/test_runner.py | 1 - .../output_registration/test_register_all.py | 91 +++++++++++++++++++ .../specs/app/test_app_version.py | 10 +- 11 files changed, 102 insertions(+), 70 deletions(-) create mode 100644 tests/bfabric_app_runner/output_registration/test_register_all.py diff --git a/bfabric_app_runner/docs/changelog.md b/bfabric_app_runner/docs/changelog.md index 803b5927..de6be135 100644 --- a/bfabric_app_runner/docs/changelog.md +++ b/bfabric_app_runner/docs/changelog.md @@ -4,6 +4,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## \[Unreleased\] +- The legacy default-resource reuse is removed from output registration ([#361](https://github.com/fgcz/bfabricPy/issues/361)). Copied outputs now always register a new resource, instead of recycling the `pending` placeholder resource that the old wrapper creator pre-created on the workunit. The current submitter creates no such placeholder, so this was already a no-op for apps submitted through it. The `reuse_default_resource` app-spec field and the `--reuse-default-resource` / `--no-reuse-default-resource` options of `outputs register` and `outputs register-single-file` are gone; an `app.yml` that still sets the field keeps working, as the key is ignored rather than rejected. A workunit submitted through the *legacy* wrapper creator will now keep its unused `pending` placeholder resource next to the real outputs. - Relative paths in the app spec (`pylock`, `local_extra_deps`, `prepend_paths`, and the host side of the docker `mounts` entries) are now resolved against the directory containing the `app.yml`, and a leading `~` is expanded ([#212](https://github.com/fgcz/bfabricPy/issues/212)). Previously they were resolved against the working directory of the run (a per-workunit scratch directory), which could not be relied on. Container-side mount paths are unchanged. - New `${app.dir}` template variable holding the app spec's directory, for paths that no spec field can resolve, e.g. a script referenced from inside a `command` string: `command: uv run --script ${app.dir}/dispatch.py`. Together with the above, an app spec directory can be relocated or checked out elsewhere without rewriting its paths. - A failing app command now reports as a single `Error: Command failed with exit code N: ` line instead of stacking ~10 frames of app-runner boilerplate under the app's own error output ([#231](https://github.com/fgcz/bfabricPy/issues/231)). Commands raise the new `CommandFailedError`, which the CLI renders without a traceback; `BFABRICPY_LOG_LEVEL=DEBUG` still shows it. A genuine app-runner bug (e.g. a `TypeError`) keeps its full traceback as before. diff --git a/bfabric_app_runner/docs/user_guides/cli_reference.md b/bfabric_app_runner/docs/user_guides/cli_reference.md index 82b01268..9e3e81fb 100644 --- a/bfabric_app_runner/docs/user_guides/cli_reference.md +++ b/bfabric_app_runner/docs/user_guides/cli_reference.md @@ -11,9 +11,6 @@ Every command that talks to B-Fabric also accepts these options (omitted from th | `--config-env` | Override the config environment; falls back to `BFABRICPY_CONFIG_ENV` or the file default | | `--config-file`| Override the config file path (default `~/.bfabricpy.yml`) | -The `outputs` commands additionally accept `--reuse-default-resource` / `--no-reuse-default-resource`, which -controls whether the workunit's auto-created default resource is reused. - ## run ### run workunit diff --git a/bfabric_app_runner/docs/user_guides/working_with_outputs.md b/bfabric_app_runner/docs/user_guides/working_with_outputs.md index 8a30b8a3..bfe05f03 100644 --- a/bfabric_app_runner/docs/user_guides/working_with_outputs.md +++ b/bfabric_app_runner/docs/user_guides/working_with_outputs.md @@ -114,9 +114,6 @@ bfabric-app-runner outputs register outputs.yml workunit_ref `--force-storage` : Override the storage location. -`--reuse-default-resource` / `--no-reuse-default-resource` -: Whether to reuse the workunit's auto-created default resource for the first copied file (default: enabled). - ### Register a single file Register a single output file without an outputs YAML: @@ -148,9 +145,6 @@ bfabric-app-runner outputs register-single-file results/output.mzML \ `--force-storage` : Override the storage location. -`--reuse-default-resource` / `--no-reuse-default-resource` -: Whether to reuse the workunit's auto-created default resource (default: disabled for this command). - ## Validating Output Specs Validate an outputs YAML file: diff --git a/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py b/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py index a82a034d..971a5112 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py +++ b/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py @@ -122,7 +122,6 @@ def execute_outputs(action: ActionOutputs, client: Bfabric) -> None: client=client, ssh_user=action.ssh_user, force_storage=action.force_storage, - reuse_default_resource=True, ) # Create a workflowstep template if specified if bfabric_app_spec.workflow_template_step_id: diff --git a/bfabric_app_runner/src/bfabric_app_runner/app_runner/runner.py b/bfabric_app_runner/src/bfabric_app_runner/app_runner/runner.py index 2a83cdf5..ac867357 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/app_runner/runner.py +++ b/bfabric_app_runner/src/bfabric_app_runner/app_runner/runner.py @@ -179,7 +179,6 @@ def run_app( workunit_definition=workunit_definition, client=client, ssh_user=ssh_user, - reuse_default_resource=app_spec.reuse_default_resource, force_storage=force_storage, ) diff --git a/bfabric_app_runner/src/bfabric_app_runner/cli/outputs.py b/bfabric_app_runner/src/bfabric_app_runner/cli/outputs.py index ffcc843a..9df7705d 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/cli/outputs.py +++ b/bfabric_app_runner/src/bfabric_app_runner/cli/outputs.py @@ -24,8 +24,6 @@ def cmd_outputs_register( ssh_user: str | None = None, force_storage: Path | None = None, client: Bfabric, - # TODO - reuse_default_resource: bool = True, ) -> None: """Register the output files of a workunit.""" register_outputs( @@ -33,7 +31,6 @@ def cmd_outputs_register( workunit_definition=_get_workunit_definition(client, workunit_ref), client=client, ssh_user=ssh_user, - reuse_default_resource=reuse_default_resource, force_storage=force_storage, ) @@ -48,7 +45,6 @@ def cmd_outputs_register_single_file( update_existing: UpdateExisting = UpdateExisting.NO, ssh_user: str | None = None, force_storage: Path | None = None, - reuse_default_resource: bool = False, client: Bfabric, ) -> None: """Register a single file in the workunit. @@ -70,6 +66,5 @@ def cmd_outputs_register_single_file( workunit_definition=_get_workunit_definition(client, workunit_ref), specs_list=[spec], ssh_user=ssh_user, - reuse_default_resource=reuse_default_resource, force_storage=force_storage, ) diff --git a/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py b/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py index 006f9b8e..8bcd0e8e 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py +++ b/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py @@ -4,7 +4,7 @@ import polars as pl import yaml -from bfabric.entities import Dataset, Resource, Storage, Workunit +from bfabric.entities import Dataset, Resource, Storage from bfabric.operations.dataset import ( CreateDatasetParams, create_dataset, @@ -55,13 +55,9 @@ def register_file_in_workunit( spec: CopyResourceSpec, client: Bfabric, workunit_definition: WorkunitDefinition, - resource_id: int | None = None, ) -> None: """Registers a file in the workunit.""" existing_id = _identify_existing_resource_id(client, spec, workunit_definition) - if resource_id is not None and existing_id is not None and resource_id != existing_id: - raise ValueError(f"Resource id {resource_id} does not match existing resource id {existing_id}") - checksum = md5_checksum(spec.local_path) output_folder = _get_output_folder(spec, workunit_definition=workunit_definition) resource_data = { @@ -73,8 +69,6 @@ def register_file_in_workunit( "status": "available", "size": spec.local_path.stat().st_size, } - if resource_id is not None: - resource_data["id"] = resource_id if existing_id is not None: resource_data["id"] = existing_id @@ -228,30 +222,14 @@ def _save_link(spec: SaveLinkSpec, client: Bfabric, workunit_definition: Workuni logger.info(f"Link {spec.name} saved with id {res[0]['id']} for entity {entity_type} with id {entity_id}") -def find_default_resource_id(workunit_definition: WorkunitDefinition, client: Bfabric) -> int | None: - """Finds the default resource's id for the workunit. Maybe in the future, this will be always `None`.""" - workunit_id = workunit_definition.registration.workunit_id # pyright: ignore[reportOptionalMemberAccess] - workunit = client.reader.read_id(Workunit, workunit_id) - candidate_resources = [ - resource for resource in workunit.resources if resource["name"] not in ["slurm_stdout", "slurm_stderr"] - ] - # We also check that the resource is pending, as else we might re-use a resource that was created by the app... - if len(candidate_resources) == 1 and candidate_resources[0]["status"] == "pending": - return candidate_resources[0].id - return None - - def register_all( client: Bfabric, workunit_definition: WorkunitDefinition, specs_list: list[SpecType], ssh_user: str | None, - reuse_default_resource: bool, force_storage: Path | None, ) -> None: """Registers all the output specs to the workunit.""" - default_resource_was_reused = not reuse_default_resource - storage = _get_storage(client, force_storage, specs_list, workunit_definition) logger.info(f"Using storage: {storage}") @@ -264,17 +242,7 @@ def register_all( storage=storage, ssh_user=ssh_user, ) - if not default_resource_was_reused: - resource_id = find_default_resource_id(workunit_definition=workunit_definition, client=client) - default_resource_was_reused = True - else: - resource_id = None - register_file_in_workunit( - spec, - client=client, - workunit_definition=workunit_definition, - resource_id=resource_id, - ) + register_file_in_workunit(spec, client=client, workunit_definition=workunit_definition) elif isinstance(spec, SaveDatasetSpec): _save_dataset(spec, client, workunit_definition=workunit_definition) elif isinstance(spec, SaveLinkSpec): @@ -302,7 +270,6 @@ def register_outputs( workunit_definition: WorkunitDefinition, client: Bfabric, ssh_user: str | None, - reuse_default_resource: bool, force_storage: Path | None, ) -> None: """Registers outputs to the workunit.""" @@ -312,6 +279,5 @@ def register_outputs( workunit_definition=workunit_definition, specs_list=specs_list, ssh_user=ssh_user, - reuse_default_resource=reuse_default_resource, force_storage=force_storage, ) diff --git a/bfabric_app_runner/src/bfabric_app_runner/specs/app/app_version.py b/bfabric_app_runner/src/bfabric_app_runner/specs/app/app_version.py index e4239885..27b90cca 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/specs/app/app_version.py +++ b/bfabric_app_runner/src/bfabric_app_runner/specs/app/app_version.py @@ -20,11 +20,6 @@ class AppVersion(BaseModel): commands: CommandsSpec """The dispatch, process, and (optional) collect commands that implement this version.""" - # TODO remove when new submitter becomes available - reuse_default_resource: bool = True - """Legacy flag: reuse the workunit's auto-created default resource for the first copied output - instead of creating a new resource entry.""" - class AppVersionTemplate(BaseModel): """Template for a single app version, expanded to an ``AppVersion`` after variable interpolation.""" @@ -35,11 +30,6 @@ class AppVersionTemplate(BaseModel): commands: CommandsSpec """The dispatch, process, and (optional) collect commands that implement this version.""" - # TODO remove when new submitter becomes available - reuse_default_resource: bool = True - """Legacy flag: reuse the workunit's auto-created default resource for the first copied output - instead of creating a new resource entry.""" - def evaluate(self, variables_app: VariablesApp) -> AppVersion: """Evaluates the template to a concrete ``AppVersion`` instance.""" data_template = self.model_dump(mode="json") @@ -59,11 +49,6 @@ class AppVersionMultiTemplate(BaseModel): commands: CommandsSpec """The dispatch, process, and (optional) collect commands that implement these versions.""" - # TODO remove when new submitter becomes available - reuse_default_resource: bool = True - """Legacy flag: reuse the workunit's auto-created default resource for the first copied output - instead of creating a new resource entry.""" - @field_validator("version", mode="before") def _version_ensure_list(cls, values: Any) -> list[str]: if not isinstance(values, list): diff --git a/tests/bfabric_app_runner/app_runner/test_runner.py b/tests/bfabric_app_runner/app_runner/test_runner.py index 58a5cabb..bac09add 100644 --- a/tests/bfabric_app_runner/app_runner/test_runner.py +++ b/tests/bfabric_app_runner/app_runner/test_runner.py @@ -19,7 +19,6 @@ class MockCommands(BaseModel): class MockAppVersion(BaseModel): commands: MockCommands = MockCommands() - reuse_default_resource: bool = False @pytest.fixture diff --git a/tests/bfabric_app_runner/output_registration/test_register_all.py b/tests/bfabric_app_runner/output_registration/test_register_all.py new file mode 100644 index 00000000..8230c374 --- /dev/null +++ b/tests/bfabric_app_runner/output_registration/test_register_all.py @@ -0,0 +1,91 @@ +from pathlib import Path + +import pytest +from bfabric.entities import Workunit +from bfabric_app_runner.output_registration.register import register_all +from bfabric_app_runner.specs.outputs_spec import CopyResourceSpec + + +def _resource(mocker, *, id: int, name: str, status: str): + """A resource as `Workunit.resources` yields it: attribute `id`, item access for the rest.""" + resource = mocker.MagicMock(id=id) + resource.__getitem__.side_effect = {"name": name, "status": status}.__getitem__ + return resource + + +class TestRegisterAll: + """Output registration always creates a fresh resource, never recycling a pre-existing one. + + The workunit resources set up here are what the legacy WrapperCreator left behind: a `pending` + placeholder output resource plus two `slurm_std*` log resources. Recycling the placeholder's id + was the legacy `reuse_default_resource` behaviour, removed in #361. + """ + + @pytest.fixture() + def spec(self, tmp_path) -> CopyResourceSpec: + local_path = tmp_path / "result.txt" + local_path.write_text("payload") + return CopyResourceSpec(local_path=local_path, store_entry_path=Path("result.txt")) + + @pytest.fixture() + def workunit_definition(self, mocker): + wd = mocker.MagicMock() + wd.registration.workunit_id = 5000 + wd.registration.storage_id = 2 + wd.registration.storage_output_folder = Path("out/folder") + return wd + + @pytest.fixture() + def client(self, mocker): + """Client whose `reader.read_id` answers per entity type, with `resources` set by each test.""" + + def read_id(entity_type, entity_id): + entity = mocker.MagicMock() + if entity_type is Workunit: + entity.resources = client.workunit_resources + return entity + + client = mocker.MagicMock() + client.workunit_resources = [] + client.reader.read_id.side_effect = read_id + client.reader.query_one.return_value = None + return client + + @pytest.fixture(autouse=True) + def _mock_transfer(self, mocker): + mocker.patch("bfabric_app_runner.output_registration.register.copy_file_to_storage") + mocker.patch("bfabric_app_runner.output_registration.register.md5_checksum", return_value="checksum") + + @pytest.mark.parametrize( + "resource_names_and_status", + [ + pytest.param([], id="no_existing_resources"), + pytest.param([("App 1 - resource", "pending")], id="legacy_placeholder_only"), + pytest.param( + [("App 1 - resource", "pending"), ("slurm_stdout", "available"), ("slurm_stderr", "available")], + id="legacy_placeholder_with_log_resources", + ), + pytest.param([("earlier.txt", "available")], id="resource_created_by_the_app"), + ], + ) + def test_registers_a_new_resource(self, client, workunit_definition, spec, mocker, resource_names_and_status): + client.workunit_resources = [ + _resource(mocker, id=100 + index, name=name, status=status) + for index, (name, status) in enumerate(resource_names_and_status) + ] + + register_all( + client=client, + workunit_definition=workunit_definition, + specs_list=[spec], + ssh_user=None, + force_storage=None, + ) + + endpoint, resource_data = client.save.call_args.args + assert endpoint == "resource" + assert "id" not in resource_data + assert resource_data["name"] == "result.txt" + assert resource_data["workunitid"] == 5000 + assert resource_data["relativepath"] == "out/folder/result.txt" + assert resource_data["status"] == "available" diff --git a/tests/bfabric_app_runner/specs/app/test_app_version.py b/tests/bfabric_app_runner/specs/app/test_app_version.py index 0be9f14a..779ef497 100644 --- a/tests/bfabric_app_runner/specs/app/test_app_version.py +++ b/tests/bfabric_app_runner/specs/app/test_app_version.py @@ -23,7 +23,6 @@ def parsed() -> AppVersion: ), collect=CommandShell(command="collect"), ), - reuse_default_resource=True, ) @@ -53,7 +52,6 @@ def serialized() -> str: work_dir_target: null writeable: [] type: docker -reuse_default_resource: true version: 0.0.1""" @@ -63,3 +61,11 @@ def test_serialize(parsed, serialized): def test_parse(parsed, serialized): assert AppVersion.model_validate(yaml.safe_load(serialized)) == parsed + + +def test_parse_ignores_removed_reuse_default_resource(parsed, serialized): + """An app.yml still carrying the removed legacy flag must keep parsing, not be rejected.""" + data = yaml.safe_load(serialized) | {"reuse_default_resource": True} + app_version = AppVersion.model_validate(data) + assert app_version == parsed + assert not hasattr(app_version, "reuse_default_resource") From 533fe4933ef4c28385bbfdecdf34e2c18e6d51a6 Mon Sep 17 00:00:00 2001 From: Leonardo Schwarz Date: Wed, 12 Aug 2026 12:33:44 +0200 Subject: [PATCH 2/2] docs(app-runner): condense the #361 changelog entry --- bfabric_app_runner/docs/changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bfabric_app_runner/docs/changelog.md b/bfabric_app_runner/docs/changelog.md index de6be135..807e8980 100644 --- a/bfabric_app_runner/docs/changelog.md +++ b/bfabric_app_runner/docs/changelog.md @@ -4,7 +4,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## \[Unreleased\] -- The legacy default-resource reuse is removed from output registration ([#361](https://github.com/fgcz/bfabricPy/issues/361)). Copied outputs now always register a new resource, instead of recycling the `pending` placeholder resource that the old wrapper creator pre-created on the workunit. The current submitter creates no such placeholder, so this was already a no-op for apps submitted through it. The `reuse_default_resource` app-spec field and the `--reuse-default-resource` / `--no-reuse-default-resource` options of `outputs register` and `outputs register-single-file` are gone; an `app.yml` that still sets the field keeps working, as the key is ignored rather than rejected. A workunit submitted through the *legacy* wrapper creator will now keep its unused `pending` placeholder resource next to the real outputs. +- Output registration always creates a new resource instead of recycling the legacy wrapper creator's `pending` placeholder ([#361](https://github.com/fgcz/bfabricPy/issues/361)); the `reuse_default_resource` field and its CLI options are gone, but remain tolerated in an `app.yml`. - Relative paths in the app spec (`pylock`, `local_extra_deps`, `prepend_paths`, and the host side of the docker `mounts` entries) are now resolved against the directory containing the `app.yml`, and a leading `~` is expanded ([#212](https://github.com/fgcz/bfabricPy/issues/212)). Previously they were resolved against the working directory of the run (a per-workunit scratch directory), which could not be relied on. Container-side mount paths are unchanged. - New `${app.dir}` template variable holding the app spec's directory, for paths that no spec field can resolve, e.g. a script referenced from inside a `command` string: `command: uv run --script ${app.dir}/dispatch.py`. Together with the above, an app spec directory can be relocated or checked out elsewhere without rewriting its paths. - A failing app command now reports as a single `Error: Command failed with exit code N: ` line instead of stacking ~10 frames of app-runner boilerplate under the app's own error output ([#231](https://github.com/fgcz/bfabricPy/issues/231)). Commands raise the new `CommandFailedError`, which the CLI renders without a traceback; `BFABRICPY_LOG_LEVEL=DEBUG` still shows it. A genuine app-runner bug (e.g. a `TypeError`) keeps its full traceback as before.