diff --git a/bfabric_app_runner/docs/changelog.md b/bfabric_app_runner/docs/changelog.md index 4548ec54..95939f5a 100644 --- a/bfabric_app_runner/docs/changelog.md +++ b/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. 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")