From 944450aa38d989126d12703d9785b3e338c8a309 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Thu, 9 Jan 2025 18:18:39 +0000 Subject: [PATCH 1/2] Uniform use of tmp_path in ro-crate tests --- test/unit/data/model/test_model_store.py | 59 ++++++++++-------------- 1 file changed, 24 insertions(+), 35 deletions(-) diff --git a/test/unit/data/model/test_model_store.py b/test/unit/data/model/test_model_store.py index cbf8afa5a7e2..99d43c556251 100644 --- a/test/unit/data/model/test_model_store.py +++ b/test/unit/data/model/test_model_store.py @@ -383,7 +383,7 @@ def test_import_library_require_permissions(): assert error_caught -def test_import_export_library(): +def test_import_export_library(tmp_path): """Test basics of library, library folder, and library dataset import/export.""" app = _mock_app() sa_session = app.model.context @@ -412,12 +412,11 @@ def test_import_export_library(): assert len(root_folder.datasets) == 1 assert len(root_folder.folders) == 1 - temp_directory = mkdtemp() - with store.DirectoryModelExportStore(temp_directory, app=app) as export_store: + with store.DirectoryModelExportStore(tmp_path, app=app) as export_store: export_store.export_library(library) import_model_store = store.get_import_model_store_for_directory( - temp_directory, app=app, user=u, import_options=store.ImportOptions(allow_library_creation=True) + tmp_path, app=app, user=u, import_options=store.ImportOptions(allow_library_creation=True) ) import_model_store.perform_import() @@ -593,7 +592,7 @@ def validate_invocation_collection_crate_directory(crate_directory): assert dataset in root["hasPart"] -def test_export_history_with_missing_hid(): +def test_export_history_with_missing_hid(tmp_path): # The dataset's hid was used to compose the file name during the export but it # can be missing sometimes. We now use the dataset's encoded id instead. app = _mock_app() @@ -603,8 +602,7 @@ def test_export_history_with_missing_hid(): d1.hid = None app.commit() - temp_directory = mkdtemp() - with store.DirectoryModelExportStore(temp_directory, app=app, export_files="copy") as export_store: + with store.DirectoryModelExportStore(tmp_path, app=app, export_files="copy") as export_store: export_store.export_history(history) @@ -612,37 +610,33 @@ def test_export_history_to_ro_crate(tmp_path): app = _mock_app() u, history, d1, d2, j = _setup_simple_cat_job(app) - crate_directory = tmp_path / "crate" - with store.ROCrateModelExportStore(crate_directory, app=app) as export_store: + with store.ROCrateModelExportStore(tmp_path, app=app) as export_store: export_store.export_history(history) - validate_history_crate_directory(crate_directory) + validate_history_crate_directory(tmp_path) def test_export_invocation_to_ro_crate(tmp_path): app = _mock_app() workflow_invocation = _setup_invocation(app) - crate_directory = tmp_path / "crate" - with store.ROCrateModelExportStore(crate_directory, app=app) as export_store: + with store.ROCrateModelExportStore(tmp_path, app=app) as export_store: export_store.export_workflow_invocation(workflow_invocation) - validate_invocation_crate_directory(crate_directory) + validate_invocation_crate_directory(tmp_path) def test_export_simple_invocation_to_ro_crate(tmp_path): app = _mock_app() workflow_invocation = _setup_simple_invocation(app) - crate_directory = tmp_path / "crate" - with store.ROCrateModelExportStore(crate_directory, app=app) as export_store: + with store.ROCrateModelExportStore(tmp_path, app=app) as export_store: export_store.export_workflow_invocation(workflow_invocation) - validate_invocation_crate_directory(crate_directory) + validate_invocation_crate_directory(tmp_path) def test_export_collection_invocation_to_ro_crate(tmp_path): app = _mock_app() workflow_invocation = _setup_collection_invocation(app) - crate_directory = tmp_path / "crate" - with store.ROCrateModelExportStore(crate_directory, app=app) as export_store: + with store.ROCrateModelExportStore(tmp_path, app=app) as export_store: export_store.export_workflow_invocation(workflow_invocation) - validate_invocation_collection_crate_directory(crate_directory) + validate_invocation_collection_crate_directory(tmp_path) def test_export_invocation_to_ro_crate_archive(tmp_path): @@ -727,7 +721,7 @@ def test_import_export_edit_datasets(): assert d1.dataset.object_store_id == "foo1", d1.dataset.object_store_id -def test_import_export_edit_collection(): +def test_import_export_edit_collection(tmp_path): """Test modifying existing collections with imports.""" app = _mock_app() sa_session = app.model.context @@ -743,13 +737,12 @@ def test_import_export_edit_collection(): import_history = model.History(name="Test History for Import", user=u) app.add_and_commit(import_history) - temp_directory = mkdtemp() - with store.DirectoryModelExportStore(temp_directory, app=app, for_edit=True) as export_store: + with store.DirectoryModelExportStore(tmp_path, app=app, for_edit=True) as export_store: export_store.add_dataset_collection(hc1) # Fabric editing metadata for collection... - collections_metadata_path = os.path.join(temp_directory, store.ATTRS_FILENAME_COLLECTIONS) - datasets_metadata_path = os.path.join(temp_directory, store.ATTRS_FILENAME_DATASETS) + collections_metadata_path = os.path.join(tmp_path, store.ATTRS_FILENAME_COLLECTIONS) + datasets_metadata_path = os.path.join(tmp_path, store.ATTRS_FILENAME_DATASETS) with open(collections_metadata_path) as f: hdcas_metadata = json.load(f) @@ -798,14 +791,14 @@ def test_import_export_edit_collection(): with open(collections_metadata_path, "w") as collections_f: json.dump(hdcas_metadata, collections_f) - _perform_import_from_directory(temp_directory, app, u, import_history, store.ImportOptions(allow_edit=True)) + _perform_import_from_directory(tmp_path, app, u, import_history, store.ImportOptions(allow_edit=True)) sa_session.refresh(c1) assert c1.populated_state == model.DatasetCollection.populated_states.OK, c1.populated_state assert len(c1.elements) == 2 -def test_import_export_composite_datasets(): +def test_import_export_composite_datasets(tmp_path): app = _mock_app() sa_session = app.model.context @@ -819,13 +812,12 @@ def test_import_export_composite_datasets(): app.write_primary_file(d1, "cool primary file") app.write_composite_file(d1, "cool composite file", "child_file") - temp_directory = mkdtemp() - with store.DirectoryModelExportStore(temp_directory, app=app, export_files="copy") as export_store: + with store.DirectoryModelExportStore(tmp_path, app=app, export_files="copy") as export_store: export_store.add_dataset(d1) import_history = model.History(name="Test History for Import", user=u) app.add_and_commit(import_history) - _perform_import_from_directory(temp_directory, app, u, import_history) + _perform_import_from_directory(tmp_path, app, u, import_history) assert len(import_history.datasets) == 1 import_dataset = import_history.datasets[0] _assert_extra_files_has_parent_directory_with_single_file_containing( @@ -848,7 +840,7 @@ def _assert_extra_files_has_parent_directory_with_single_file_containing( assert contents == expected_contents -def test_edit_metadata_files(): +def test_edit_metadata_files(tmp_path): app = _mock_app(store_by="uuid") sa_session = app.model.context @@ -864,15 +856,12 @@ def test_edit_metadata_files(): assert d1.metadata.bam_index assert isinstance(d1.metadata.bam_index, model.MetadataFile) - temp_directory = mkdtemp() - with store.DirectoryModelExportStore( - temp_directory, app=app, for_edit=True, strip_metadata_files=False - ) as export_store: + with store.DirectoryModelExportStore(tmp_path, app=app, for_edit=True, strip_metadata_files=False) as export_store: export_store.add_dataset(d1) import_history = model.History(name="Test History for Import", user=u) app.add_and_commit(import_history) - _perform_import_from_directory(temp_directory, app, u, import_history, store.ImportOptions(allow_edit=True)) + _perform_import_from_directory(tmp_path, app, u, import_history, store.ImportOptions(allow_edit=True)) def test_sessionless_import_edit_datasets(): From dad7c26a83d69b987cf6ac55099e2fef31989967 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Fri, 10 Jan 2025 12:28:51 +0000 Subject: [PATCH 2/2] Use `id` entity attribute when setting `exampleOfWork` property Fix the following error in 3 unit tests when using rocrate 0.13.0 : ``` ______________________ test_export_invocation_to_ro_crate ______________________ tmp_path = PosixPath('/tmp/pytest-of-runner/pytest-5/test_export_invocation_to_ro_c0') def test_export_invocation_to_ro_crate(tmp_path): app = _mock_app() workflow_invocation = _setup_invocation(app) crate_directory = tmp_path / "crate" with store.ROCrateModelExportStore(crate_directory, app=app) as export_store: export_store.export_workflow_invocation(workflow_invocation) > validate_invocation_crate_directory(crate_directory) tests/data/model/test_model_store.py:627: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ tests/data/model/test_model_store.py:564: in validate_invocation_crate_directory validate_create_action(crate) _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ ro_crate = def validate_create_action(ro_crate: ROCrate): workflow = ro_crate.mainEntity actions = [_ for _ in ro_crate.contextual_entities if "CreateAction" in _.type] assert len(actions) == 1 wf_action = actions[0] assert wf_action["instrument"] assert wf_action["instrument"] is workflow wf_objects = wf_action["object"] wf_results = wf_action["result"] assert len(wf_objects) == 1 assert len(wf_results) == 1 for entity in wf_results: if entity.id.endswith(".txt"): assert "File" in entity.type wf_output_file = entity assert wf_output_file["encodingFormat"] == "text/plain" > assert wf_output_file["exampleOfWork"] is workflow["output"][0] E AssertionError: assert '#d2ed04ef-36f8-49ae-86ce-47cb03a856c0' is tests/data/model/test_model_store.py:540: AssertionError ``` Also: - Remove unused `_add_step_tool_pv` method. --- lib/galaxy/model/store/ro_crate_utils.py | 60 +++++++++--------------- 1 file changed, 22 insertions(+), 38 deletions(-) diff --git a/lib/galaxy/model/store/ro_crate_utils.py b/lib/galaxy/model/store/ro_crate_utils.py index b6b583a57a77..33072d27baa1 100644 --- a/lib/galaxy/model/store/ro_crate_utils.py +++ b/lib/galaxy/model/store/ro_crate_utils.py @@ -123,25 +123,27 @@ def _add_file(self, dataset: HistoryDatasetAssociation, properties: Dict[Any, An def _add_files(self, crate: ROCrate): for wfda in self.invocation.input_datasets: if not self.file_entities.get(wfda.dataset.dataset.id): + dataset_formal_param = self._add_dataset_formal_parameter(wfda.dataset, crate) + crate.mainEntity.append_to("input", dataset_formal_param) properties = { - "exampleOfWork": {"@id": f"#{wfda.dataset.dataset.uuid}"}, + "exampleOfWork": {"@id": dataset_formal_param.id}, } file_entity = self._add_file(wfda.dataset, properties, crate) - dataset_formal_param = self._add_dataset_formal_parameter(wfda.dataset, crate) - crate.mainEntity.append_to("input", dataset_formal_param) self.create_action.append_to("object", file_entity) for wfda in self.invocation.output_datasets: if not self.file_entities.get(wfda.dataset.dataset.id): + dataset_formal_param = self._add_dataset_formal_parameter(wfda.dataset, crate) + crate.mainEntity.append_to("output", dataset_formal_param) properties = { - "exampleOfWork": {"@id": f"#{wfda.dataset.dataset.uuid}"}, + "exampleOfWork": {"@id": dataset_formal_param.id}, } file_entity = self._add_file(wfda.dataset, properties, crate) - dataset_formal_param = self._add_dataset_formal_parameter(wfda.dataset, crate) - crate.mainEntity.append_to("output", dataset_formal_param) self.create_action.append_to("result", file_entity) - def _add_collection(self, hdca: HistoryDatasetCollectionAssociation, crate: ROCrate) -> ContextEntity: + def _add_collection( + self, hdca: HistoryDatasetCollectionAssociation, crate: ROCrate, collection_formal_param: ContextEntity + ) -> ContextEntity: name = hdca.name dataset_ids = [] for hda in hdca.dataset_instances: @@ -158,7 +160,7 @@ def _add_collection(self, hdca: HistoryDatasetCollectionAssociation, crate: ROCr "@type": "Collection", "additionalType": self._get_collection_additional_type(hdca.collection.collection_type), "hasPart": dataset_ids, - "exampleOfWork": {"@id": f"#{hdca.type_id}-param"}, + "exampleOfWork": {"@id": collection_formal_param.id}, } collection_entity = crate.add( ContextEntity( @@ -185,14 +187,14 @@ def _get_parameter_additional_type(self, parameter_type: Optional[str]) -> str: def _add_collections(self, crate: ROCrate): for wfdca in self.invocation.input_dataset_collections: - collection_entity = self._add_collection(wfdca.dataset_collection, crate) collection_formal_param = self._add_collection_formal_parameter(wfdca.dataset_collection, crate) + collection_entity = self._add_collection(wfdca.dataset_collection, crate, collection_formal_param) crate.mainEntity.append_to("input", collection_formal_param) self.create_action.append_to("object", collection_entity) for wfdca in self.invocation.output_dataset_collections: - collection_entity = self._add_collection(wfdca.dataset_collection, crate) collection_formal_param = self._add_collection_formal_parameter(wfdca.dataset_collection, crate) + collection_entity = self._add_collection(wfdca.dataset_collection, crate, collection_formal_param) crate.mainEntity.append_to("output", collection_formal_param) self.create_action.append_to("result", collection_entity) @@ -336,31 +338,14 @@ def _add_profiles(self, crate: ROCrate): def _add_parameters(self, crate: ROCrate): for step in self.invocation.steps: if step.workflow_step.type == "parameter_input": - property_value = self._add_step_parameter_pv(step, crate) - formal_param = self._add_step_parameter_fp(step, crate) - crate.mainEntity.append_to("input", formal_param) + property_value = self._add_step_parameter(step, crate) self.create_action.append_to("object", property_value) - def _add_step_parameter_pv(self, step: WorkflowInvocationStep, crate: ROCrate): - param_id = step.workflow_step.label - return crate.add( - ContextEntity( - crate, - f"{param_id}-pv", - properties={ - "@type": "PropertyValue", - "name": f"{param_id}", - "value": step.output_value.value, - "exampleOfWork": {"@id": f"#{param_id}-param"}, - }, - ) - ) - - def _add_step_parameter_fp(self, step: WorkflowInvocationStep, crate: ROCrate): + def _add_step_parameter(self, step: WorkflowInvocationStep, crate: ROCrate) -> ContextEntity: param_id = step.workflow_step.label assert step.workflow_step.tool_inputs param_type = step.workflow_step.tool_inputs["parameter_type"] - return crate.add( + formal_param = crate.add( ContextEntity( crate, f"{param_id}-param", @@ -373,19 +358,16 @@ def _add_step_parameter_fp(self, step: WorkflowInvocationStep, crate: ROCrate): }, ) ) - - def _add_step_tool_pv(self, step: WorkflowInvocationStep, tool_input: str, crate: ROCrate): - param_id = tool_input - assert step.workflow_step.tool_inputs + crate.mainEntity.append_to("input", formal_param) return crate.add( ContextEntity( crate, f"{param_id}-pv", properties={ "@type": "PropertyValue", - "name": f"{step.workflow_step.label}", - "value": step.workflow_step.tool_inputs[tool_input], - "exampleOfWork": {"@id": f"#{param_id}-param"}, + "name": f"{param_id}", + "value": step.output_value.value, + "exampleOfWork": {"@id": formal_param.id}, }, ) ) @@ -419,7 +401,9 @@ def _add_dataset_formal_parameter(self, hda: HistoryDatasetAssociation, crate: R ) ) - def _add_collection_formal_parameter(self, hdca: HistoryDatasetCollectionAssociation, crate: ROCrate): + def _add_collection_formal_parameter( + self, hdca: HistoryDatasetCollectionAssociation, crate: ROCrate + ) -> ContextEntity: return crate.add( ContextEntity( crate,