From e2a12ba9bb15e1802cfe26afa9d69d45888e4616 Mon Sep 17 00:00:00 2001
From: Bruce Martin <bruce@chanzuckerberg.com>
Date: Thu, 9 Apr 2020 08:13:41 -0600
Subject: [PATCH] diffexp limit UI and configuration (#1336)

* warning on maxCount for diffexp

* cleanup logging

* clarification

* make the limits configurable

* make diff exp limit work

* danger!

* remove debugging code

* fix merge with master

* fix unit tests

Co-authored-by: Colin Megill <colinmegill@gmail.com>
---
 .../src/components/menubar/diffexpButtons.js  | 176 ++++++++++++++++++
 client/src/components/menubar/index.js        |  89 +--------
 server/common/app_config.py                   |  34 ++--
 server/common/default_config.py               |   4 +
 server/test/test_anndata_adaptor.py           |   4 +-
 server/test/test_nan_anndata_adaptor.py       |   4 +-
 6 files changed, 200 insertions(+), 111 deletions(-)
 create mode 100644 client/src/components/menubar/diffexpButtons.js

diff --git a/client/src/components/menubar/diffexpButtons.js b/client/src/components/menubar/diffexpButtons.js
new file mode 100644
index 00000000..3fab8269
--- /dev/null
+++ b/client/src/components/menubar/diffexpButtons.js
@@ -0,0 +1,176 @@
+// jshint esversion: 6
+import React from "react";
+import { connect } from "react-redux";
+import {
+  Popover,
+  Button,
+  ButtonGroup,
+  AnchorButton,
+  Tooltip,
+  Position
+} from "@blueprintjs/core";
+import * as globals from "../../globals";
+import actions from "../../actions";
+import CellSetButton from "./cellSetButtons";
+
+@connect(state => ({
+  config: state.config,
+  crossfilter: state.crossfilter,
+  differential: state.differential,
+  celllist1: state.differential?.celllist1,
+  celllist2: state.differential?.celllist2,
+  diffexpMayBeSlow: state.config?.parameters?.["diffexp-may-be-slow"] ?? false,
+  diffexpCellcountMax: state.config?.limits?.diffexp_cellcount_max
+}))
+class DiffexpButtons extends React.Component {
+  constructor(props) {
+    super(props);
+    this.state = {
+      userDismissedPopover: false
+    };
+  }
+
+  computeDiffExp = () => {
+    const { dispatch, differential } = this.props;
+    if (differential.celllist1 && differential.celllist2) {
+      dispatch(
+        actions.requestDifferentialExpression(
+          differential.celllist1,
+          differential.celllist2
+        )
+      );
+    }
+  };
+
+  clearDifferentialExpression = () => {
+    const { dispatch, differential } = this.props;
+    dispatch({
+      type: "clear differential expression",
+      diffExp: differential.diffExp
+    });
+    dispatch({
+      type: "clear scatterplot"
+    });
+  };
+
+  handlePopoverDismiss = () => {
+    this.setState({
+      userDismissedPopover: true
+    });
+  };
+
+  render() {
+    /* diffexp-related buttons may be disabled */
+    const { differential, diffexpMayBeSlow, diffexpCellcountMax } = this.props;
+    const { userDismissedPopover } = this.state;
+
+    const haveBothCellSets =
+      !!differential.celllist1 && !!differential.celllist2;
+
+    const haveEitherCellSet =
+      !!differential.celllist1 || !!differential.celllist2;
+
+    const slowMsg = diffexpMayBeSlow
+      ? " (CAUTION: large dataset - may take longer or fail)"
+      : "";
+    const tipMessage = `See top 10 differentially expressed genes${slowMsg}`;
+    const tipMessageWarn = `The total number of cells for differential expression computation
+                            may not exceed ${diffexpCellcountMax}. Try reselecting new cell sets.`;
+
+    const warnMaxSizeExceeded =
+      haveEitherCellSet &&
+      !!diffexpCellcountMax &&
+      (differential.celllist1?.length ?? 0) +
+        (differential.celllist2?.length ?? 0) >
+        diffexpCellcountMax;
+
+    return (
+      <ButtonGroup style={{ marginRight: 10 }}>
+        <CellSetButton
+          {...this.props} // eslint-disable-line react/jsx-props-no-spreading
+          eitherCellSetOneOrTwo={1}
+        />
+        <CellSetButton
+          {...this.props} // eslint-disable-line react/jsx-props-no-spreading
+          eitherCellSetOneOrTwo={2}
+        />
+        {!differential.diffExp ? (
+          <Popover
+            isOpen={/* warnMaxSizeExceeded && !userDismissedPopover */ false}
+            position={Position.BOTTOM}
+            target={
+              <Tooltip
+                content={warnMaxSizeExceeded ? tipMessageWarn : tipMessage}
+                position="bottom"
+                hoverOpenDelay={globals.tooltipHoverOpenDelayQuick}
+                intent={warnMaxSizeExceeded ? "danger" : "none"}
+              >
+                <AnchorButton
+                  disabled={!haveBothCellSets || warnMaxSizeExceeded}
+                  intent={warnMaxSizeExceeded ? "danger" : "primary"}
+                  data-testid="diffexp-button"
+                  loading={differential.loading}
+                  icon="left-join"
+                  fill
+                  onClick={this.computeDiffExp}
+                />
+              </Tooltip>
+            }
+            content={
+              <div
+                style={{
+                  display: "flex",
+                  justifyContent: "flex-start",
+                  alignItems: "flex-end",
+                  flexDirection: "column",
+                  padding: 10,
+                  maxWidth: 310
+                }}
+              >
+                <p>
+                  {`The total number of cells for differential expression computation
+                may not exceed ${diffexpCellcountMax}`}
+                </p>
+                <Button
+                  type="button"
+                  data-testid="diffexp-maxsize-exceeded-warning-dismiss"
+                  intent="warning"
+                  onClick={this.clearDifferentialExpression}
+                >
+                  Dismiss and clear cell sets
+                </Button>
+                <Button
+                  type="button"
+                  data-testid="diffexp-popover-dismiss"
+                  intent="none"
+                  onClick={this.handlePopoverDismiss}
+                >
+                  Dismiss
+                </Button>
+              </div>
+            }
+          />
+        ) : null}
+
+        {differential.diffExp ? (
+          <Tooltip
+            content="Remove differentially expressed gene list and clear cell selections"
+            position="bottom"
+            hoverOpenDelay={globals.tooltipHoverOpenDelayQuick}
+          >
+            <Button
+              type="button"
+              fill
+              intent="warning"
+              onClick={this.clearDifferentialExpression}
+            >
+              Clear Differential Expression
+            </Button>
+          </Tooltip>
+        ) : null}
+      </ButtonGroup>
+    );
+  }
+}
+
+export default DiffexpButtons;
diff --git a/client/src/components/menubar/index.js b/client/src/components/menubar/index.js
index 52951f7e..2e1e886b 100644
--- a/client/src/components/menubar/index.js
+++ b/client/src/components/menubar/index.js
@@ -4,12 +4,12 @@ import { connect } from "react-redux";
 import { Button, ButtonGroup, AnchorButton, Tooltip } from "@blueprintjs/core";
 import * as globals from "../../globals";
 import actions from "../../actions";
-import CellSetButton from "./cellSetButtons";
 import Clip from "./clip";
 import Embedding from "./embedding";
 import InformationMenu from "./infoMenu";
 import Subset from "./subset";
 import UndoRedoReset from "./undoRedo";
+import DiffexpButtons from "./diffexpButtons";
 
 @connect(state => ({
   universe: state.universe,
@@ -164,29 +164,6 @@ class MenuBar extends React.Component {
     this.setState({ pendingClipPercentiles: null });
   };
 
-  computeDiffExp = () => {
-    const { dispatch, differential } = this.props;
-    if (differential.celllist1 && differential.celllist2) {
-      dispatch(
-        actions.requestDifferentialExpression(
-          differential.celllist1,
-          differential.celllist2
-        )
-      );
-    }
-  };
-
-  clearDifferentialExpression = () => {
-    const { dispatch, differential } = this.props;
-    dispatch({
-      type: "clear differential expression",
-      diffExp: differential.diffExp
-    });
-    dispatch({
-      type: "clear scatterplot"
-    });
-  };
-
   handleCentroidChange = () => {
     const { dispatch, showCentroidLabels } = this.props;
 
@@ -209,71 +186,11 @@ class MenuBar extends React.Component {
     return world.nObs !== universe.nObs;
   };
 
-  renderDiffExp() {
-    /* diffexp-related buttons may be disabled */
-    const { disableDiffexp, differential, diffexpMayBeSlow } = this.props;
-    if (disableDiffexp) return null;
-
-    const haveBothCellSets =
-      !!differential.celllist1 && !!differential.celllist2;
-
-    const slowMsg = diffexpMayBeSlow
-      ? " (CAUTION: large dataset - may take longer or fail)"
-      : "";
-    const tipMessage = `See top 10 differentially expressed genes${slowMsg}`;
-
-    return (
-      <ButtonGroup style={{ marginRight: 10 }}>
-        <CellSetButton
-          {...this.props} // eslint-disable-line react/jsx-props-no-spreading
-          eitherCellSetOneOrTwo={1}
-        />
-        <CellSetButton
-          {...this.props} // eslint-disable-line react/jsx-props-no-spreading
-          eitherCellSetOneOrTwo={2}
-        />
-        {!differential.diffExp ? (
-          <Tooltip
-            content={tipMessage}
-            position="bottom"
-            hoverOpenDelay={globals.tooltipHoverOpenDelayQuick}
-          >
-            <AnchorButton
-              disabled={!haveBothCellSets}
-              intent="primary"
-              data-testid="diffexp-button"
-              loading={differential.loading}
-              icon="left-join"
-              fill
-              onClick={this.computeDiffExp}
-            />
-          </Tooltip>
-        ) : null}
-
-        {differential.diffExp ? (
-          <Tooltip
-            content="Remove differentially expressed gene list and clear cell selections"
-            position="bottom"
-            hoverOpenDelay={globals.tooltipHoverOpenDelayQuick}
-          >
-            <Button
-              type="button"
-              fill
-              intent="warning"
-              onClick={this.clearDifferentialExpression}
-            >
-              Clear Differential Expression
-            </Button>
-          </Tooltip>
-        ) : null}
-      </ButtonGroup>
-    );
-  }
-
   render() {
     const {
       dispatch,
       libraryVersions,
+      disableDiffexp,
       undoDisabled,
       redoDisabled,
       selectionTool,
@@ -307,7 +224,7 @@ class MenuBar extends React.Component {
           display: "flex"
         }}
       >
-        {this.renderDiffExp()}
+        {disableDiffexp ? null : <DiffexpButtons/>}
         <Subset
           subsetPossible={this.subsetPossible()}
           subsetResetPossible={this.subsetResetPossible()}
diff --git a/server/common/app_config.py b/server/common/app_config.py
index acaaeb8e..50510b96 100644
--- a/server/common/app_config.py
+++ b/server/common/app_config.py
@@ -1,6 +1,3 @@
-# -*- coding: utf-8 -*-
-
-
 from server import __version__ as cellxgene_version
 from flatten_dict import flatten
 from os import mkdir, environ
@@ -22,18 +19,6 @@ DEFAULT_SERVER_PORT = int(environ.get("CXG_SERVER_PORT", "5005"))
 # anything bigger than this will generate a special message
 BIG_FILE_SIZE_THRESHOLD = 100 * 2 ** 20  # 100MB
 
-""" Default limits for requests """
-Default_Limits = {
-    # Max number of columns that may be requested for /annotations or /data routes.
-    # This is a simplistic means of preventing excess resource consumption (eg,
-    # requesting the entire X matrix in one request) or other DoS style attacks/errors.
-    # Set to None to disable check.
-    "column_request_max": 32,
-    # Max number of cells that will be accepted for differential expression.
-    # Set to None to disable the check.
-    "diffexp_cellcount_max": None,  # None is disabled
-}
-
 
 class AppFeature(object):
     def __init__(self, path, available=False, method="POST", extra={}):
@@ -103,12 +88,12 @@ class AppConfig(object):
             self.adaptor__cxg_adaptor__tiledb_ctx = dc["adaptor"]["cxg_adaptor"]["tiledb_ctx"]
             self.adaptor__anndata_adaptor__backed = dc["adaptor"]["anndata_adaptor"]["backed"]
 
+            self.limits__diffexp_cellcount_max = dc["limits"]["diffexp_cellcount_max"]
+            self.limits__column_request_max = dc["limits"]["column_request_max"]
+
         except KeyError as e:
             raise ConfigurationError(f"Unexpected config: {str(e)}")
 
-        # Used for various limits, eg, size of requests.  Not currently configurable.
-        self.limits = Default_Limits
-
         # The annotation object is created during complete_config and stored here.
         self.user_annotations = None
 
@@ -128,7 +113,6 @@ class AppConfig(object):
 
     def __mapping(self, config):
         """Create a mapping from attribute names to (location in the config tree, value)"""
-
         dc = copy.deepcopy(config)
         mapping = {}
 
@@ -211,6 +195,7 @@ class AppConfig(object):
         self.handle_embeddings(context)
         self.handle_diffexp(context)
         self.handle_adaptor(context)
+        self.handle_limits(context)
 
         self.is_completed = True
         self.check_config()
@@ -447,6 +432,10 @@ class AppConfig(object):
         # anndata
         self.__check_attr("adaptor__anndata_adaptor__backed", bool)
 
+    def handle_limits(self, context):
+        self.__check_attr("limits__diffexp_cellcount_max", (type(None), int))
+        self.__check_attr("limits__column_request_max", (type(None), int))
+
     def get_title(self, data_adaptor):
         return self.single_dataset__title if self.single_dataset__title else data_adaptor.get_title()
 
@@ -514,12 +503,15 @@ class AppConfig(object):
         config["library_versions"] = library_versions
         config["links"] = links
         config["parameters"] = parameters
-        config["limits"] = self.limits
+        config["limits"] = {
+            'column_request_max': self.limits__column_request_max,
+            'diffexp_cellcount_max': self.limits__diffexp_cellcount_max,
+        }
 
         return c
 
     def exceeds_limit(self, limit_name, value):
-        limit_value = self.limits.get(limit_name, None)
+        limit_value = getattr(self, "limits__" + limit_name, None)
         if limit_value is None:  # disabled
             return False
         return value > limit_value
diff --git a/server/common/default_config.py b/server/common/default_config.py
index 635bbd0f..1b3c8683 100644
--- a/server/common/default_config.py
+++ b/server/common/default_config.py
@@ -80,6 +80,10 @@ adaptor:
   anndata_adaptor:
       backed: false
 
+limits:
+  column_request_max: 32
+  diffexp_cellcount_max: null
+
 """
 
 
diff --git a/server/test/test_anndata_adaptor.py b/server/test/test_anndata_adaptor.py
index 12d73e47..cc069853 100644
--- a/server/test/test_anndata_adaptor.py
+++ b/server/test/test_anndata_adaptor.py
@@ -41,11 +41,11 @@ class AdaptorTest(unittest.TestCase):
             "diffexp__lfc_cutoff": 0.01,
             "adaptor__anndata_adaptor__backed": self.backed,
             "single_dataset__datapath": self.data_locator,
+            "limits__diffexp_cellcount_max": None,
+            "limits__column_request_max": None
         }
         config = AppConfig()
         config.update(**args)
-        for k in config.limits.keys():
-            config.limits[k] = None
         config.complete_config()
         self.data = AnndataAdaptor(DataLocator(self.data_locator), config)
 
diff --git a/server/test/test_nan_anndata_adaptor.py b/server/test/test_nan_anndata_adaptor.py
index 861c2776..8c37b7d6 100644
--- a/server/test/test_nan_anndata_adaptor.py
+++ b/server/test/test_nan_anndata_adaptor.py
@@ -19,13 +19,13 @@ class NaNTest(unittest.TestCase):
             "single_dataset__obs_names": None,
             "single_dataset__var_names": None,
             "diffexp__lfc_cutoff": 0.01,
+            "limits__diffexp_cellcount_max": None,
+            "limits__column_request_max": None,
         }
         config = AppConfig()
         config.update(**self.args)
         locator = DataLocator("test/test_datasets/nan.h5ad")
         config.update(single_dataset__datapath=locator.path)
-        for k in config.limits.keys():
-            config.limits[k] = None
         config.complete_config()
 
         with warnings.catch_warnings():