annotations CLI and file UX rework (#1049)

* rename config param label-file

* annotations rework - CLI params, file naming and backups

* lint

* improve cli option error checks

* enable session cookies

* enable session cookies

* add session id

* name annotations file in multi-dataset and multi-user safe manner

* pass data user hash to front-end

* add annotation collection name support to front-end

* add constant for annotation data collection name

* parameterize annotation collection name; make it sticky in the session

* clarify comments

* hard wire a temporary data collection name for testing

* prettier

* test comment

* package command

* set annotations  filename dialog

* name  and hash are visible

* wire up data collection capture

server/app/app.py

@@ -1,4 +1,5 @@
 import os
+import datetime
 
 from flask import Flask
 from flask_caching import Cache
@@ -21,7 +22,10 @@ class Server:
         self.app.json_encoder = Float32JSONEncoder
         self.cache.init_app(self.app)
         Compress(self.app)
-        CORS(self.app)
+        CORS(self.app, supports_credentials=True)
+
+        # enable session data
+        self.app.permanent_session_lifetime = datetime.timedelta(days=50 * 365)
 
         # Config
         SECRET_KEY = os.environ.get("CXG_SECRET_KEY", default="SparkleAndShine")

server/app/driver/driver.py

@@ -36,6 +36,16 @@ class CXGDriver(metaclass=ABCMeta):
             "diffexp_may_be_slow": False
         }
 
+    @abstractmethod
+    def get_config_parameters(self, uid=None):
+        """
+            return a dict of properties that will be used to set the engine-specific
+            "parameters" info for client-side configuration.
+
+            See rest.py /config route for use
+        """
+        pass
+
     @property
     def features(self):
         features = {
@@ -61,7 +71,7 @@ class CXGDriver(metaclass=ABCMeta):
         pass
 
     @abstractmethod
-    def annotation_to_fbs_matrix(self, axis, field=None):
+    def annotation_to_fbs_matrix(self, axis, field=None, uid=None):
         """
         Gets annotation value for each observation
         :param axis: string obs or var
@@ -71,7 +81,7 @@ class CXGDriver(metaclass=ABCMeta):
         pass
 
     @abstractmethod
-    def annotation_put_fbs(self, axis, fbs):
+    def annotation_put_fbs(self, axis, fbs, uid=None):
         """
         Put/save FBS as user-defined labels
         """

server/app/rest_api/rest.py

@@ -1,8 +1,9 @@
 from http import HTTPStatus
 import warnings
-from os.path import basename
+from uuid import uuid4
+import re
 
-from flask import Blueprint, current_app, jsonify, make_response, request
+from flask import Blueprint, current_app, jsonify, make_response, request, session
 from flask_restful import Api, Resource
 from server import __version__ as cellxgene_version
 from anndata import __version__ as anndata_version
@@ -11,6 +12,8 @@ from server.app.util.constants import (
     Axis,
     DiffExpMode,
     JSON_NaN_to_num_warning_msg,
+    CXGUID,
+    CXG_ANNO_COLLECTION
 )
 from server.app.util.errors import (
     FilterError,
@@ -20,23 +23,20 @@ from server.app.util.errors import (
     DisabledFeatureError,
 )
 
-"""
-Sort order for routes
-1. Initialize
-2. Data & Metadata
-3. Computation
-"""
-
 
 class SchemaAPI(Resource):
     def get(self):
+        cxguid = get_userid(session)
+        anno_collection = get_anno_collection(session)
         return make_response(
-            jsonify({"schema": current_app.data.get_schema()}), HTTPStatus.OK
+            jsonify({"schema": current_app.data.get_schema(uid=cxguid, collection=anno_collection)}), HTTPStatus.OK
         )
 
 
 class ConfigAPI(Resource):
     def get(self):
+        cxguid = get_userid(session)
+        anno_collection = get_anno_collection(session)
         config = {
             "config": {
                 "features": [
@@ -69,9 +69,7 @@ class ConfigAPI(Resource):
                     "about-dataset": current_app.config["ABOUT_DATASET"]
                 },
                 "parameters": {
-                    "max-category-items": current_app.data.config["max_category_items"],
-                    "disable-diffexp": current_app.data.config["disable_diffexp"],
-                    "diffexp-may-be-slow": current_app.data.config["diffexp_may_be_slow"]
+                    **current_app.data.get_config_parameters(uid=cxguid, collection=anno_collection)
                 },
                 "library_versions": {
                     "cellxgene": cellxgene_version,
@@ -80,10 +78,6 @@ class ConfigAPI(Resource):
             }
         }
 
-        label_file = current_app.data.config["label_file"]
-        if label_file:
-            config["config"]["parameters"]["label_file"] = basename(label_file)
-
         return make_response(jsonify(config), HTTPStatus.OK)
 
 
@@ -93,9 +87,12 @@ class AnnotationsObsAPI(Resource):
         preferred_mimetype = request.accept_mimetypes.best_match(
             ["application/octet-stream"]
         )
+        cxguid = get_userid(session)
+        anno_collection = get_anno_collection(session)
         try:
             if preferred_mimetype == "application/octet-stream":
-                return make_response(current_app.data.annotation_to_fbs_matrix("obs", fields),
+                fbs = current_app.data.annotation_to_fbs_matrix("obs", fields, uid=cxguid, collection=anno_collection)
+                return make_response(fbs,
                                      HTTPStatus.OK,
                                      {"Content-Type": "application/octet-stream"})
             else:
@@ -106,9 +103,18 @@ class AnnotationsObsAPI(Resource):
             return make_response(str(e), HTTPStatus.INTERNAL_SERVER_ERROR)
 
     def put(self):
+        cxguid = get_userid(session)
+        anno_collection = request.args.get("annotation-collection-name", default=None)
+        if anno_collection is not None:
+            if not is_safe_collection_name(anno_collection):
+                return make_response(f"Error, bad annotation collection name", HTTPStatus.BAD_REQUEST)
+            set_anno_collection(session, anno_collection)
+        else:
+            anno_collection = get_anno_collection(session)
+
         try:
             fbs = request.get_data()
-            res = current_app.data.annotation_put_fbs("obs", fbs)
+            res = current_app.data.annotation_put_fbs("obs", fbs, uid=cxguid, collection=anno_collection)
             return make_response(
                 res, HTTPStatus.OK, {"Content-Type": "application/json"}
             )
@@ -246,6 +252,35 @@ class LayoutObsAPI(Resource):
             return make_response(str(e), HTTPStatus.INTERNAL_SERVER_ERROR)
 
 
+def get_userid(ss):
+    if CXGUID not in ss:
+        ss[CXGUID] = uuid4().hex
+        ss.permanent = True
+    return ss[CXGUID]
+
+
+def get_anno_collection(ss):
+    collection = ss[CXG_ANNO_COLLECTION] if CXG_ANNO_COLLECTION in ss else None
+    return collection
+
+
+def set_anno_collection(ss, name):
+    ss[CXG_ANNO_COLLECTION] = name
+    ss.permanent = True
+
+
+def is_safe_collection_name(name):
+    """
+    return true if this is a safe collection name
+
+    this is ultra convervative. If we want to allow full legal file name syntax,
+    we could look at modules like `pathvalidate`
+    """
+    if name is None:
+        return False
+    return re.match(r'^\w+$', name) is not None
+
+
 def get_api_resources():
     bp = Blueprint("api", __name__, url_prefix="/api/v0.2")
     api = Api(bp)

server/app/scanpy_engine/labels.py

@@ -1,52 +1,61 @@
 """
-Helpers for user annotations / label_file parameter
+Helpers for user annotations
 """
-from os.path import exists, splitext, getsize
-from os import remove, rename
+import os
+import os.path
+from datetime import datetime
 import pandas as pd
 
 
 def read_labels(fname):
-    if exists(fname) and getsize(fname) > 0:
+    if fname is not None and os.path.exists(fname) and os.path.getsize(fname) > 0:
         return pd.read_csv(fname, dtype='category', index_col=0, header=0, comment='#')
     else:
         return pd.DataFrame()
 
 
-def write_labels(fname, df, header=None):
-    rotate_fname(fname)
+def write_labels(fname, df, header=None, backup_dir=None):
+    if backup_dir is not None:
+        backup(fname, backup_dir)
+        # rotate_fname(fname, backup_dir)
     if not df.empty:
-        f = open(fname, 'a', newline="")
-        if header is not None:
-            f.write(header)
-        df.to_csv(f)
+        with open(fname, 'w', newline="") as f:
+            if header is not None:
+                f.write(header)
+            df.to_csv(f)
     else:
-        open(fname, 'a').close()
+        open(fname, 'w').close()
 
 
-def rotate_fname(fname):
+def backup(fname, backup_dir, max_backups=9):
     """
-    save N backups of file.
-        fname -> fname-0
-        fname-0 -> fname->1
-        ...
-        fname-(N-1) -> fname-N
+    save N backups of file to backup_dir.
+        1. fname -> backup_dir/fname-TIME
+        2. delete excess files in backup_dir
     """
 
-    def rotate(src, dst):
-        if exists(src):
-            if exists(dst):
-                remove(dst)
-            rename(src, dst)
+    # Make sure there is work to do
+    if not os.path.exists(fname):
+        return
 
-    rotation_size = 9  # rotation size
-    name, ext = splitext(fname)
+    # Ensure backup_dir exists
+    if not os.path.exists(backup_dir):
+        os.mkdir(backup_dir)
 
-    # rotate existing files
-    for i in range(rotation_size - 1, 0, -1):
-        src = f"{name}-{i}{ext}"
-        tgt = f"{name}-{i+1}{ext}"
-        rotate(src, tgt)
+    # Save current file to backup_dir
+    fname_base = os.path.basename(fname)
+    fname_base_root, fname_base_ext = os.path.splitext(fname_base)
+    # don't use ISO standard time format, as it contains characters illegal on some filesytems.
+    nowish = datetime.now().strftime('%Y-%m-%dT%H-%M-%S')
+    backup_fname = os.path.join(backup_dir, f"{fname_base_root}-{nowish}{fname_base_ext}")
+    if os.path.exists(backup_fname):
+        os.remove(backup_fname)
+    os.rename(fname, backup_fname)
 
-    tgt = f"{name}-1{ext}"
-    rotate(fname, tgt)
+    # prune the backup_dir to max number of backup files, keeping the most recent backups
+    backups = list(filter(lambda s: s.startswith(fname_base_root), os.listdir(backup_dir)))
+    excess_count = len(backups) - max_backups
+    if excess_count > 0:
+        backups.sort()
+        for bu in backups[0:excess_count]:
+            os.remove(os.path.join(backup_dir, bu))

server/app/scanpy_engine/scanpy_engine.py

@@ -2,6 +2,9 @@ import warnings
 import copy
 import threading
 from datetime import datetime
+import os.path
+from hashlib import blake2b
+import base64
 
 import numpy as np
 import pandas
@@ -37,7 +40,7 @@ class ScanpyEngine(CXGDriver):
     def __init__(self, data_locator=None, args={}):
         super().__init__(data_locator, args)
         # lock used to protect label file write ops
-        self.label_lock = threading.Lock()
+        self.label_lock = threading.RLock()
         if self.data:
             self._validate_and_initialize()
 
@@ -54,12 +57,41 @@ class ScanpyEngine(CXGDriver):
             "obs_names": None,
             "var_names": None,
             "diffexp_lfc_cutoff": 0.01,
-            "label_file": None,
+            "annotations": False,
+            "annotations_file": None,
+            "annotations_output_dir": None,
             "backed": False,
             "disable_diffexp": False,
             "diffexp_may_be_slow": False
         }
 
+    def get_config_parameters(self, uid=None, collection=None):
+        params = {
+            "max-category-items": self.config["max_category_items"],
+            "disable-diffexp": self.config["disable_diffexp"],
+            "diffexp-may-be-slow": self.config["diffexp_may_be_slow"],
+            "annotations": self.config["annotations"]
+        }
+        if self.config["annotations"]:
+            if uid is not None:
+                params.update({
+                    "annotations-user-data-idhash": self.get_userdata_idhash(uid)
+                })
+            if self.config['annotations_file'] is not None:
+                # user has hard-wired the name of the annotation data collection
+                fname = os.path.basename(self.config['annotations_file'])
+                collection_fname = os.path.splitext(fname)[0]
+                params.update({
+                    'annotations-data-collection-is-read-only': True,
+                    'annotations-data-collection-name': collection_fname
+                })
+            elif collection is not None:
+                params.update({
+                    'annotations-data-collection-is-read-only': False,
+                    'annotations-data-collection-name': collection
+                })
+        return params
+
     @staticmethod
     def _create_unique_column_name(df, col_name_prefix):
         """ given the columns of a dataframe, and a name prefix, return a column name which
@@ -197,20 +229,66 @@ class ScanpyEngine(CXGDriver):
             self.schema["layout"]["obs"].append(layout_schema)
 
     @requires_data
-    def get_schema(self):
+    def get_schema(self, uid=None, collection=None):
         schema = self.schema  # base schema
         # add label obs annotations as needed
-        if self.labels is not None:
+        labels = read_labels(self.get_anno_fname(uid, collection))
+        if labels is not None and not labels.empty:
             schema = copy.deepcopy(schema)
-            for col in self.labels.columns:
+            for col in labels.columns:
                 col_schema = {
                     "name": col,
                     "writable": True,
                 }
-                col_schema.update(self._get_col_type(self.labels[col]))
+                col_schema.update(self._get_col_type(labels[col]))
                 schema["annotations"]["obs"]["columns"].append(col_schema)
         return schema
 
+    def get_userdata_idhash(self, uid):
+        """
+        Return a short hash that weakly identifies the user and dataset.
+        Used to create safe annotations output file names.
+        """
+        id = (uid + self.data_locator.abspath()).encode()
+        idhash = base64.b32encode(blake2b(id, digest_size=5).digest()).decode('utf-8')
+        return idhash
+
+    def get_anno_fname(self, uid=None, collection=None):
+        """ return the current annotation file name """
+        if not self.config["annotations"]:
+            return None
+
+        if self.config["annotations_file"] is not None:
+            return self.config["annotations_file"]
+
+        # we need to generate a file name, which we can only do if we have a UID and collection name
+        if uid is None or collection is None:
+            return None
+        idhash = self.get_userdata_idhash(uid)
+        return os.path.join(self.get_anno_output_dir(), f"{collection}-{idhash}.csv")
+
+    def get_anno_output_dir(self):
+        """ return the current annotation output directory """
+        if not self.config["annotations"]:
+            return None
+
+        if self.config['annotations_output_dir']:
+            return self.config['annotations_output_dir']
+
+        if self.config['annotations_file']:
+            return os.path.dirname(os.path.abspath(self.config['annotations_file']))
+
+        return os.getcwd()
+
+    def get_anno_backup_dir(self, uid, collection=None):
+        """ return the current annotation backup directory """
+        if not self.config["annotations"]:
+            return None
+
+        fname = self.get_anno_fname(uid, collection)
+        root, ext = os.path.splitext(fname)
+        return f"{root}-backups"
+
     def _load_data(self, data_locator):
         # as of AnnData 0.6.19, backed mode performs initial load fast, but at the
         # cost of significantly slower access to X data.
@@ -240,17 +318,6 @@ class ScanpyEngine(CXGDriver):
                 f"Please check your input and try again."
             )
 
-        if self.config["label_file"]:
-            try:
-                self.labels = read_labels(self.config["label_file"])
-            except Exception as e:
-                raise ScanpyFileError(
-                    f"Error while loading label file: {e}, File must be in the .csv format, please check "
-                    f"your input and try again."
-                )
-        else:
-            self.labels = None
-
     @requires_data
     def _validate_and_initialize(self):
         # var and obs column names must be unique
@@ -262,9 +329,13 @@ class ScanpyEngine(CXGDriver):
         self.cell_count = self.data.shape[0]
         self.gene_count = self.data.shape[1]
         self._default_and_validate_layouts()
-        self._validate_label_data()
         self._create_schema()
 
+        # if the user has specified a fixed label file, go ahead and validate it
+        # so that we can remove errors early in the process.
+        if self.config["annotations_file"]:
+            self._validate_label_data(read_labels(self.get_anno_fname()))
+
         # heuristic
         n_values = self.data.shape[0] * self.data.shape[1]
         if (n_values > 1e8 and self.config['backed'] is True) or (n_values > 5e8):
@@ -352,24 +423,20 @@ class ScanpyEngine(CXGDriver):
                         )
 
     @requires_data
-    def _validate_label_data(self, labels=None):
+    def _validate_label_data(self, labels):
         """
         labels is None if disabled, empty if enabled by no data
         """
-        if labels is None:
-            labels = self.labels
-
         if labels is None or labels.empty:
             return
 
         # all lables must have a name, which must be unique and not used in obs column names
         if not labels.columns.is_unique:
-            raise KeyError(f"All column names specified in {self.config['label_file']} must be unique.")
+            raise KeyError(f"All column names specified in user annotations must be unique.")
 
         # the label index must be unique, and must have same values the anndata obs index
         if not labels.index.is_unique:
-            raise KeyError(f"All row index values specified in the label file "
-                           f"`{self.config['label_file']}` must be unique.")
+            raise KeyError(f"All row index values specified in user annotations must be unique.")
 
         if not labels.index.equals(self.original_obs_index):
             raise KeyError("Label file row index does not match H5AD file index. "
@@ -450,10 +517,21 @@ class ScanpyEngine(CXGDriver):
         return obs_selector, var_selector
 
     @requires_data
-    def annotation_to_fbs_matrix(self, axis, fields=None):
+    def annotation_to_fbs_matrix(self, axis, fields=None, uid=None, collection=None):
         if axis == Axis.OBS:
-            if self.labels is not None and not self.labels.empty:
-                df = self.data.obs.join(self.labels, self.config['obs_names'])
+            if self.config["annotations"]:
+                try:
+                    labels = read_labels(self.get_anno_fname(uid, collection))
+                except Exception as e:
+                    raise ScanpyFileError(
+                        f"Error while loading label file: {e}, File must be in the .csv format, please check "
+                        f"your input and try again."
+                    )
+            else:
+                labels = None
+
+            if labels is not None and not labels.empty:
+                df = self.data.obs.join(labels, self.config['obs_names'])
             else:
                 df = self.data.obs
         else:
@@ -463,18 +541,21 @@ class ScanpyEngine(CXGDriver):
         return encode_matrix_fbs(df, col_idx=df.columns)
 
     @requires_data
-    def annotation_put_fbs(self, axis, fbs):
-        fname = self.config["label_file"]
-        if not fname or self.labels is None:
+    def annotation_put_fbs(self, axis, fbs, uid=None, collection=None):
+        if not self.config["annotations"]:
             raise DisabledFeatureError("Writable annotations are not enabled")
 
+        fname = self.get_anno_fname(uid, collection)
+        if not fname:
+            raise ScanpyFileError("Writable annotations - unable to determine file name for annotations")
+
         if axis != Axis.OBS:
             raise ValueError("Only OBS dimension access is supported")
 
         new_label_df = decode_matrix_fbs(fbs)
         if not new_label_df.empty:
             new_label_df.index = self.original_obs_index
-        self._validate_label_data(labels=new_label_df)  # paranoia
+        self._validate_label_data(new_label_df)  # paranoia
 
         # if any of the new column labels overlap with our existing labels, raise error
         duplicate_columns = list(set(new_label_df.columns) & set(self.data.obs.columns))
@@ -485,14 +566,13 @@ class ScanpyEngine(CXGDriver):
         # update our internal state and save it.  Multi-threading often enabled,
         # so treat this as a critical section.
         with self.label_lock:
-            self.labels = new_label_df
             lastmod = self.data_locator.lastmodtime()
             lastmodstr = "'unknown'" if lastmod is None else lastmod.isoformat(timespec="seconds")
             header = f"# Annotations generated on {datetime.now().isoformat(timespec='seconds')} " \
                      f"using cellxgene version {cellxgene_version}\n" \
                      f"# Input data file was {self.data_locator.uri_or_path}, " \
                      f"which was last modified on {lastmodstr}\n"
-            write_labels(fname, self.labels, header)
+            write_labels(fname, new_label_df, header, backup_dir=self.get_anno_backup_dir(uid, collection))
 
         return jsonify_scanpy({"status": "OK"})
 

server/app/util/constants.py

@@ -33,3 +33,6 @@ JSON_NaN_to_num_warning_msg = (
 REACTIVE_LIMIT = 1_000_000
 
 MAX_LAYOUTS = 30
+
+CXGUID = "cxguid"
+CXG_ANNO_COLLECTION = "cxg_anno_collection"

server/app/util/data_locator.py

@@ -57,6 +57,16 @@ class DataLocator():
         else:
             return getattr(info, 'LastModified', None)
 
+    def abspath(self):
+        """
+        return the absolute path for the locator - only really does something
+        for file: protocol, as all others are already absolute
+        """
+        if self.islocal():
+            return os.path.abspath(self.path)
+        else:
+            return self.uri_or_path
+
     def isfile(self):
         return self.fs.isfile(self.cname)