Merge pull request #1254 from Sage-Bionetworks/develop-filename-annotations-FDS-654

BugFix: Fixed an issue where files on synapse were not being annotated correctly

Author: GiaJordan

schematic/models/commands.py

@@ -110,9 +110,9 @@ def submit_manifest(
     """
     Running CLI with manifest validation (optional) and submission options.
     """
-    if jsonld is None:
-        jsonld =  CONFIG.model_location
-        log_value_from_config("jsonld", jsonld)
+    
+    jsonld =  CONFIG.model_location
+    log_value_from_config("jsonld", jsonld)
 
     metadata_model = MetadataModel(
         inputMModelLocation=jsonld, inputMModelLocationType="local"

schematic/store/synapse.py

@@ -574,13 +574,7 @@ def updateDatasetManifestFiles(self, sg: SchemaGenerator, datasetId: str, store:
         # the columns Filename and entityId are assumed to be present in manifest schema
         # TODO: use idiomatic panda syntax
         if dataset_files:
-            new_files = {"Filename": [], "entityId": []}
-
-            # find new files if any
-            for file_id, file_name in dataset_files:
-                if not file_id in manifest["entityId"].values:
-                    new_files["Filename"].append(file_name)
-                    new_files["entityId"].append(file_id)
+            new_files = self._get_file_entityIds(dataset_files=dataset_files, only_new_files=True, manifest=manifest)
 
             # update manifest so that it contain new files
             new_files = pd.DataFrame(new_files)
@@ -600,6 +594,38 @@ def updateDatasetManifestFiles(self, sg: SchemaGenerator, datasetId: str, store:
         manifest = manifest.fillna("") 
 
         return manifest_id, manifest
+    
+    def _get_file_entityIds(self, dataset_files: List,  only_new_files: bool = False, manifest: pd.DataFrame = None):
+        """
+        Get a dictionary of files in a dataset. Either files that are not in the current manifest or all files
+        
+        Args:
+            manifest: metadata manifest
+            dataset_file: List of all files in a dataset
+            only_new_files: boolean to control whether only new files are returned or all files in the dataset
+        Returns:
+            files: dictionary of file names and entityIDs, with scope as specified by `only_new_files`
+        """
+        files = {"Filename": [], "entityId": []}
+
+        if only_new_files:
+            if manifest is None:
+                raise UnboundLocalError(
+                    "No manifest was passed in, a manifest is required when `only_new_files` is True."
+                )
+            
+            # find new files (that are not in the current manifest) if any
+            for file_id, file_name in dataset_files:
+                if not file_id in manifest["entityId"].values:
+                    files["Filename"].append(file_name)
+                    files["entityId"].append(file_id)
+        else:
+            # get all files
+            for file_id, file_name in dataset_files:
+                files["Filename"].append(file_name)
+                files["entityId"].append(file_id)
+
+        return files
 
     def getProjectManifests(self, projectId: str) -> List[str]:
         """Gets all metadata manifest files across all datasets in a specified project.
@@ -1343,6 +1369,18 @@ def add_entities(
             manifest (pd.DataFrame): modified to add entitiyId as appropriate.
 
         '''
+
+        # Expected behavior is to annotate files if `Filename` is present regardless of `-mrt` setting
+        if 'filename' in [col.lower() for col in manifest.columns]:
+            # get current list of files and store as dataframe
+            dataset_files = self.getFilesInStorageDataset(datasetId)
+            files_and_entityIds = self._get_file_entityIds(dataset_files=dataset_files, only_new_files=False)
+            file_df = pd.DataFrame(files_and_entityIds)
+            
+            # Merge dataframes to add entityIds
+            manifest = manifest.merge(file_df, how = 'left', on='Filename', suffixes=['_x',None]).drop('entityId_x',axis=1)
+
+        # Fill `entityId` for each row if missing and annotate entity as appropriate
         for idx, row in manifest.iterrows():
             if not row["entityId"] and (manifest_record_type == 'file_and_entities' or 
                 manifest_record_type == 'table_file_and_entities'):

tests/data/mock_manifests/annotations_test_manifest.csv

@@ -1,3 +1,3 @@
-Component,CheckList,CheckRegexList,CheckRegexSingle,CheckNum,CheckFloat,CheckInt,CheckString,CheckURL,CheckMatchatLeast,CheckMatchatLeastvalues,CheckMatchExactly,CheckMatchExactlyvalues,CheckRecommended,CheckAges,CheckUnique,Uuid,entityId
+Component,CheckList,CheckRegexList,CheckRegexSingle,CheckNum,CheckFloat,CheckInt,CheckString,CheckURL,CheckMatchatLeast,CheckMatchatLeastvalues,CheckMatchExactly,CheckMatchExactlyvalues,CheckRecommended,CheckAges,CheckUnique,Id,entityId
 MockComponent,"valid,list,values","a,c,f",a,6,99.65,7,valid,https://www.google.com/,1985,4891,23487492,24323472834,,6571,str1,0f7812cc-8a0e-4f54-b8c4-e497cb7b34d0,syn35367245
 MockComponent,"valid,list,values","a,c,f",a,6,99.65,8.52,valid,https://www.google.com/,1985,4891,23487492,24323472834,,6571,str1,da82f8e2-c7b0-428f-8f9d-677252ef5f68,syn35367246

tests/data/mock_manifests/test_BulkRNAseq.csv

@@ -0,0 +1,3 @@
+Filename,Sample ID,File Format,Component,Genome Build,Genome FASTA
+TestRNA-seqDataset1/TestRNA-seq-dummy-dataset.rtf,ABCD,BAM,BulkRNA-seqAssay,GRCh38,
+TestRNA-seqDataset1/TestRNA-seq-dummy-dataset2.rtf,EFGH,CRAM,BulkRNA-seqAssay,GRCm39,

tests/test_store.py

@@ -14,6 +14,7 @@
 from schematic.store.synapse import SynapseStorage, DatasetFileView, ManifestDownload
 from schematic.schemas.generator import SchemaGenerator
 from synapseclient.core.exceptions import SynapseHTTPError
+from synapseclient.entity import File
 from schematic.configuration.configuration import Configuration
 
 logging.basicConfig(level=logging.DEBUG)
@@ -113,9 +114,37 @@ def test_getFileAnnotations(self, synapse_store):
 
         assert expected_dict == actual_dict
 
-    def test_annotation_submission(self, synapse_store, helpers, config: Configuration):
-        manifest_path = "mock_manifests/annotations_test_manifest.csv"
-
+    @pytest.mark.parametrize('only_new_files',[True, False])
+    def test_get_file_entityIds(self, helpers, synapse_store, only_new_files):
+        manifest_path = "mock_manifests/test_BulkRNAseq.csv"
+        dataset_files = synapse_store.getFilesInStorageDataset('syn39241199')
+
+        if only_new_files:
+            # Prepare manifest is getting Ids for new files only
+            manifest = helpers.get_data_frame(manifest_path)
+            entityIds = pd.DataFrame({'entityId': ['syn39242580', 'syn51900502']})
+            manifest = manifest.join(entityIds)
+            
+            # get entityIds for new files
+            files_and_Ids = synapse_store._get_file_entityIds(dataset_files=dataset_files, only_new_files=only_new_files, manifest=manifest)
+
+            # Assert that there are no new files
+            for value in files_and_Ids.values():
+                assert value == []
+            
+        else:
+            # get entityIds for all files
+            files_and_Ids = synapse_store._get_file_entityIds(dataset_files=dataset_files, only_new_files=only_new_files)
+
+            # assert that the correct number of files were found
+            assert len(files_and_Ids['entityId']) == 2
+
+    @pytest.mark.parametrize('manifest_path, test_annotations, datasetId, manifest_record_type',
+                             [  ("mock_manifests/annotations_test_manifest.csv", {'CheckInt': '7', 'CheckList': 'valid, list, values'}, 'syn34295552', 'file_and_entities'),
+                                ("mock_manifests/test_BulkRNAseq.csv", {'FileFormat': 'BAM', 'GenomeBuild': 'GRCh38'}, 'syn39241199', 'table_and_file')],
+                            ids = ['non file-based',
+                                    'file-based'])
+    def test_annotation_submission(self, synapse_store, helpers, manifest_path, test_annotations, datasetId, manifest_record_type, config: Configuration):
         # Upload dataset annotations
         sg = SchemaGenerator(config.model_location)
 
@@ -129,8 +158,8 @@ def test_annotation_submission(self, synapse_store, helpers, config: Configurati
                     manifest_id = synapse_store.associateMetadataWithFiles(
                         schemaGenerator = sg,
                         metadataManifestPath = helpers.get_data_path(manifest_path),
-                        datasetId = 'syn34295552',
-                        manifest_record_type = 'file_and_entities',
+                        datasetId = datasetId,
+                        manifest_record_type = manifest_record_type,
                         useSchemaLabel = True,
                         hideBlanks = True,
                         restrict_manifest = False,
@@ -139,17 +168,19 @@ def test_annotation_submission(self, synapse_store, helpers, config: Configurati
             pass
 
         # Retrive annotations
-        entity_id, entity_id_spare = helpers.get_data_frame(manifest_path)["entityId"][0:2]
+        entity_id = helpers.get_data_frame(manifest_path)["entityId"][0]
         annotations = synapse_store.getFileAnnotations(entity_id)
 
         # Check annotations of interest
-        assert annotations['CheckInt'] == '7'
-        assert annotations['CheckList'] == 'valid, list, values'
-        assert 'CheckRecommended' not in annotations.keys()
-
-
-
+        for key in test_annotations.keys():
+            assert key in annotations.keys()
+            assert annotations[key] == test_annotations[key]
 
+        if manifest_path.endswith('annotations_test_manifest.csv'):
+            assert 'CheckRecommended' not in annotations.keys()
+        elif manifest_path.endswith('test_BulkRNAseq.csv'):
+            entity = synapse_store.syn.get(entity_id)
+            assert type(entity) == File
 
     @pytest.mark.parametrize("force_batch", [True, False], ids=["batch", "non_batch"])
     def test_getDatasetAnnotations(self, dataset_id, synapse_store, force_batch):
@@ -466,7 +497,6 @@ def test_upsertTable(self, helpers, synapse_store, config:Configuration, project
         # delete table        
         synapse_store.syn.delete(tableId)
 
-
 class TestDownloadManifest:
     @pytest.mark.parametrize("datasetFileView", [{"id": ["syn51203973", "syn51203943"], "name": ["synapse_storage_manifest.csv", "synapse_storage_manifest_censored.csv"]}, {"id": ["syn51203973"], "name": ["synapse_storage_manifest.csv"]}, {"id": ["syn51203943"], "name": ["synapse_storage_manifest_censored.csv"]}])
     def test_get_manifest_id(self, synapse_store, datasetFileView):