diff --git a/client/package.json b/client/package.json index 8861b786..acabc622 100644 --- a/client/package.json +++ b/client/package.json @@ -6,7 +6,7 @@ "repository": "https://github.com/chanzuckerberg/cellxgene", "scripts": { "backend-dev": "python3.6 -m venv cellxgene && source cellxgene/bin/activate && yes | pip uninstall cellxgene || true && pip install -e .. && cellxgene launch ", - "backend-dev-anno": "python3.6 -m venv cellxgene && source cellxgene/bin/activate && yes | pip uninstall cellxgene || true && pip install -e .. && cellxgene launch --experimental-label-file labels.csv ", + "backend-dev-anno": "python3.6 -m venv cellxgene && source cellxgene/bin/activate && yes | pip uninstall cellxgene || true && pip install -e .. && cellxgene launch --experimental-annotations ", "build": "npm run clean && webpack --config configuration/webpack/webpack.config.prod.js", "clean": "rimraf build", "dev": "npm run clean && webpack --config configuration/webpack/webpack.config.dev.js", diff --git a/client/src/actions/index.js b/client/src/actions/index.js index 2751c106..76f5745b 100644 --- a/client/src/actions/index.js +++ b/client/src/actions/index.js @@ -38,7 +38,7 @@ const doInitialDataLoad = () => /* only load names for var annotations, if possible*/ const varIndexName = schema?.schema?.annotations?.var?.index; const varAnnotationsQuery = varIndexName - ? `?annotation-name=${varIndexName}` + ? `?annotation-name=${encodeURIComponent(varIndexName)}` : ""; const varAnnotationsURL = `annotations/var${varAnnotationsQuery}`; const requestBinary = ["annotations/obs", varAnnotationsURL, "layout/obs"] @@ -84,9 +84,7 @@ const regraph = () => (dispatch, getState) => { // Throws const dispatchExpressionErrors = (dispatch, res) => { - const msg = `Unexpected HTTP response while fetching expression data ${ - res.status - }, ${res.statusText}`; + const msg = `Unexpected HTTP response while fetching expression data ${res.status}, ${res.statusText}`; dispatchNetworkErrorMessageToUser(msg); throw new Error(msg); }; @@ -119,7 +117,8 @@ async function _doRequestExpressionData(dispatch, getState, genes) { headers: new Headers({ accept: "application/octet-stream", "Content-Type": "application/json" - }) + }), + credentials: "include" } ); @@ -226,9 +225,7 @@ const dispatchDiffExpErrors = (dispatch, response) => { ); break; default: { - const msg = `Unexpected differential expression HTTP response ${ - response.status - }, ${response.statusText}`; + const msg = `Unexpected differential expression HTTP response ${response.status}, ${response.statusText}`; dispatchNetworkErrorMessageToUser(msg); dispatch({ type: "request differential expression error", @@ -277,7 +274,8 @@ const requestDifferentialExpression = (set1, set2, num_genes = 10) => async ( count: num_genes, set1: { filter: { obs: { index: set1 } } }, set2: { filter: { obs: { index: set2 } } } - }) + }), + credentials: "include" } ); @@ -341,8 +339,9 @@ const resetInterface = () => (dispatch, getState) => { }; const saveObsAnnotations = () => async (dispatch, getState) => { - const { universe } = getState(); + const { universe, annotations } = getState(); const { obsAnnotations, schema } = universe; + const { dataCollectionNameIsReadOnly, dataCollectionName } = annotations; dispatch({ type: "writable obs annotations - save started" @@ -354,14 +353,21 @@ const saveObsAnnotations = () => async (dispatch, getState) => { const df = obsAnnotations.subset(null, writableAnnotations); const matrix = MatrixFBS.encodeMatrixFBS(df); try { + const queryString = + !dataCollectionNameIsReadOnly && !!dataCollectionName + ? `?annotation-collection-name=${encodeURIComponent( + dataCollectionName + )}` + : ""; const res = await fetch( - `${globals.API.prefix}${globals.API.version}annotations/obs`, + `${globals.API.prefix}${globals.API.version}annotations/obs${queryString}`, { method: "PUT", body: matrix, headers: new Headers({ "Content-Type": "application/octet-stream" - }) + }), + credentials: "include" } ); if (res.ok) { diff --git a/client/src/components/autosave/filenameDialog.js b/client/src/components/autosave/filenameDialog.js new file mode 100644 index 00000000..b6e06a49 --- /dev/null +++ b/client/src/components/autosave/filenameDialog.js @@ -0,0 +1,168 @@ +import React from "react"; +import { connect } from "react-redux"; + +import { + Button, + Tooltip, + InputGroup, + Dialog, + Classes, + Colors +} from "@blueprintjs/core"; + +@connect(state => ({ + universe: state.universe, + idhash: state.config?.parameters?.["annotations-user-data-idhash"] ?? null, + annotations: state.annotations, + obsAnnotations: state.universe.obsAnnotations, + saveInProgress: state.autosave?.saveInProgress ?? false, + lastSavedObsAnnotations: state.autosave?.lastSavedObsAnnotations, + error: state.autosave?.error, + writableCategoriesEnabled: state.config?.parameters?.["annotations"] ?? false +})) +class FilenameDialog extends React.Component { + constructor(props) { + super(props); + this.state = { + filenameText: "" + }; + } + + dismissFilenameDialog = () => {}; + + handleCreateFilename = () => { + const { dispatch } = this.props; + const { filenameText } = this.state; + + dispatch({ + type: "set annotations collection name", + data: filenameText + }); + }; + + filenameError = () => { + const legalNames = /^\w+$/; + const { filenameText } = this.state; + let err = false; + + if (filenameText === "") { + err = "empty_string"; + } else if (!legalNames.test(filenameText)) { + /* + IMPORTANT: this test must ultimately match the test applied by the + backend, which is designed to ensure a safe file name can be created + from the data collection name. If you change this, you will also need + to change the validation code in the backend, or it will have no effect. + */ + err = "characters"; + } + + return err; + }; + + filenameErrorMessage = () => { + const err = this.filenameError(); + let markup = null; + + if (err === "empty_string") { + markup = ( + + {"Filename cannot be blank"} + + ); + } else if (err === "characters") { + markup = ( + + {"Only alphanumeric and underscore allowed"} + + ); + } + return markup; + }; + + render() { + const { writableCategoriesEnabled, annotations, idhash } = this.props; + const { filenameText } = this.state; + + return writableCategoriesEnabled && + !annotations.dataCollectionNameIsReadOnly && + !annotations.dataCollectionName ? ( + +
{ + e.preventDefault(); + this.handleCreateFilename(); + }} + > +
+
+

Name your collection of user generated annotations:

+ this.setState({ filenameText: e.target.value })} + leftIcon="tag" + /> +

+ {this.filenameErrorMessage(filenameText)} +

+
+
+

+ You can find your collection at:{" "} + + {filenameText}-{idhash}.csv + +

+
+
+
+
+ + + + +
+
+
+ + ) : null; + } +} + +export default FilenameDialog; diff --git a/client/src/components/autosave/index.js b/client/src/components/autosave/index.js index 08a2abe2..971d04d6 100644 --- a/client/src/components/autosave/index.js +++ b/client/src/components/autosave/index.js @@ -2,14 +2,16 @@ import React from "react"; import { connect } from "react-redux"; import * as globals from "../../globals"; import actions from "../../actions"; +import FilenameDialog from "./filenameDialog"; @connect(state => ({ universe: state.universe, + annotations: state.annotations, obsAnnotations: state.universe.obsAnnotations, saveInProgress: state.autosave?.saveInProgress ?? false, lastSavedObsAnnotations: state.autosave?.lastSavedObsAnnotations, error: state.autosave?.error, - writableCategoriesEnabled: state.config?.parameters?.["label_file"] ?? false + writableCategoriesEnabled: state.config?.parameters?.["annotations"] ?? false })) class Autosave extends React.Component { constructor(props) { @@ -73,6 +75,7 @@ class Autosave extends React.Component { }} > {this.statusMessage()} + ) : null; } diff --git a/client/src/components/categorical/categorical.js b/client/src/components/categorical/categorical.js index f2dc5b37..b5556c0f 100644 --- a/client/src/components/categorical/categorical.js +++ b/client/src/components/categorical/categorical.js @@ -18,7 +18,7 @@ import { AnnotationsHelpers } from "../../util/stateManager"; @connect(state => ({ categoricalSelection: state.categoricalSelection, - writableCategoriesEnabled: state.config?.parameters?.["label_file"] ?? false, + writableCategoriesEnabled: state.config?.parameters?.["annotations"] ?? false, schema: state.world?.schema })) class Categories extends React.Component { diff --git a/client/src/components/categorical/value.js b/client/src/components/categorical/value.js index 6a1ef38f..eaafdc7b 100644 --- a/client/src/components/categorical/value.js +++ b/client/src/components/categorical/value.js @@ -84,7 +84,6 @@ class CategoryValue extends React.Component { }; valueNameErrorMessage = () => { - const { editedLabelText } = this.state; const err = this.valueNameError(); if (err === false) return null; @@ -555,7 +554,9 @@ class CategoryValue extends React.Component { data-testclass="handleDeleteValue" data-testid={`handleDeleteValue-${metadataField}`} onClick={this.handleDeleteValue} - text={`Delete this label, and reassign all cells to type '${globals.unassignedCategoryLabel}'`} + text={`Delete this label, and reassign all cells to type '${ + globals.unassignedCategoryLabel + }'`} /> ) : null} diff --git a/client/src/reducers/annotations.js b/client/src/reducers/annotations.js index 047b9abd..177cbf8a 100644 --- a/client/src/reducers/annotations.js +++ b/client/src/reducers/annotations.js @@ -3,6 +3,24 @@ Reducers for annotation UI-state. */ const Annotations = ( state = { + /* + Annotations collection name - which will be used to save the named set of annotations + in some persistent store (database, file system, etc). + + The backend may expect this to be a legal file name, which is typically alpha-numeric, plus [_-,.]. + Keep it simple or the server may return an error. + + If `dataCollectionNameIsReadOnly` is true, you may NOT change the data collection name. + If false, you may change `dataCollectionName` and it will be used at the time the annotations are + written to the back-end. + + */ + dataCollectionNameIsReadOnly: true, + dataCollectionName: null, + + /* + Annotations UI component state + */ isEditingCategoryName: false, isEditingLabelName: false, categoryBeingEdited: null, @@ -12,6 +30,31 @@ const Annotations = ( action ) => { switch (action.type) { + case "configuration load complete": { + const DefaultDataCollectionName = null; + const dataCollectionName = + action.config.parameters?.["annotations-data-collection-name"] ?? null; + const dataCollectionNameIsReadOnly = + action.config.parameters?.[ + "annotations-data-collection-name-is-read-only" + ] ?? false; + return { + ...state, + dataCollectionNameIsReadOnly, + dataCollectionName + }; + } + + case "set annotations collection name": { + if (state.dataCollectionNameIsReadOnly) { + throw new Error("data collection name is read only"); + } + return { + ...state, + dataCollectionName: action.data + }; + } + /* CATEGORY */ case "annotation: activate add new label mode": return { diff --git a/client/src/util/actionHelpers.js b/client/src/util/actionHelpers.js index c7e23267..de4fa55a 100644 --- a/client/src/util/actionHelpers.js +++ b/client/src/util/actionHelpers.js @@ -33,7 +33,8 @@ const doFetch = async (url, acceptType) => { method: "get", headers: new Headers({ Accept: acceptType - }) + }), + credentials: "include" }); if (res.ok && res.headers.get("Content-Type").includes(acceptType)) { return res; diff --git a/server/app/app.py b/server/app/app.py index 85f51262..466cc822 100644 --- a/server/app/app.py +++ b/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") diff --git a/server/app/driver/driver.py b/server/app/driver/driver.py index 0b970ab0..30aa8f06 100644 --- a/server/app/driver/driver.py +++ b/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 """ diff --git a/server/app/rest_api/rest.py b/server/app/rest_api/rest.py index b4a68c27..780dd38f 100644 --- a/server/app/rest_api/rest.py +++ b/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) diff --git a/server/app/scanpy_engine/labels.py b/server/app/scanpy_engine/labels.py index f06aeb40..3a5f7d08 100644 --- a/server/app/scanpy_engine/labels.py +++ b/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)) diff --git a/server/app/scanpy_engine/scanpy_engine.py b/server/app/scanpy_engine/scanpy_engine.py index 0392a9cb..a1a2b32a 100644 --- a/server/app/scanpy_engine/scanpy_engine.py +++ b/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"}) diff --git a/server/app/util/constants.py b/server/app/util/constants.py index 13c818a6..b53d476c 100644 --- a/server/app/util/constants.py +++ b/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" diff --git a/server/app/util/data_locator.py b/server/app/util/data_locator.py index e4d1af5d..8a44f06d 100644 --- a/server/app/util/data_locator.py +++ b/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) diff --git a/server/cli/launch.py b/server/cli/launch.py index 1d9b5691..5cf50e51 100644 --- a/server/cli/launch.py +++ b/server/cli/launch.py @@ -2,7 +2,7 @@ import errno import functools import logging from os import devnull -from os.path import splitext, basename +from os.path import splitext, basename, isdir import sys import warnings import webbrowser @@ -70,12 +70,28 @@ def common_args(func): metavar="", help="Minimum log fold change threshold for differential expression.",) @click.option( - "--experimental-label-file", + "--experimental-annotations", + is_flag=True, + default=False, + show_default=True, + help="Enable user annotation of data." + ) + @click.option( + "--experimental-annotations-file", default=None, show_default=True, multiple=False, metavar="", help="CSV file containing user annotations; will be overwritten. Created if does not exist.",) + @click.option( + "--experimental-annotations-output-dir", + default=None, + show_default=False, + multiple=False, + metavar="", + help="Directory where annotation CSV files will be written (directory must exist). " + "Defaults to current directory.", + ) @click.option( "--backed", "-b", @@ -96,16 +112,20 @@ def common_args(func): return wrapper -def parse_engine_args(embedding, obs_names, var_names, max_category_items, - diffexp_lfc_cutoff, experimental_label_file, backed, - disable_diffexp): +def parse_engine_args(embedding, obs_names, var_names, max_category_items, diffexp_lfc_cutoff, + experimental_annotations, experimental_annotations_file, + experimental_annotations_output_dir, backed, disable_diffexp): + annotations_file = experimental_annotations_file if experimental_annotations else None + annotations_output_dir = experimental_annotations_output_dir if experimental_annotations else None return { "layout": embedding, "max_category_items": max_category_items, "diffexp_lfc_cutoff": diffexp_lfc_cutoff, "obs_names": obs_names, "var_names": var_names, - "label_file": experimental_label_file, + "annotations": experimental_annotations, + "annotations_file": annotations_file, + "annotations_output_dir": annotations_output_dir, "backed": backed, "disable_diffexp": disable_diffexp } @@ -177,7 +197,9 @@ def launch( title, scripts, about, - experimental_label_file, + experimental_annotations, + experimental_annotations_file, + experimental_annotations_output_dir, backed, disable_diffexp ): @@ -196,7 +218,11 @@ def launch( > cellxgene launch """ e_args = parse_engine_args(embedding, obs_names, var_names, max_category_items, - diffexp_lfc_cutoff, experimental_label_file, backed, + diffexp_lfc_cutoff, + experimental_annotations, + experimental_annotations_file, + experimental_annotations_output_dir, + backed, disable_diffexp) try: data_locator = DataLocator(data) @@ -256,10 +282,24 @@ def launch( else: port = find_available_port(host) - if experimental_label_file: - lf_name, lf_ext = splitext(experimental_label_file) - if lf_ext and lf_ext != ".csv": - raise click.FileError(basename(experimental_label_file), hint="label file type must be .csv") + if not experimental_annotations: + if experimental_annotations_file is not None: + click.echo("Warning: --experimental-annotations-file ignored as --annotations not enabled.") + if experimental_annotations_output_dir is not None: + click.echo("Warning: --experimental-annotations-output-dir ignored as --annotations not enabled.") + else: + if experimental_annotations_file is not None and experimental_annotations_output_dir is not None: + raise click.ClickException("--experimental-annotations-file and --experimental-annotations-output-dir " + "may not be used together.") + + if experimental_annotations_file is not None: + lf_name, lf_ext = splitext(experimental_annotations_file) + if lf_ext and lf_ext != ".csv": + raise click.FileError(basename(experimental_annotations_file), hint="annotation file type must be .csv") + + if experimental_annotations_output_dir is not None and not isdir(experimental_annotations_output_dir): + raise click.ClickException('--experimental-annotations-output-dir must specify an existing directory. ' + f'"{experimental_annotations_output_dir}" does not exist.') if about: def url_check(url): diff --git a/server/test/test_scanpy_engine_data_load.py b/server/test/test_scanpy_engine_data_load.py index 3379a5dc..2be0b400 100644 --- a/server/test/test_scanpy_engine_data_load.py +++ b/server/test/test_scanpy_engine_data_load.py @@ -24,7 +24,9 @@ class DataLoadEngineTest(unittest.TestCase): "obs_names": "foo", "var_names": "bar", "diffexp_lfc_cutoff": 0.1, - "label_file": None, + "annotations": False, + "annotations_file": None, + "annotations_output_dir": None, "backed": False, "diffexp_may_be_slow": False, "disable_diffexp": False diff --git a/server/test/test_writable_annotation.py b/server/test/test_writable_annotation.py index 4e817566..5df57f1c 100644 --- a/server/test/test_writable_annotation.py +++ b/server/test/test_writable_annotation.py @@ -16,14 +16,16 @@ from server.app.util.data_locator import DataLocator class WritableAnnotationTest(unittest.TestCase): def setUp(self): self.tmpDir = tempfile.mkdtemp() - self.label_file = path.join(self.tmpDir, "labels.csv") + self.annotations_file = path.join(self.tmpDir, "test_annotations.csv") args = { "layout": ["umap"], "max_category_items": 100, "obs_names": None, "var_names": None, "diffexp_lfc_cutoff": 0.01, - "label_file": self.label_file + "annotations": True, + "annotations_file": self.annotations_file, + "annotations_output_dir": None } self.data = ScanpyEngine(DataLocator("example-dataset/pbmc3k.h5ad"), args) @@ -59,8 +61,8 @@ class WritableAnnotationTest(unittest.TestCase): }) res = self.data.annotation_put_fbs("obs", fbs) self.assertEqual(res, json.dumps({"status": "OK"})) - self.assertTrue(path.exists(self.label_file)) - df = pd.read_csv(self.label_file, index_col=0, header=0, comment='#') + self.assertTrue(path.exists(self.annotations_file)) + df = pd.read_csv(self.annotations_file, index_col=0, header=0, comment='#') self.assertEqual(df.shape, (n_rows, 2)) self.assertEqual(set(df.columns), set(['cat_A', 'cat_B'])) self.assertTrue(self.data.original_obs_index.equals(df.index)) @@ -74,15 +76,18 @@ class WritableAnnotationTest(unittest.TestCase): }) res = self.data.annotation_put_fbs("obs", fbs) self.assertEqual(res, json.dumps({"status": "OK"})) - self.assertTrue(path.exists(self.label_file)) - df = pd.read_csv(self.label_file, index_col=0, header=0, comment='#') + self.assertTrue(path.exists(self.annotations_file)) + df = pd.read_csv(self.annotations_file, index_col=0, header=0, comment='#') self.assertEqual(set(df.columns), set(['cat_A', 'cat_C'])) self.assertTrue(np.all(df['cat_A'] == ['label_A1' for l in range(0, n_rows)])) self.assertTrue(np.all(df['cat_C'] == ['label_C' for l in range(0, n_rows)])) # rotation - name, ext = path.splitext(self.label_file) - self.assertTrue(path.exists(f"{name}-1{ext}")) + name, ext = path.splitext(self.annotations_file) + backup_dir = f"{name}-backups" + self.assertTrue(path.isdir(backup_dir)) + found_files = listdir(backup_dir) + self.assertEqual(len(found_files), 1) def test_file_rotation_to_max_9(self): # verify we stop rotation at 9 @@ -95,10 +100,11 @@ class WritableAnnotationTest(unittest.TestCase): res = self.data.annotation_put_fbs("obs", fbs) self.assertEqual(res, json.dumps({"status": "OK"})) - name, ext = path.splitext(self.label_file) - expected_files = [self.label_file] + [f"{name}-{i}{ext}" for i in range(1, 10)] - found_files = [path.join(self.tmpDir, p) for p in listdir(self.tmpDir)] - self.assertEqual(set(expected_files), set(found_files)) + name, ext = path.splitext(self.annotations_file) + backup_dir = f"{name}-backups" + self.assertTrue(path.isdir(backup_dir)) + found_files = listdir(backup_dir) + self.assertTrue(len(found_files) <= 9) def test_put_get_roundtrip(self): # verify that OBS PUTs (annotation_put_fbs) are accessible via