Merge branch 'dev' into migrate-pane-tests-playwright

Author: tonypzy

README.md

@@ -959,6 +959,7 @@ The following `opts` are generic in the sense that they are the same for all vis
 - `opts.marginright` : right margin (in pixels)
 - `opts.margintop`   : top margin (in pixels)
 - `opts.marginbottom`: bottom margin (in pixels)
+- `opts.caption`     : caption displayed below the plot (`string`; optional)
 
 `opts` are passed as dictionary in python scripts.You can pass `opts` like:
 

js/panes/EmbeddingsPane.js

@@ -115,7 +115,9 @@ class EmbeddingsPane extends React.Component {
     });
     var url = window.URL.createObjectURL(blob);
     var link = document.createElement('a');
-    link.download = 'visdom_tsne_data.txt';
+    link.download = this.props.contentID
+      ? `${this.props.contentID}_tsne_data.txt`
+      : 'plot_tsne_data.txt';
     link.href = url;
     link.click();
   };

js/panes/NetworkPane.js

@@ -46,7 +46,8 @@ function NetworkPane(props) {
     }
 
     requestAnimationFrame(() => {
-      saveSvgAsPng(svg, 'plot.png', {
+      const filename = props.contentID ? `${props.contentID}.png` : 'plot.png';
+      saveSvgAsPng(svg, filename, {
         scale: 2,
         backgroundColor: '#FFFFFF',
       });

js/panes/PlotPane.js

@@ -52,7 +52,7 @@ var PlotPane = (props) => {
   const handleDownload = () => {
     Plotly.downloadImage(plotlyRef.current, {
       format: 'svg',
-      filename: contentID,
+      filename: contentID || 'plot',
     });
   };
 
@@ -67,7 +67,7 @@ var PlotPane = (props) => {
     const url = window.URL.createObjectURL(blob);
     const link = document.createElement('a');
     link.href = url;
-    link.download = `${contentID}_metadata.json`;
+    link.download = `${contentID || 'plot'}_metadata.json`;
     document.body.appendChild(link);
     link.click();
     document.body.removeChild(link);
@@ -242,6 +242,10 @@ var PlotPane = (props) => {
       layout.margin.t = 30;
     }
 
+    if (content.caption) {
+      layout.margin.b = Math.max(layout.margin.b || 60, 100);
+    }
+
     // draw / redraw plot with layout-options
     Plotly.react(contentID, data.concat(smooth_data), content.layout, {
       showLink: false,
@@ -321,11 +325,11 @@ var PlotPane = (props) => {
   }
 
   var caption_widget = '';
-  if (isHistory && content && content.caption) {
+  if (content && content.caption) {
     caption_widget = (
-      <span className="widget" key="plot_caption">
+      <div className="widget plot-caption" key="plot_caption">
         {content.caption}
-      </span>
+      </div>
     );
   }
 

py/test/test_plots.py py/tests/test_plots.pypy/test/test_plots.py renamed to py/tests/test_plots.py

@@ -0,0 +1,60 @@
+"""
+Tests that the server routes persistence through ``Application.storage``
+(the DataStore backend) rather than calling ``serialize_env`` directly.
+
+These guard the PR-#2 wiring: end-to-end save/fork/reload behavior is already
+covered in ``test_environment_lifecycle``; here we assert the abstraction itself
+is in place so a future refactor cannot silently bypass the backend.
+"""
+
+import json
+import tempfile
+import unittest
+
+import tornado.testing
+
+from visdom.data_model.base import DataStore
+from visdom.data_model.json_store import JSONStore
+from visdom.server.app import Application
+
+
+class TestStorageWiring(tornado.testing.AsyncHTTPTestCase):
+    def setUp(self):
+        self._tmp_dir = tempfile.mkdtemp(prefix="visdom_wire_")
+        super().setUp()
+
+    def get_app(self):
+        return Application(port=self.get_http_port(), env_path=self._tmp_dir)
+
+    def post_json(self, path, body):
+        return self.fetch(
+            path,
+            method="POST",
+            body=json.dumps(body),
+            headers={"Content-Type": "application/json"},
+        )
+
+    def test_application_has_json_store(self):
+        self.assertIsInstance(self._app.storage, DataStore)
+        self.assertIsInstance(self._app.storage, JSONStore)
+        self.assertEqual(self._app.storage.env_path, self._tmp_dir)
+
+    def test_save_routes_through_storage(self):
+        calls = []
+        real_save_envs = self._app.storage.save_envs
+
+        def spy(state, eids):
+            calls.append(list(eids))
+            return real_save_envs(state, eids)
+
+        self._app.storage.save_envs = spy
+
+        resp = self.post_json("/save", {"data": ["main"]})
+
+        self.assertEqual(len(calls), 1)
+        self.assertIn("main", calls[0])
+        self.assertIn("main", json.loads(resp.body))
+
+
+if __name__ == "__main__":
+    unittest.main()

py/tests/test_storage_wiring.py

@@ -21,7 +21,8 @@
 import tornado.escape  # noqa E402: gotta install ioloop first
 
 from visdom.utils.shared_utils import warn_once, ensure_dir_exists, get_visdom_path
-from visdom.utils.server_utils import serialize_env, LazyEnvData
+from visdom.utils.server_utils import LazyEnvData
+from visdom.data_model.json_store import JSONStore
 from visdom.server.handlers.socket_handlers import (
     SocketHandler,
     SocketWrap,
@@ -83,6 +84,7 @@ def __init__(
         self.max_old_content = DEFAULT_MAX_OLD_CONTENT
         self.max_text_lines = DEFAULT_MAX_TEXT_LINES
         self.env_path = env_path
+        self.storage = JSONStore(env_path)
         self.state = self.load_state()
         self.layouts = self.load_layouts()
         self.user_settings = self.load_user_settings()
@@ -223,7 +225,7 @@ def load_state(self):
 
         if "main" not in state and "main.json" not in env_jsons:
             state["main"] = {"jsons": {}, "reload": {}}
-            serialize_env(state, ["main"], env_path=self.env_path)
+            self.storage.save_env("main", state["main"])
 
         return state
 

py/visdom/server/app.py

@@ -59,6 +59,7 @@ def initialize(self, app=None):
             self.sources = app.sources
             self.port = app.port
             self.env_path = app.env_path
+            self.storage = app.storage
             self.login_enabled = app.login_enabled
             self.max_text_lines = app.max_text_lines
             self.max_old_content = app.max_old_content

py/visdom/server/handlers/base_handlers.py

@@ -30,8 +30,6 @@
 from visdom.utils.server_utils import (
     check_auth,
     broadcast_envs,
-    serialize_env,
-    serialize_all,
     send_to_sources,
     broadcast,
     escape_eid,
@@ -81,6 +79,7 @@ def initialize(self, app):
         self.sources = app.sources
         self.port = app.port
         self.env_path = app.env_path
+        self.storage = app.storage
         self.login_enabled = app.login_enabled
         self.app = app
         self.readonly = app.readonly
@@ -156,11 +155,11 @@ def on_message(self, message):
                 self.state[msg["eid"]] = copy.deepcopy(self.state[prev_eid])
                 self.state[msg["eid"]]["reload"] = msg["data"]
                 self.eid = msg["eid"]
-                serialize_env(self.state, [self.eid], env_path=self.env_path)
+                self.storage.save_env(self.eid, self.state[self.eid])
 
         elif cmd == "save_all":
             tornado.ioloop.IOLoop.current().run_in_executor(
-                None, serialize_all, self.state, self.env_path
+                None, self.storage.save_all, self.state
             )
 
         elif cmd == "delete_env":

py/visdom/server/handlers/socket_handlers.py

@@ -35,7 +35,6 @@
     register_window,
     gather_envs,
     broadcast_envs,
-    serialize_env,
     escape_eid,
     compare_envs,
     load_env,
@@ -118,6 +117,39 @@ def update_packet(p, args, max_text_lines, max_old_content, max_image_history):
         patch = jsonpatch.make_patch(old_p, p)
         return p, patch.patch
 
+    @staticmethod
+    def update_embeddings_packet(p, args, max_old_content):
+        update_type = args["data"]["update_type"]
+        content_id = get_rand_id()
+        if update_type == "EntitySelected":
+            selected = args["data"]["selected"]
+            p["content"]["selected"] = selected
+            p["contentID"] = content_id
+            # `selected` may not exist yet on the first selection, so use "add"
+            # (which also overwrites when the key is already present).
+            return [
+                {"op": "add", "path": "/content/selected", "value": selected},
+                {"op": "replace", "path": "/contentID", "value": content_id},
+            ]
+        if update_type == "RegionSelected":
+            old_data = p["content"]["data"]
+            new_data = args["data"]["points"]
+            p["old_content"].append(old_data)
+            # Cap retained history to prevent unbounded in-memory growth (#1320).
+            if len(p["old_content"]) > max_old_content:
+                p["old_content"] = p["old_content"][-max_old_content:]
+            p["content"]["data"] = new_data
+            p["content"]["has_previous"] = True
+            p["content"]["selected"] = None
+            p["contentID"] = content_id
+            return [
+                {"op": "replace", "path": "/content/data", "value": new_data},
+                {"op": "add", "path": "/content/has_previous", "value": True},
+                {"op": "add", "path": "/content/selected", "value": None},
+                {"op": "replace", "path": "/contentID", "value": content_id},
+            ]
+        return []
+
     @staticmethod
     def update(p, args, max_text_lines, max_old_content, max_image_history):
         # Update text in window, separated by a line break
@@ -127,20 +159,6 @@ def update(p, args, max_text_lines, max_old_content, max_image_history):
             if len(lines) > max_text_lines:
                 p["content"] = "<br>".join(lines[-max_text_lines:])
             return p
-        if p["type"] == "embeddings":
-            # TODO embeddings updates should be handled outside of the regular
-            # update flow, as update packets are easy to create manually and
-            # expensive to calculate otherwise
-            if args["data"]["update_type"] == "EntitySelected":
-                p["content"]["selected"] = args["data"]["selected"]
-            elif args["data"]["update_type"] == "RegionSelected":
-                p["content"]["selected"] = None
-                p["old_content"].append(p["content"]["data"])
-                if len(p["old_content"]) > max_old_content:
-                    p["old_content"] = p["old_content"][-max_old_content:]
-                p["content"]["has_previous"] = True
-                p["content"]["data"] = args["data"]["points"]
-            return p
         if p["type"] == "image_history":
             utype = args["data"][0]["type"]
             if utype == "image_history":
@@ -325,6 +343,17 @@ def update(p, args, max_text_lines, max_old_content, max_image_history):
 
         return p
 
+    @staticmethod
+    def broadcast_window_update(handler, args, eid, p, diff_packet):
+        broadcast_packet = {
+            "command": "window_update",
+            "win": args["win"],
+            "eid": eid,
+            "content": diff_packet,
+            "version": p.get("version", 1),
+        }
+        broadcast(handler, json.dumps(broadcast_packet, cls=NanSafeEncoder), eid)
+
     @staticmethod
     def wrap_func(handler, args):
         if "win" not in args:
@@ -374,6 +403,14 @@ def wrap_func(handler, args):
             )
             return
 
+        if p["type"] == "embeddings":
+            diff_packet = UpdateHandler.update_embeddings_packet(
+                p, args, handler.max_old_content
+            )
+            UpdateHandler.broadcast_window_update(handler, args, eid, p, diff_packet)
+            handler.write(p["id"])
+            return
+
         p, diff_packet = UpdateHandler.update_packet(
             p,
             args,
@@ -387,14 +424,7 @@ def wrap_func(handler, args):
             broadcast_msg["eid"] = eid
             broadcast(handler, json.dumps(broadcast_msg, cls=NanSafeEncoder), eid)
         else:
-            broadcast_packet = {
-                "command": "window_update",
-                "win": args["win"],
-                "eid": eid,
-                "content": diff_packet,
-                "version": p.get("version", 1),
-            }
-            broadcast(handler, json.dumps(broadcast_packet, cls=NanSafeEncoder), eid)
+            UpdateHandler.broadcast_window_update(handler, args, eid, p, diff_packet)
         handler.write(p["id"])
 
     @check_auth
@@ -502,7 +532,7 @@ def wrap_func(handler, args):
         assert prev_eid in handler.state, "env to be forked doesn't exist"
 
         handler.state[eid] = copy.deepcopy(handler.state[prev_eid])
-        serialize_env(handler.state, [eid], env_path=handler.env_path)
+        handler.storage.save_env(eid, handler.state[eid])
         broadcast_envs(handler)
 
         handler.write(eid)
@@ -608,7 +638,7 @@ def wrap_func(handler, args):
         envs = args["data"]
         envs = [escape_eid(eid) for eid in envs]
         # this drops invalid env ids
-        ret = serialize_env(handler.state, envs, env_path=handler.env_path)
+        ret = handler.storage.save_envs(handler.state, envs)
         handler.write(json.dumps(ret))
 
     @check_auth
@@ -790,8 +820,7 @@ def post(self):
 
         self.state[new_eid] = {"jsons": data["jsons"], "reload": data["reload"]}
 
-        if self.env_path is not None:
-            serialize_env(self.state, [new_eid], env_path=self.env_path)
+        self.storage.save_env(new_eid, self.state[new_eid])
 
         broadcast_envs(self)