diff --git a/.github/workflows/branch.yml b/.github/workflows/branch.yml index b40942e..385d297 100644 --- a/.github/workflows/branch.yml +++ b/.github/workflows/branch.yml @@ -20,7 +20,7 @@ jobs: # NOTE - this doesn't currently work if the PR is coming from a fork, due to limitations in GitHub actions secrets - name: Post PR comment if: failure() - uses: mshick/add-pr-comment@ffd016c7e151d97d69d21a843022fd4cd5b96fe5 # v3.9.0 + uses: mshick/add-pr-comment@8e4927817251f1ff60c001f04568532b38e0b4a0 # v3.11.0 with: message: | ## This PR is against the `main` branch :x: diff --git a/pyproject.toml b/pyproject.toml index 18fe397..2ab98db 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,7 +9,7 @@ readme = "README.md" [tool.poetry.dependencies] python = "^3.11" # use `rev` to pin to a specific hash, or tag, from the git repo -labretriever = {git = "https://github.com/cmatKhan/labretriever.git", rev = "72bbf3b"} +labretriever = {git = "https://github.com/cmatKhan/labretriever.git", rev = "ecaa7e3"} shiny = "^1.4.0" shinywidgets = "^0.7.1" upsetjs-jupyter-widget = "^1.9.0" diff --git a/tests/unit/test_sample_conditions.py b/tests/unit/test_sample_conditions.py new file mode 100644 index 0000000..c0b194d --- /dev/null +++ b/tests/unit/test_sample_conditions.py @@ -0,0 +1,126 @@ +"""Unit tests for ``tfbpshiny.utils.sample_conditions``.""" + +from __future__ import annotations + +from typing import Any + +import pandas as pd +import pytest + +from tfbpshiny.utils.sample_conditions import ( + build_condition_label, + fetch_sample_condition_map, +) + +# ---------- build_condition_label ---------- + + +@pytest.mark.parametrize( + "values,expected", + [ + (["YPD"], "YPD"), + (["ZEV", "P", "45"], "ZEV / P / 45"), + (["ZEV", None, "45"], "ZEV / 45"), + ([None, None], ""), + ([], ""), + ([" spaced "], "spaced"), + (["", " ", "YPD"], "YPD"), + (["nan", "NaN", "YPD"], "YPD"), + ([float("nan"), "YPD"], "YPD"), + ([1, 2.5, "x"], "1 / 2.5 / x"), + ], +) +def test_build_condition_label(values: list[Any], expected: str) -> None: + assert build_condition_label(values) == expected + + +# ---------- fetch_sample_condition_map ---------- + + +class _StubVdb: + """Minimal stub capturing the last-issued SQL and returning a canned DataFrame.""" + + def __init__(self, df: pd.DataFrame) -> None: + self._df = df + self.last_sql: str | None = None + + def query(self, sql: str, **params: Any) -> pd.DataFrame: + self.last_sql = sql + return self._df + + +def test_fetch_sample_condition_map_empty_cols_returns_empty() -> None: + vdb = _StubVdb(pd.DataFrame()) + assert fetch_sample_condition_map(vdb, "anything", []) == {} + # No query should have been issued. + assert vdb.last_sql is None + + +def test_fetch_sample_condition_map_single_column() -> None: + df = pd.DataFrame( + { + "sample_id": ["s1", "s2", "s3"], + "Experimental condition": ["YPD", "HEAT", None], + } + ) + vdb = _StubVdb(df) + result = fetch_sample_condition_map(vdb, "harbison", ["Experimental condition"]) + + assert result == {"s1": "YPD", "s2": "HEAT"} + # s3 had an all-NULL label and must not be present. + assert "s3" not in result + # Column with a space is double-quoted in the SQL. + assert vdb.last_sql is not None + assert '"Experimental condition"' in vdb.last_sql + assert "FROM harbison_meta" in vdb.last_sql + + +def test_fetch_sample_condition_map_multi_column_joined() -> None: + df = pd.DataFrame( + { + "sample_id": ["s1", "s2"], + "mechanism": ["ZEV", "GEV"], + "restriction": ["P", None], + } + ) + vdb = _StubVdb(df) + result = fetch_sample_condition_map(vdb, "hackett", ["mechanism", "restriction"]) + + assert result == {"s1": "ZEV / P", "s2": "GEV"} + + +def test_fetch_sample_condition_map_non_string_sample_id_coerced() -> None: + # sample_id comes back from DuckDB as int or bytes in some schemas. + df = pd.DataFrame({"sample_id": [1, 2], "cond": ["A", "B"]}) + vdb = _StubVdb(df) + result = fetch_sample_condition_map(vdb, "ds", ["cond"]) + assert result == {"1": "A", "2": "B"} + + +@pytest.mark.parametrize( + "bad_name", + [ + "foo; DROP TABLE users", + "foo-bar", + "foo bar", + "1foo", + "", + 'foo"; SELECT', + ], +) +def test_fetch_sample_condition_map_rejects_unsafe_db_name(bad_name: str) -> None: + vdb = _StubVdb(pd.DataFrame({"sample_id": ["s1"], "cond": ["A"]})) + with pytest.raises(ValueError, match="safe SQL identifier"): + fetch_sample_condition_map(vdb, bad_name, ["cond"]) + + +def test_fetch_sample_condition_map_escapes_embedded_quotes_in_cols() -> None: + # Column name with an embedded double-quote must be escaped as "" per + # DuckDB identifier rules; otherwise the SQL would break out of quoting. + df = pd.DataFrame({"sample_id": ["s1"], 'weird"name': ["A"]}) + vdb = _StubVdb(df) + result = fetch_sample_condition_map(vdb, "ds", ['weird"name']) + assert result == {"s1": "A"} + assert vdb.last_sql is not None + # The emitted identifier is "weird""name" — one escaped embedded quote. + assert '"weird""name"' in vdb.last_sql diff --git a/tfbpshiny/app.py b/tfbpshiny/app.py index 60e4eed..ae5ca4e 100644 --- a/tfbpshiny/app.py +++ b/tfbpshiny/app.py @@ -137,6 +137,7 @@ def app_server(input: Any, output: Any, session: Any) -> None: col_preference=col_preference, dataset_filters=dataset_filters, vdb=vdb, + app_datasets=app_datasets, logger=logger, ) @@ -154,6 +155,7 @@ def app_server(input: Any, output: Any, session: Any) -> None: col_preference=col_preference_p, dataset_filters=dataset_filters, vdb=vdb, + app_datasets=app_datasets, logger=logger, ) diff --git a/tfbpshiny/modules/binding/page_test.py b/tfbpshiny/modules/binding/page_test.py index d6b784c..c55b1e6 100644 --- a/tfbpshiny/modules/binding/page_test.py +++ b/tfbpshiny/modules/binding/page_test.py @@ -24,6 +24,7 @@ binding_workspace_server, ) from tfbpshiny.modules.binding.ui import binding_sidebar_ui, binding_workspace_ui +from tfbpshiny.utils.vdb_init import AppDatasets logger = logging.getLogger("shiny") @@ -157,6 +158,7 @@ def active_binding_datasets() -> list[str]: col_preference=col_preference, dataset_filters=_dataset_filters, vdb=vdb, + app_datasets=AppDatasets(condition_cols={}, upstream_cols={}), logger=logger, ) diff --git a/tfbpshiny/modules/binding/server/workspace.py b/tfbpshiny/modules/binding/server/workspace.py index fcb731b..c31f727 100644 --- a/tfbpshiny/modules/binding/server/workspace.py +++ b/tfbpshiny/modules/binding/server/workspace.py @@ -2,6 +2,7 @@ import itertools from collections.abc import Callable +from html import escape from logging import Logger from typing import Any, Literal @@ -16,7 +17,8 @@ get_measurement_column, regulator_scatter_sql, ) -from tfbpshiny.utils.vdb_init import get_regulator_display_name +from tfbpshiny.utils.sample_conditions import fetch_sample_condition_map +from tfbpshiny.utils.vdb_init import AppDatasets, get_regulator_display_name @module.server @@ -29,6 +31,7 @@ def binding_workspace_server( col_preference: Callable[[], str], dataset_filters: reactive.Value[dict[str, Any]], vdb: VirtualDB, + app_datasets: AppDatasets, logger: Logger, ) -> None: """ @@ -46,6 +49,35 @@ def binding_workspace_server( zip(_reg_df["regulator_locus_tag"], _reg_df["display_name"]) ) + @reactive.calc + def _condition_maps() -> dict[str, dict[str, str]]: + """ + ``{db_name: {sample_id: label}}`` for each active dataset that has + experimental_condition columns. + + Used to annotate tooltips on the selected-regulator overlay in the + distribution plot so the user can distinguish multiple samples of the + same regulator. Datasets without ``condition_cols`` are omitted from + the outer dict, causing the tooltip to skip their side. + + :trigger active_binding_datasets: re-runs when the user toggles a + binding dataset on or off. + :returns: Outer dict keyed by db_name; inner dict maps sample_id to + the joined condition label. + + """ + out: dict[str, dict[str, str]] = {} + for db in active_binding_datasets(): + cols = app_datasets.condition_cols.get(db, []) + if not cols: + continue + try: + out[db] = fetch_sample_condition_map(vdb, db, cols) + except Exception: + logger.exception("Failed to fetch condition map for %s", db) + out[db] = {} + return out + @reactive.calc def _pairs() -> list[tuple[str, str]]: """ @@ -169,6 +201,8 @@ def distributions_plot() -> ui.Tag: except Exception: selected_reg = "" + cond_maps = _condition_maps() + # Build a single combined box trace using x as the category axis. # Each point's x value is the pair label; Plotly groups points under # each category and draws one box per unique x value. @@ -185,10 +219,15 @@ def distributions_plot() -> ui.Tag: label_a = display_names.get(db_a, db_a) label_b = display_names.get(db_b, db_b) pair_label = f"{label_a}
vs
{label_b}" + cond_a = cond_maps.get(db_a, {}) + cond_b = cond_maps.get(db_b, {}) if not df.empty: df_clean = df.dropna(subset=["correlation"]) - for tag, corr in zip( - df_clean["regulator_locus_tag"], df_clean["correlation"] + for tag, corr, sample_a, sample_b in zip( + df_clean["regulator_locus_tag"], + df_clean["correlation"], + df_clean["db_a_id"], + df_clean["db_b_id"], ): display = sym_map.get(tag, tag) all_x.append(pair_label) @@ -198,7 +237,24 @@ def distributions_plot() -> ui.Tag: if tag == selected_reg: sel_x.append(pair_label) sel_y.append(corr) - sel_hover.append(display) + # Per-dot hover: regulator + r + one condition line per + # dataset that has a non-empty label for this sample. + # All DB-sourced strings are HTML-escaped before being + # joined with the
separators because Plotly renders + # hovertext as HTML (stored-XSS sink if any researcher- + # uploaded metadata ever contained markup). + hover_lines = [escape(display), f"r = {corr:.3f}"] + label_sample_a = cond_a.get(str(sample_a), "") + if label_sample_a: + hover_lines.append( + f"{escape(label_a)}: {escape(label_sample_a)}" + ) + label_sample_b = cond_b.get(str(sample_b), "") + if label_sample_b: + hover_lines.append( + f"{escape(label_b)}: {escape(label_sample_b)}" + ) + sel_hover.append("
".join(hover_lines)) sel_tags.append(tag) fig.add_trace( @@ -224,9 +280,9 @@ def distributions_plot() -> ui.Tag: x=sel_x, y=sel_y, mode="markers", - text=sel_hover, + hovertext=sel_hover, customdata=sel_tags, - hovertemplate="%{text}
r = %{y:.3f}", + hovertemplate="%{hovertext}", marker=dict(size=10, color="black", symbol="circle"), showlegend=False, ) diff --git a/tfbpshiny/modules/perturbation/page_test.py b/tfbpshiny/modules/perturbation/page_test.py index af8f531..ccce01d 100644 --- a/tfbpshiny/modules/perturbation/page_test.py +++ b/tfbpshiny/modules/perturbation/page_test.py @@ -28,6 +28,7 @@ perturbation_sidebar_ui, perturbation_workspace_ui, ) +from tfbpshiny.utils.vdb_init import AppDatasets logger = logging.getLogger("shiny") @@ -165,6 +166,7 @@ def active_perturbation_datasets() -> list[str]: col_preference=col_preference, dataset_filters=_dataset_filters, vdb=vdb, + app_datasets=AppDatasets(condition_cols={}, upstream_cols={}), logger=logger, ) diff --git a/tfbpshiny/modules/perturbation/server/workspace.py b/tfbpshiny/modules/perturbation/server/workspace.py index d799979..927c984 100644 --- a/tfbpshiny/modules/perturbation/server/workspace.py +++ b/tfbpshiny/modules/perturbation/server/workspace.py @@ -2,6 +2,7 @@ import itertools from collections.abc import Callable +from html import escape from logging import Logger from typing import Any, Literal @@ -16,7 +17,8 @@ get_measurement_column, regulator_scatter_sql, ) -from tfbpshiny.utils.vdb_init import get_regulator_display_name +from tfbpshiny.utils.sample_conditions import fetch_sample_condition_map +from tfbpshiny.utils.vdb_init import AppDatasets, get_regulator_display_name @module.server @@ -29,6 +31,7 @@ def perturbation_workspace_server( col_preference: Callable[[], str], dataset_filters: reactive.Value[dict[str, Any]], vdb: VirtualDB, + app_datasets: AppDatasets, logger: Logger, ) -> None: """ @@ -46,6 +49,35 @@ def perturbation_workspace_server( zip(_reg_df["regulator_locus_tag"], _reg_df["display_name"]) ) + @reactive.calc + def _condition_maps() -> dict[str, dict[str, str]]: + """ + ``{db_name: {sample_id: label}}`` for each active dataset that has + experimental_condition columns. + + Used to annotate tooltips on the selected-regulator overlay in the + distribution plot so the user can distinguish multiple samples of the + same regulator. Datasets without ``condition_cols`` are omitted from + the outer dict, causing the tooltip to skip their side. + + :trigger active_perturbation_datasets: re-runs when the user toggles + a perturbation dataset on or off. + :returns: Outer dict keyed by db_name; inner dict maps sample_id to + the joined condition label. + + """ + out: dict[str, dict[str, str]] = {} + for db in active_perturbation_datasets(): + cols = app_datasets.condition_cols.get(db, []) + if not cols: + continue + try: + out[db] = fetch_sample_condition_map(vdb, db, cols) + except Exception: + logger.exception("Failed to fetch condition map for %s", db) + out[db] = {} + return out + @reactive.calc def _pairs() -> list[tuple[str, str]]: """ @@ -167,6 +199,8 @@ def distributions_plot() -> ui.Tag: except Exception: selected_reg = "" + cond_maps = _condition_maps() + # Build a single combined box trace using x as the category axis. # Each point's x value is the pair label; Plotly groups points under # each category and draws one box per unique x value. @@ -183,10 +217,15 @@ def distributions_plot() -> ui.Tag: label_a = display_names.get(db_a, db_a) label_b = display_names.get(db_b, db_b) pair_label = f"{label_a}
vs
{label_b}" + cond_a = cond_maps.get(db_a, {}) + cond_b = cond_maps.get(db_b, {}) if not df.empty: df_clean = df.dropna(subset=["correlation"]) - for tag, corr in zip( - df_clean["regulator_locus_tag"], df_clean["correlation"] + for tag, corr, sample_a, sample_b in zip( + df_clean["regulator_locus_tag"], + df_clean["correlation"], + df_clean["db_a_id"], + df_clean["db_b_id"], ): display = sym_map.get(tag, tag) all_x.append(pair_label) @@ -196,7 +235,24 @@ def distributions_plot() -> ui.Tag: if tag == selected_reg: sel_x.append(pair_label) sel_y.append(corr) - sel_hover.append(display) + # Per-dot hover: regulator + r + one condition line per + # dataset that has a non-empty label for this sample. + # All DB-sourced strings are HTML-escaped before being + # joined with the
separators because Plotly renders + # hovertext as HTML (stored-XSS sink if any researcher- + # uploaded metadata ever contained markup). + hover_lines = [escape(display), f"r = {corr:.3f}"] + label_sample_a = cond_a.get(str(sample_a), "") + if label_sample_a: + hover_lines.append( + f"{escape(label_a)}: {escape(label_sample_a)}" + ) + label_sample_b = cond_b.get(str(sample_b), "") + if label_sample_b: + hover_lines.append( + f"{escape(label_b)}: {escape(label_sample_b)}" + ) + sel_hover.append("
".join(hover_lines)) sel_tags.append(tag) fig.add_trace( @@ -222,9 +278,9 @@ def distributions_plot() -> ui.Tag: x=sel_x, y=sel_y, mode="markers", - text=sel_hover, + hovertext=sel_hover, customdata=sel_tags, - hovertemplate="%{text}
r = %{y:.3f}", + hovertemplate="%{hovertext}", marker=dict(size=10, color="black", symbol="circle"), showlegend=False, ) diff --git a/tfbpshiny/modules/select_datasets/export.py b/tfbpshiny/modules/select_datasets/export.py index 5d7a90e..9771f7f 100644 --- a/tfbpshiny/modules/select_datasets/export.py +++ b/tfbpshiny/modules/select_datasets/export.py @@ -140,9 +140,13 @@ def build_export_tarball( :func:`_query_to_csv_bytes` so only one dataset's data is in memory at once. - Uses ``tarfile`` pipe mode (``w|gz``) for streaming writes into an - in-memory ``BytesIO`` buffer — no temp files on disk. The full buffer - is returned to the caller for chunked yielding. + Uses ``tarfile`` non-pipe mode (``w:gz``) with a seekable in-memory + ``BytesIO`` buffer — no temp files on disk. The full buffer is returned + to the caller for chunked yielding. Gzip compression is pinned to + ``compresslevel=1`` to favour export speed over archive size; the + absolute speedup and size penalty depend on the dataset mix (see + issue #242). ``compresslevel`` is not accepted in pipe mode (``w|gz``), + which is why non-pipe mode is used here. A thread-safe DuckDB cursor is created via ``vdb._conn.cursor()`` so this function can safely run in a worker thread while the main event @@ -161,7 +165,7 @@ def build_export_tarball( out = io.BytesIO() try: - with tarfile.open(mode="w|gz", fileobj=out) as tar: + with tarfile.open(mode="w:gz", fileobj=out, compresslevel=1) as tar: for ds in datasets: dir_name = _safe_dir_name(ds.display_name) diff --git a/tfbpshiny/utils/sample_conditions.py b/tfbpshiny/utils/sample_conditions.py new file mode 100644 index 0000000..8748b5e --- /dev/null +++ b/tfbpshiny/utils/sample_conditions.py @@ -0,0 +1,97 @@ +""" +Helpers for attaching experimental-condition labels to individual samples. + +The binding and perturbation distribution plots show one dot per sample pair. When +a regulator has multiple samples in a dataset the plot produces multiple dots for +that regulator, and the user needs a way to tell them apart in the tooltip. This +module builds ``{sample_id: condition_label}`` lookups from a dataset's ``_meta`` +view so that the plot code can append human-readable condition text to each +selected-overlay dot's hover string. + +``build_condition_label`` is a pure function and is the main unit-testable +surface. ``fetch_sample_condition_map`` is a thin VirtualDB wrapper that assembles +the SQL, runs it, and applies the label builder row-by-row. + +""" + +from __future__ import annotations + +import re +from typing import Any + +from labretriever import VirtualDB + + +def build_condition_label(values: list[Any]) -> str: + """ + Combine one or more raw condition-column values into a single label. + + Non-empty, non-NaN string representations are joined with ``" / "``. A + value of ``None``, a ``NaN`` float, or a string that is empty / whitespace + / the literal ``"nan"`` (case-insensitive) is dropped. When every value + drops out, an empty string is returned. + + :param values: One or more raw values from a single meta-table row. + :returns: Display label, or ``""`` when nothing useful remains. + + """ + parts: list[str] = [] + for v in values: + if v is None: + continue + # Catch float NaN without requiring a pandas/numpy import here. + if isinstance(v, float) and v != v: + continue + s = str(v).strip() + if not s or s.lower() == "nan": + continue + parts.append(s) + return " / ".join(parts) + + +_SAFE_IDENT_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def fetch_sample_condition_map( + vdb: VirtualDB, db_name: str, cols: list[str] +) -> dict[str, str]: + """ + Build a ``{sample_id: condition_label}`` map for one dataset. + + Queries ``{db_name}_meta`` for ``sample_id`` plus the requested condition + columns and composes each row's label with :func:`build_condition_label`. + Samples whose composed label is empty are omitted. + + Identifier safety: ``db_name`` must match a SQL-identifier pattern + (``[A-Za-z_][A-Za-z0-9_]*``), otherwise a ``ValueError`` is raised before + any SQL is built. Column names are double-quoted with embedded quotes + doubled (DuckDB's identifier-escape), so columns containing spaces are + fine but a column containing ``"`` cannot break out of the quoting. + + :param vdb: VirtualDB instance. + :param db_name: Dataset name (the base name; ``_meta`` is appended). + Must be a valid SQL identifier. + :param cols: Condition column names, as taken from + ``AppDatasets.condition_cols[db_name]``. May include spaces — each + column is double-quoted in the generated SQL. + :returns: Mapping from ``sample_id`` to its joined condition label. Empty + when ``cols`` is empty. + :raises ValueError: If ``db_name`` is not a safe identifier. + + """ + if not cols: + return {} + if not _SAFE_IDENT_RE.match(db_name): + raise ValueError(f"db_name is not a safe SQL identifier: {db_name!r}") + quoted = ", ".join(f'"{c.replace(chr(34), chr(34) * 2)}"' for c in cols) + sql = f"SELECT sample_id, {quoted} FROM {db_name}_meta" + df = vdb.query(sql) + result: dict[str, str] = {} + for _, row in df.iterrows(): + label = build_condition_label([row[c] for c in cols]) + if label: + result[str(row["sample_id"])] = label + return result + + +__all__ = ["build_condition_label", "fetch_sample_condition_map"]