Merge branch 'dev-define-engines-abc' into dev-define-nucleus-detection-engine

Author: shaneahmed

tests/conftest.py

@@ -4,7 +4,9 @@
 
 import os
 import shutil
+import socket
 import time
+from contextlib import closing
 from pathlib import Path
 from typing import TYPE_CHECKING
 
@@ -16,6 +18,13 @@
 from tiatoolbox.data import _fetch_remote_sample
 from tiatoolbox.utils.env_detection import has_gpu, running_on_ci
 
+# Reserve a free port for tileserver tests
+_TILES_ENV = "TIATOOLBOX_TILESERVER_PORT"
+if _TILES_ENV not in os.environ:
+    with closing(socket.socket(socket.AF_INET, socket.SOCK_STREAM)) as sock:
+        sock.bind(("127.0.0.1", 0))
+        os.environ[_TILES_ENV] = str(sock.getsockname()[1])
+
 if TYPE_CHECKING:
     from collections.abc import Callable
 

tests/test_app_bokeh.py

@@ -9,6 +9,7 @@
 import re
 import time
 from pathlib import Path
+from types import SimpleNamespace
 from typing import TYPE_CHECKING
 
 import bokeh.models as bkmodels
@@ -25,7 +26,7 @@
 from scipy.ndimage import label
 
 from tiatoolbox.data import _fetch_remote_sample
-from tiatoolbox.visualization.bokeh_app import main
+from tiatoolbox.visualization.bokeh_app import app_hooks, main
 from tiatoolbox.visualization.tileserver import TileServer
 from tiatoolbox.visualization.ui_utils import get_level_by_extent
 
@@ -41,6 +42,13 @@
 GRIDLINES = 2
 
 
+class _DummySessionContext:
+    """Simple shim matching the subset of Bokeh's SessionContext we use."""
+
+    def __init__(self: _DummySessionContext, user: str) -> None:
+        self.request = SimpleNamespace(arguments={"user": user})
+
+
 # helper functions and fixtures
 def get_tile(layer: str, x: float, y: float, z: float, *, show: bool) -> np.ndarray:
     """Get a tile from the server.
@@ -77,7 +85,9 @@ def get_renderer_prop(prop: str) -> json:
             The property to get.
 
     """
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/renderer/{prop}")
+    resp = main.UI["s"].get(
+        f"http://{main.host2}:{main.port}/tileserver/renderer/{prop}",
+    )
     return resp.json()
 
 
@@ -144,7 +154,7 @@ def run_app() -> None:
         layers={},
     )
     CORS(app, send_wildcard=True)
-    app.run(host="127.0.0.1", threaded=True)
+    app.run(host="127.0.0.1", port=int(main.port), threaded=True)
 
 
 @pytest.fixture(scope="module")
@@ -376,13 +386,17 @@ def test_type_cmap_select(doc: Document) -> None:
 
     # remove the type cmap
     cmap_select.value = []
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/secondary_cmap")
+    resp = main.UI["s"].get(
+        f"http://{main.host2}:{main.port}/tileserver/secondary_cmap"
+    )
     assert resp.json()["score_prop"] == "None"
 
     # check callback works regardless of order
     cmap_select.value = ["0"]
     cmap_select.value = ["0", "prob"]
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/secondary_cmap")
+    resp = main.UI["s"].get(
+        f"http://{main.host2}:{main.port}/tileserver/secondary_cmap"
+    )
     assert resp.json()["score_prop"] == "prob"
 
 
@@ -706,24 +720,24 @@ def test_cmap_select(doc: Document) -> None:
     main.UI["cprop_input"].value = ["prob"]
     # set to jet
     cmap_select.value = "jet"
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/cmap")
+    resp = main.UI["s"].get(f"http://{main.host2}:{main.port}/tileserver/cmap")
     assert resp.json() == "jet"
     # set to dict
     cmap_select.value = "dict"
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/cmap")
+    resp = main.UI["s"].get(f"http://{main.host2}:{main.port}/tileserver/cmap")
     assert isinstance(resp.json(), dict)
 
     main.UI["cprop_input"].value = ["type"]
     # should now be the type mapping
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/cmap")
+    resp = main.UI["s"].get(f"http://{main.host2}:{main.port}/tileserver/cmap")
     for key in main.UI["vstate"].mapper:
         assert str(key) in resp.json()
         assert np.all(
             np.array(resp.json()[str(key)]) == np.array(main.UI["vstate"].mapper[key]),
         )
     # set the cmap to "coolwarm"
     cmap_select.value = "coolwarm"
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/cmap")
+    resp = main.UI["s"].get(f"http://{main.host2}:{main.port}/tileserver/cmap")
     # as cprop is type (categorical), it should have had no effect
     for key in main.UI["vstate"].mapper:
         assert str(key) in resp.json()
@@ -732,7 +746,7 @@ def test_cmap_select(doc: Document) -> None:
         )
 
     main.UI["cprop_input"].value = ["prob"]
-    resp = main.UI["s"].get(f"http://{main.host2}:5000/tileserver/cmap")
+    resp = main.UI["s"].get(f"http://{main.host2}:{main.port}/tileserver/cmap")
     # should be coolwarm as that is the last cmap we set, and prob is continuous
     assert resp.json() == "coolwarm"
 
@@ -778,3 +792,51 @@ def test_clearing_doc(doc: Document) -> None:
     """Test that the doc can be cleared."""
     doc.clear()
     assert len(doc.roots) == 0
+
+
+def test_app_hooks_session_destroyed(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Hook should call reset endpoint and exit."""
+    recorded: dict[str, object] = {}
+
+    def fake_get(url: str, *, timeout: int) -> None:
+        """Fake requests.get to record parameters."""
+        recorded["url"] = url
+        recorded["timeout"] = timeout
+
+    monkeypatch.setattr(app_hooks, "PORT", "6150")
+    monkeypatch.setattr(app_hooks.requests, "get", fake_get)
+    exited = False
+
+    def fake_exit() -> None:
+        """Fake sys.exit to record call."""
+        nonlocal exited
+        exited = True
+
+    monkeypatch.setattr(app_hooks, "sys", SimpleNamespace(exit=fake_exit))
+    app_hooks.on_session_destroyed(_DummySessionContext("user-1"))
+    assert recorded["url"] == "http://127.0.0.1:6150/tileserver/reset/user-1"
+    assert recorded["timeout"] == 5
+    assert exited
+
+
+def test_app_hooks_session_destroyed_suppresses_timeout(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
+    """ReadTimeout should be suppressed and exit still called."""
+
+    def fake_get(*_: object, **__: object) -> None:
+        """Fake requests.get to raise ReadTimeout."""
+        raise app_hooks.requests.exceptions.ReadTimeout  # type: ignore[attr-defined]
+
+    monkeypatch.setattr(app_hooks, "PORT", "6160")
+    monkeypatch.setattr(app_hooks.requests, "get", fake_get)
+    exited = False
+
+    def fake_exit() -> None:
+        """Fake sys.exit to record call."""
+        nonlocal exited
+        exited = True
+
+    monkeypatch.setattr(app_hooks, "sys", SimpleNamespace(exit=fake_exit))
+    app_hooks.on_session_destroyed(_DummySessionContext("user-2"))
+    assert exited

tests/test_json_config_bokeh.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import os
 import time
 from contextlib import suppress
 from threading import Thread
@@ -14,6 +15,8 @@
 from tiatoolbox.cli.visualize import run_bokeh, run_tileserver
 from tiatoolbox.data import _fetch_remote_sample
 
+PORT = os.environ.get("TIATOOLBOX_TILESERVER_PORT", "5000")
+
 if TYPE_CHECKING:
     from pathlib import Path
 
@@ -65,7 +68,10 @@ def bk_session(data_path: dict[str, Path]) -> ClientSession:
     yield session
     session.close()
     with suppress(requests.exceptions.ConnectionError):
-        requests.post("http://localhost:5000/tileserver/shutdown", timeout=2)
+        requests.post(
+            f"http://localhost:{PORT}/tileserver/shutdown",
+            timeout=2,
+        )
 
 
 def test_slides_available(bk_session: ClientSession) -> None:

tests/test_server_bokeh.py

@@ -2,6 +2,7 @@
 
 from __future__ import annotations
 
+import os
 import time
 from contextlib import suppress
 from threading import Thread
@@ -16,6 +17,8 @@
 from tiatoolbox.cli.visualize import run_bokeh, run_tileserver
 from tiatoolbox.data import _fetch_remote_sample
 
+PORT = os.environ.get("TIATOOLBOX_TILESERVER_PORT", "5000")
+
 if TYPE_CHECKING:
     from pathlib import Path
 
@@ -72,7 +75,10 @@ def bk_session(data_path: dict[str, Path]) -> ClientSession:
     yield session
     session.close()
     with suppress(requests.exceptions.ConnectionError):
-        requests.post("http://localhost:5000/tileserver/shutdown", timeout=2)
+        requests.post(
+            f"http://localhost:{PORT}/tileserver/shutdown",
+            timeout=2,
+        )
 
 
 def test_slides_available(bk_session: ClientSession) -> None:

tiatoolbox/annotation/storage.py

@@ -2861,6 +2861,33 @@ def _initialize_query_string_parameters(
 
         return query_string, query_parameters
 
+    @staticmethod
+    def _warn_if_query_not_using_index(
+        cur: sqlite3.Cursor,
+        query_string: str,
+        query_parameters: dict[str, object],
+    ) -> None:
+        """Log a warning when SQLite does not use an index."""
+        query_plan = cur.execute(
+            "EXPLAIN QUERY PLAN " + query_string,
+            query_parameters,
+        ).fetchall()
+        uses_index = False
+        for plan in query_plan:
+            detail = str(plan[-1]).upper()
+            # macOS SQLite may report "USING COVERING INDEX".
+            if "AUTOMATIC" not in detail and (
+                "USING INDEX" in detail or "USING COVERING INDEX" in detail
+            ):
+                uses_index = True
+                break
+        if not uses_index:
+            logger.warning(
+                "Query is not using an index. "
+                "Consider adding an index to improve performance.",
+                stacklevel=2,
+            )
+
     def _query(
         self: SQLiteStore,
         columns: str,
@@ -2970,16 +2997,7 @@ def _query(
 
         # Warn if the query is not using an index
         if index_warning:
-            query_plan = cur.execute(
-                "EXPLAIN QUERY PLAN " + query_string,
-                query_parameters,
-            ).fetchone()
-            if "USING INDEX" not in query_plan[-1]:
-                logger.warning(
-                    "Query is not using an index. "
-                    "Consider adding an index to improve performance.",
-                    stacklevel=2,
-                )
+            self._warn_if_query_not_using_index(cur, query_string, query_parameters)
         # if area column exists, sort annotations by area
         if "area" in self.table_columns:
             query_string += "\nORDER BY area DESC"

tiatoolbox/cli/visualize.py

@@ -26,7 +26,8 @@ def run_app() -> None:
             layers={},
         )
         CORS(app, send_wildcard=True)
-        app.run(host="127.0.0.1", threaded=True)
+        port = int(os.environ.get("TIATOOLBOX_TILESERVER_PORT", "5000"))
+        app.run(host="127.0.0.1", port=port, threaded=True)
 
     proc = Thread(target=run_app, daemon=True)
     proc.start()

tiatoolbox/visualization/bokeh_app/app_hooks.py

@@ -1,15 +1,21 @@
 """Hooks to be executed upon specific events in bokeh app."""
 
+import os
 import sys
 from contextlib import suppress
 
 import requests
 from bokeh.application.application import SessionContext
 
+PORT = os.environ.get("TIATOOLBOX_TILESERVER_PORT", "5000")
+
 
 def on_session_destroyed(session_context: SessionContext) -> None:
     """Hook to be executed when a session is destroyed."""
     user = session_context.request.arguments["user"]
     with suppress(requests.exceptions.ReadTimeout):
-        requests.get(f"http://127.0.0.1:5000/tileserver/reset/{user}", timeout=5)
+        requests.get(
+            f"http://127.0.0.1:{PORT}/tileserver/reset/{user}",
+            timeout=5,
+        )
     sys.exit()