Merge pull request #1292 from Sage-Bionetworks/develop

v23.9.2

Author: andrewelamb

pyproject.toml

@@ -148,5 +148,13 @@ markers = [
     """\
     rule_benchmark: marks tests covering \
     validation rule benchmarking
-    """    
+    """,
+    """\
+    synapse_credentials_needed: marks api tests that \
+    require synapse credentials to run
+    """,
+    """\
+    empty_token: marks api tests that \
+    send empty credentials in the request
+    """
 ]

schematic/manifest/generator.py

@@ -1554,6 +1554,16 @@ def get_manifest(
                     annotations = store.getDatasetAnnotations(dataset_id)
                     # Update `additional_metadata` and generate manifest
                     manifest_url, manifest_df = self.get_manifest_with_annotations(annotations,strict=strict)
+                
+                # If the annotations are empty, 
+                # ie if there are no annotations to pull or annotations were unable to be pulled because the metadata is not file based, 
+                # then create manifest from an empty manifest
+                if annotations.empty:
+                    empty_manifest_df = self.get_dataframe_by_url(empty_manifest_url)
+                    manifest_df = empty_manifest_df
+
+                    logger.warning(f"Annotations were not able to be gathered for the given parameters. This manifest will be generated from an empty manifest.")
+                    
             else:
                 empty_manifest_df = self.get_dataframe_by_url(empty_manifest_url)
                 if self.is_file_based:

schematic/schemas/explorer.py

@@ -184,7 +184,7 @@ def get_adjacent_nodes_by_relationship(
         return list(nodes)
 
     def is_class_in_schema(self, class_label):
-        if self.schema_nx.nodes[class_label]:
+        if class_label in self.schema_nx.nodes():
             return True
         else:
             return False

schematic/store/synapse.py

@@ -1412,7 +1412,7 @@ def _create_entity_id(self, idx, row, manifest, datasetId):
         manifest.loc[idx, "entityId"] = entityId
         return manifest, entityId
 
-    def add_entities(
+    def add_annotations_to_entities_files(
                     self,
                     se,
                     schemaGenerator,
@@ -1457,12 +1457,13 @@ def add_entities(
                 manifest.loc[idx, "entityId"] = manifest_synapse_table_id
                 entityId = ''
             else:
-                # get the entity id corresponding to this row
+                # get the file id of the file to annotate, collected in above step.
                 entityId = row["entityId"]
 
             # Adding annotations to connected files.
             if entityId:
                 self._add_annotations(se, schemaGenerator, row, entityId, hideBlanks)
+                logger.info(f"Added annotations to entity: {entityId}")
         return manifest
 
     def upload_manifest_as_table(
@@ -1506,7 +1507,7 @@ def upload_manifest_as_table(
                                                     useSchemaLabel,
                                                     table_manipulation)
 
-        manifest = self.add_entities(se, schemaGenerator, manifest, manifest_record_type, datasetId, hideBlanks, manifest_synapse_table_id)
+        manifest = self.add_annotations_to_entities_files(se, schemaGenerator, manifest, manifest_record_type, datasetId, hideBlanks, manifest_synapse_table_id)
         # Load manifest to synapse as a CSV File
         manifest_synapse_file_id = self.upload_manifest_file(manifest, metadataManifestPath, datasetId, restrict, component_name = component_name)
 
@@ -1540,8 +1541,7 @@ def upload_manifest_as_csv(
                             restrict,
                             manifest_record_type,
                             hideBlanks,
-                            component_name,
-                            with_entities = False,):
+                            component_name):
         """Upload manifest to Synapse as a csv only.
         Args:
             se: SchemaExplorer object
@@ -1557,8 +1557,8 @@ def upload_manifest_as_csv(
         Return:
             manifest_synapse_file_id (str): SynID of manifest csv uploaded to synapse.
         """
-        if with_entities:
-            manifest = self.add_entities(se, schemaGenerator, manifest, manifest_record_type, datasetId, hideBlanks)
+        # remove with_entities parameter and rename add_annotations, as add_annototaions_to_files_entities.
+        manifest = self.add_annotations_to_entities_files(se, schemaGenerator, manifest, manifest_record_type, datasetId, hideBlanks)
 
         # Load manifest to synapse as a CSV File
         manifest_synapse_file_id = self.upload_manifest_file(manifest,
@@ -1613,7 +1613,7 @@ def upload_manifest_combo(
                                                     useSchemaLabel=useSchemaLabel,
                                                     table_manipulation=table_manipulation,)
 
-        manifest = self.add_entities(se, schemaGenerator, manifest, manifest_record_type, datasetId, hideBlanks, manifest_synapse_table_id)
+        manifest = self.add_annotations_to_entities_files(se, schemaGenerator, manifest, manifest_record_type, datasetId, hideBlanks, manifest_synapse_table_id)
 
         # Load manifest to synapse as a CSV File
         manifest_synapse_file_id = self.upload_manifest_file(manifest, metadataManifestPath, datasetId, restrict, component_name)
@@ -1690,7 +1690,6 @@ def associateMetadataWithFiles(
                                         hideBlanks=hideBlanks,
                                         manifest_record_type=manifest_record_type,
                                         component_name = component_name,
-                                        with_entities = False,
                                         )
         elif manifest_record_type == "table_and_file":
             manifest_synapse_file_id = self.upload_manifest_as_table(
@@ -1718,7 +1717,6 @@ def associateMetadataWithFiles(
                                         hideBlanks=hideBlanks,
                                         manifest_record_type=manifest_record_type,
                                         component_name = component_name,
-                                        with_entities=True,
                                         )
         elif manifest_record_type == "table_file_and_entities":
             manifest_synapse_file_id = self.upload_manifest_combo(

tests/test_api.py

@@ -95,6 +95,7 @@ def syn_token(config:Configuration):
 
 @pytest.mark.schematic_api
 class TestSynapseStorage:
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.parametrize("return_type", ["json", "csv"])
     def test_get_storage_assets_tables(self, client, syn_token, return_type):
         params = {
@@ -120,6 +121,8 @@ def test_get_storage_assets_tables(self, client, syn_token, return_type):
             os.remove(response_dt)
         else: 
             pass
+
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.parametrize("full_path", [True, False])
     @pytest.mark.parametrize("file_names", [None, "Sample_A.txt"])
     def test_get_dataset_files(self,full_path, file_names, syn_token, client):
@@ -150,6 +153,7 @@ def test_get_dataset_files(self,full_path, file_names, syn_token, client):
             else: 
                 assert ["syn25705259","Boolean Test"] and ["syn23667202","DataTypeX_table"] in response_dt
 
+    @pytest.mark.synapse_credentials_needed
     def test_get_storage_project_dataset(self, syn_token, client):
         params = {
         "access_token": syn_token,
@@ -162,6 +166,7 @@ def test_get_storage_project_dataset(self, syn_token, client):
         response_dt = json.loads(response.data)
         assert ["syn26251193","Issue522"] in response_dt
 
+    @pytest.mark.synapse_credentials_needed
     def test_get_storage_project_manifests(self, syn_token, client):
 
         params = {
@@ -174,6 +179,7 @@ def test_get_storage_project_manifests(self, syn_token, client):
 
         assert response.status_code == 200
 
+    @pytest.mark.synapse_credentials_needed
     def test_get_storage_projects(self, syn_token, client):
 
         params = {
@@ -185,6 +191,7 @@ def test_get_storage_projects(self, syn_token, client):
 
         assert response.status_code == 200
 
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.parametrize("entity_id", ["syn34640850", "syn23643253", "syn24992754"])
     def test_get_entity_type(self, syn_token, client, entity_id):
         params = {
@@ -203,6 +210,7 @@ def test_get_entity_type(self, syn_token, client, entity_id):
         elif entity_id == "syn24992754":
             assert response_dt == "project"
 
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.parametrize("entity_id", ["syn30988314", "syn27221721"])
     def test_if_in_assetview(self, syn_token, client, entity_id):
         params = {
@@ -399,6 +407,7 @@ def ifPandasDataframe(self, response_dt):
             assert isinstance(df, pd.DataFrame)
 
 
+    @pytest.mark.empty_token
     #@pytest.mark.parametrize("output_format", [None, "excel", "google_sheet", "dataframe (only if getting existing manifests)"])
     @pytest.mark.parametrize("output_format", ["excel"])
     @pytest.mark.parametrize("data_type", ["Biospecimen", "Patient", "all manifests", ["Biospecimen", "Patient"]])
@@ -455,6 +464,7 @@ def test_generate_existing_manifest(self, client, data_model_jsonld, data_type,
             self.ifGoogleSheetExists(response_dt)
 
 
+    @pytest.mark.empty_token
     @pytest.mark.parametrize("output_format", ["excel", "google_sheet", "dataframe (only if getting existing manifests)", None])
     @pytest.mark.parametrize("data_type", ["all manifests", ["Biospecimen", "Patient"], "Patient"])
     def test_generate_new_manifest(self, caplog, client, data_model_jsonld, data_type, output_format):
@@ -621,6 +631,7 @@ def test_validate_manifest(self, data_model_jsonld, client, json_str, restrict_r
         assert "errors" in response_dt.keys()
         assert "warnings" in response_dt.keys()
 
+    @pytest.mark.synapse_credentials_needed
     def test_get_datatype_manifest(self, client, syn_token):
         params = {
             "access_token": syn_token,
@@ -642,6 +653,7 @@ def test_get_datatype_manifest(self, client, syn_token):
                 "Year of Birth": "Int64",
                 "entityId": "string"}
 
+    @pytest.mark.synapse_credentials_needed
     # small manifest: syn51078535; big manifest: syn51156998
     @pytest.mark.parametrize("manifest_id, expected_component, expected_file_name", [("syn51078535", "BulkRNA-seqAssay", "synapse_storage_manifest.csv"), ("syn51156998", "Biospecimen", "synapse_storage_manifest_biospecimen.csv")])
     @pytest.mark.parametrize("new_manifest_name",[None,"Example.csv"]) 
@@ -694,6 +706,8 @@ def test_manifest_download(self, config: Configuration, client, syn_token, manif
             os.remove(manifest_file_path)
         except: 
             pass
+
+    @pytest.mark.synapse_credentials_needed        
     # test downloading a manifest with access restriction and see if the correct error message got raised
     def test_download_access_restricted_manifest(self, client, syn_token):
         params = {
@@ -707,6 +721,7 @@ def test_download_access_restricted_manifest(self, client, syn_token):
             raise TypeError('the type error got raised')
         assert exc_info.value.args[0] == "the type error got raised"
 
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.parametrize("as_json", [None, True, False])
     @pytest.mark.parametrize("new_manifest_name", [None, "Test"])
     def test_dataset_manifest_download(self, client, as_json, syn_token, new_manifest_name):
@@ -735,6 +750,7 @@ def test_dataset_manifest_download(self, client, as_json, syn_token, new_manifes
             assert isinstance(response_path, str)
             assert response_path.endswith(".csv")
 
+    @pytest.mark.synapse_credentials_needed    
     @pytest.mark.submission
     def test_submit_manifest_table_and_file_replace(self, client, syn_token, data_model_jsonld, test_manifest_submit):
         """Testing submit manifest in a csv format as a table and a file. Only replace the table
@@ -755,6 +771,7 @@ def test_submit_manifest_table_and_file_replace(self, client, syn_token, data_mo
         response_csv = client.post('http://localhost:3001/v1/model/submit', query_string=params, data={"file_name": (open(test_manifest_submit, 'rb'), "test.csv")})
         assert response_csv.status_code == 200
 
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.submission
     def test_submit_manifest_file_only_replace(self, client, syn_token, data_model_jsonld, test_manifest_submit):
         """Testing submit manifest in a csv format as a file
@@ -773,6 +790,7 @@ def test_submit_manifest_file_only_replace(self, client, syn_token, data_model_j
         response_csv = client.post('http://localhost:3001/v1/model/submit', query_string=params, data={"file_name": (open(test_manifest_submit, 'rb'), "test.csv")})
         assert response_csv.status_code == 200 
 
+    @pytest.mark.synapse_credentials_needed    
     @pytest.mark.submission
     def test_submit_manifest_json_str_replace(self, client, syn_token, data_model_jsonld):
         """Submit json str as a file
@@ -794,6 +812,7 @@ def test_submit_manifest_json_str_replace(self, client, syn_token, data_model_js
         response = client.post('http://localhost:3001/v1/model/submit', query_string = params, data={"file_name":''})
         assert response.status_code == 200
 
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.submission
     def test_submit_manifest_w_file_and_entities(self, client, syn_token, data_model_jsonld, test_manifest_submit):
         params = {
@@ -812,6 +831,7 @@ def test_submit_manifest_w_file_and_entities(self, client, syn_token, data_model
         response_csv = client.post('http://localhost:3001/v1/model/submit', query_string=params, data={"file_name": (open(test_manifest_submit, 'rb'), "test.csv")})
         assert response_csv.status_code == 200
 
+    @pytest.mark.synapse_credentials_needed
     @pytest.mark.submission
     def test_submit_manifest_table_and_file_upsert(self, client, syn_token, data_model_jsonld, test_upsert_manifest_csv, ):
         params = {

tests/test_manifest.py

@@ -193,6 +193,37 @@ def test_get_manifest_excel(self, helpers, sheet_url, output_format, dataset_id)
         if type(manifest) is str and os.path.exists(manifest): 
             os.remove(manifest)
 
+    @pytest.mark.parametrize("dataset_id", [("syn27600056"), ("syn52397659")], ids=["Annotations present", "Annotations not present"])
+    def test_get_manifest_no_annos(self, helpers, dataset_id):
+        """
+        Test to cover manifest generation under the case where use_annotations is True 
+        but there are no annotations in the dataset
+        """
+
+        # Use a non-file based DataType
+        data_type = "Patient"
+
+        # Instantiate object with use_annotations set to True
+        generator = ManifestGenerator(
+        path_to_json_ld=helpers.get_data_path("example.model.jsonld"),
+        root=data_type,
+        use_annotations=True,
+        )
+
+        # Get manifest as a dataframe
+        manifest = generator.get_manifest(dataset_id = dataset_id, sheet_url = False, output_format = "dataframe")
+
+        # Case where annotations are present in the dataset
+        # manifest should have pulled in the annotations
+        if dataset_id == "syn27600056":
+            assert manifest["Patient ID"].size > 1
+
+        # Case where annotations are not present in the dataset
+        # manifest should be empty (One column will have the component filled so not truly empty)
+        elif dataset_id == "syn52397659":
+            assert manifest["Patient ID"].size == 1
+
+
     # test all the functions used under get_manifest
     def test_create_empty_manifest_spreadsheet(self, simple_manifest_generator):
         '''

tests/test_schemas.py

@@ -6,6 +6,7 @@
 
 from schematic.schemas import df_parser
 from schematic.utils.df_utils import load_df
+from schematic.schemas.generator import SchemaGenerator
 
 logging.basicConfig(level=logging.DEBUG)
 logger = logging.getLogger(__name__)
@@ -46,6 +47,13 @@ def extended_schema_path(helpers, tmp_path):
 
     yield extended_schema_path
 
+@pytest.fixture
+def sg(helpers):
+
+    inputModelLocation = helpers.get_data_path('example.model.jsonld')
+    sg = SchemaGenerator(inputModelLocation)
+
+    yield sg
 
 class TestDfParser:
     def test_get_class(self, helpers):
@@ -268,4 +276,19 @@ def test_get_class_label_from_display_name(self, helpers):
         assert(se_obj.get_class_label_from_display_name("model of manifestation") == "Modelofmanifestation")
         assert(se_obj.get_class_label_from_display_name("model of manifestation", strict_camel_case = True) == "ModelOfManifestation")
         assert(se_obj.get_class_label_from_display_name("model of manifestation") == "Modelofmanifestation")
-        assert(se_obj.get_class_label_from_display_name("model of manifestation", strict_camel_case = True) == "ModelOfManifestation")
+        assert(se_obj.get_class_label_from_display_name("model of manifestation", strict_camel_case = True) == "ModelOfManifestation")
+
+class TestSchemaExplorer:
+    @pytest.mark.parametrize("class_name, expected_in_schema", [("Patient",True), ("ptaient",False), ("Biospecimen",True), ("InvalidComponent",False)])
+    def test_is_class_in_schema(self, sg, class_name, expected_in_schema):
+        """
+        Test to cover checking if a given class is in a schema.
+        `is_class_in_schema` should return `True` if the class is in the schema
+        and `False` if it is not.
+        """
+
+        # Check if class is in schema
+        class_in_schema = sg.se.is_class_in_schema(class_name)
+
+        # Assert value is as expected
+        assert class_in_schema == expected_in_schema