refactor(app-runner): remove the legacy default-resource reuse from outputs

bfabric_app_runner/docs/changelog.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Changed
 
+- 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`, host side of docker `mounts`) resolve against the `app.yml` directory instead of the run's scratch directory, with a leading `~` expanded ([#212](https://github.com/fgcz/bfabricPy/issues/212)).
 - Captured command output (`uv venv` / `uv pip install`) is logged one record per line, so every line carries its level prefix.
 - Requires `bfabric` 1.20.1 for the logging fix that keeps a nested app-runner from falling back to loguru's verbose DEBUG format.

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

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:

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:

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,
             )
 

bfabric_app_runner/src/bfabric_app_runner/cli/outputs.py

@@ -24,16 +24,13 @@ 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(
         outputs_yaml=outputs_yaml,
         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,
     )

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,
     )

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):

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

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"

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")