From 23714bc9f85c55328c9dec3feffc7fa6e785dee4 Mon Sep 17 00:00:00 2001 From: bmccandless Date: Fri, 6 Nov 2020 18:04:14 -0800 Subject: [PATCH] Fix bug that occurs when all categories are removed. (#1974) Previously if the user remove all annotations, the code would still generate a tiledb uri in the write_labels call, and add that to the database. A tiledb array would not be written in this case. When the read_labels was then called, it would find the entry in the database, attempt to open the tiledb array, then fail. The patch here will set the tiledb_uri to the empty string if all categories are removed. When read_labels is called, it will see the empty uri and return None. Furthermore, if the database does have a tiledb_uri that does not exist, or cannot be read, then the code will now log a warning, and return None (instead of throwing an exception, which results in a server error). #1932 --- server/common/annotations/hosted_tiledb.py | 24 +++++++---- .../unit/common/test_writable_annotation.py | 42 ++++++++++++------- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/server/common/annotations/hosted_tiledb.py b/server/common/annotations/hosted_tiledb.py index 235f8a16..4922400d 100644 --- a/server/common/annotations/hosted_tiledb.py +++ b/server/common/annotations/hosted_tiledb.py @@ -64,7 +64,15 @@ class AnnotationsHostedTileDB(Annotations): Annotation, [Annotation.user_id == user_id, Annotation.dataset_id == dataset_id] ) if annotation_object: - df = tiledb.open(annotation_object.tiledb_uri) + if annotation_object.tiledb_uri == "": + # this mean the user has removed all the categories. + return None + try: + df = tiledb.open(annotation_object.tiledb_uri) + except tiledb.TileDBError: + # don't crash if the annotations file is missing or can't be read. + current_app.logger.warning(f"Cannot read annotation file: {annotation_object.tiledb_uri}") + return None pandas_df = self.convert_to_pandas_df(df, annotation_object.schema_hints) return pandas_df else: @@ -130,12 +138,6 @@ class AnnotationsHostedTileDB(Annotations): else: os.makedirs(uri, exist_ok=True) _, dataframe_schema_type_hints = get_dtypes_and_schemas_of_dataframe(df) - annotation = Annotation( - tiledb_uri=uri, - user_id=user_id, - dataset_id=str(dataset_id), - schema_hints=json.dumps(dataframe_schema_type_hints), - ) if not df.empty: self.check_category_names(df) # convert to tiledb datatypes @@ -143,7 +145,15 @@ class AnnotationsHostedTileDB(Annotations): for col in df: df[col] = df[col].astype(get_dtype_of_array(df[col])) tiledb.from_pandas(uri, df) + else: + uri = "" + annotation = Annotation( + tiledb_uri=uri, + user_id=user_id, + dataset_id=str(dataset_id), + schema_hints=json.dumps(dataframe_schema_type_hints), + ) self.db.session.add(annotation) self.db.session.commit() diff --git a/server/test/unit/common/test_writable_annotation.py b/server/test/unit/common/test_writable_annotation.py index fda910aa..e4e47ac1 100644 --- a/server/test/unit/common/test_writable_annotation.py +++ b/server/test/unit/common/test_writable_annotation.py @@ -2,7 +2,7 @@ import json import shutil import unittest from os import path, listdir -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock import numpy as np import pandas as pd @@ -129,20 +129,34 @@ class WritableTileDBStoredAnnotationTest(unittest.TestCase): with self.assertRaises(KeyError): self.annotation_put_fbs(fbs_bad) - @patch("server.common.annotations.hosted_tiledb.AnnotationsHostedTileDB.get_user_id") - @patch("server.common.annotations.hosted_tiledb.AnnotationsHostedTileDB.get_user_name") - def test_write_labels_stores_df_as_tiledb_array(self, mock_user_name, mock_user_id): - mock_user_id.return_value = "1234" - mock_user_name.return_value = "user1234" - self.annotations.write_labels(self.df, self.data) - # get uri - dataset_id = self.db.query([CellxGeneDataset], [CellxGeneDataset.name == self.data.get_location()])[0].id - annotation = self.db.query_for_most_recent( - Annotation, [Annotation.user_id == "1234", Annotation.dataset_id == str(dataset_id)] - ) + def test_write_labels_stores_df_as_tiledb_array(self): + with self.app.test_request_context(): + self.annotations.write_labels(self.df, self.data) + # get uri + dataset_id = self.db.query([CellxGeneDataset], [CellxGeneDataset.name == self.data.get_location()])[0].id + annotation = self.db.query_for_most_recent( + Annotation, [Annotation.user_id == "1234", Annotation.dataset_id == str(dataset_id)] + ) - df = tiledb.open(annotation.tiledb_uri) - self.assertEqual(type(df), tiledb.array.SparseArray) + df = tiledb.open(annotation.tiledb_uri) + self.assertEqual(type(df), tiledb.array.SparseArray) + + def test_remove_categories(self): + with self.app.test_request_context(): + # update empty category data, which is how annotations are removed + empty = make_fbs({}) + self.annotation_put_fbs(empty) + + # verify that the tiledb uri is an empty string. + dataset_id = self.db.query([CellxGeneDataset], [CellxGeneDataset.name == self.data.get_location()])[0].id + annotation = self.db.query_for_most_recent( + Annotation, [Annotation.user_id == self.user_id, Annotation.dataset_id == str(dataset_id)] + ) + self.assertEqual(annotation.tiledb_uri, "") + + # verify that read_labels returns None + df = self.annotations.read_labels(self.data) + self.assertIsNone(df) class WritableAnnotationTest(unittest.TestCase):