From ad3fd335ae79316563859d15cbd73df2fedbc63d Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Sun, 6 Sep 2026 22:17:32 -0600 Subject: [PATCH 1/2] feat(py): handle store for tool results A tool result is stored under `r1`, `r2`, ... so a later `run_python` call can build on it as a plain variable rather than repeating the work. `HandleStore.register()` returns the note the model gets back, caps a stored frame at 10,000 rows and says so, and registers values that are not frames too, so a scalar measure result stays available for further derivation. The note describes a frame the way `ellmer::df_schema()` does for the R agent: shape, then per column the dtype and missing count, with ranges for numeric and temporal columns, True and False counts for booleans, and unique values for the rest when there are few enough to be worth printing. That description lives in `_frames.py`, which reads pandas and polars frames through what they offer rather than by importing either, and now owns the frame predicate `_data_source.py` had. `tests/shared/handles.json` pins what both packages must agree on: the id sequence, that a value which is not a frame gets one, and the row cap with the sentence that states it. The rest of the note is each package's own, since the tool that reaches a handle is `run_r` in R and `run_python` here. --- pkg-py/src/commons/_data_source.py | 11 +- pkg-py/src/commons/_frames.py | 126 ++++++++++++ pkg-py/src/commons/_handles.py | 57 ++++++ pkg-py/tests/test_handles.py | 181 ++++++++++++++++++ .../testthat/fixtures/shared/handles.json | 62 ++++++ pkg-r/tests/testthat/test-handles.R | 51 +++++ tests/shared/README.md | 1 + tests/shared/handles.json | 62 ++++++ 8 files changed, 543 insertions(+), 8 deletions(-) create mode 100644 pkg-py/src/commons/_frames.py create mode 100644 pkg-py/src/commons/_handles.py create mode 100644 pkg-py/tests/test_handles.py create mode 100644 pkg-r/tests/testthat/fixtures/shared/handles.json create mode 100644 pkg-r/tests/testthat/test-handles.R create mode 100644 tests/shared/handles.json diff --git a/pkg-py/src/commons/_data_source.py b/pkg-py/src/commons/_data_source.py index 46f4d749..ee50e997 100644 --- a/pkg-py/src/commons/_data_source.py +++ b/pkg-py/src/commons/_data_source.py @@ -15,6 +15,7 @@ from . import _duckdb from ._backends import Backend, DuckDBBackend, EngineBackend +from ._frames import is_frame from ._sql_guard import check_query if TYPE_CHECKING: @@ -347,7 +348,7 @@ def _load_pins(self, labels: list[str]) -> None: for position, label in enumerate(labels): pin = self.pending.pins[label] value = self.pending.board.pin_read(pin) - if not _is_frame(value): + if not is_frame(value): raise TypeError( f"Pin {pin!r} is a {type(value).__name__}, not a data frame, " f"so it cannot become the table {label!r}." @@ -465,19 +466,13 @@ def _check_named_frames(frames: dict[str, Any]) -> None: "or a pins board." ) for name, frame in frames.items(): - if not _is_frame(frame): + if not is_frame(frame): raise TypeError( f"{name} must be a pandas or polars data frame, got " f"{type(frame).__name__}." ) -def _is_frame(value: Any) -> bool: - # Duck-typed rather than imported: pandas and polars are both optional at - # this boundary, and DuckDB accepts either through the same registration. - return hasattr(value, "__dataframe__") or hasattr(value, "columns") - - def normalize_table_registry(tables: Any) -> dict[str, TableId]: """Turn a `tables` argument into label -> `TableId`. diff --git a/pkg-py/src/commons/_frames.py b/pkg-py/src/commons/_frames.py new file mode 100644 index 00000000..4195c9c5 --- /dev/null +++ b/pkg-py/src/commons/_frames.py @@ -0,0 +1,126 @@ +"""Recognizing and describing a data frame, whichever library it came from. + +pandas and polars are both optional at every boundary that accepts a frame, +so neither is imported here: a frame is recognized and read through what it +offers rather than through its class. +""" + +from __future__ import annotations + +from typing import Any + +__all__ = ["describe_frame", "is_frame"] + + +def is_frame(value: Any) -> bool: + return hasattr(value, "__dataframe__") or hasattr(value, "columns") + + +# ellmer's `df_schema()` describes a frame for the R agent; this describes one +# for the Python agent, in the same terms, for whichever frame library the +# result came from. +MAX_SUMMARY_COLUMNS = 50 + + +def describe_frame(frame: Any, max_columns: int = MAX_SUMMARY_COLUMNS) -> str: + """A column-by-column description, so the model can write code against it.""" + names = list(frame.columns) + shape = f"{_count(len(frame), 'row')} and {_count(len(names), 'column')}" + lines = [f"A data frame with {shape}:"] + lines += [ + f"* {name}: {_describe_column(frame[name])}" for name in names[:max_columns] + ] + if len(names) > max_columns: + lines.append(f"and {len(names) - max_columns} more columns") + return "\n".join(lines) + + +def _count(number: int, noun: str) -> str: + return f"{number:,} {noun}" if number == 1 else f"{number:,} {noun}s" + + +def _describe_column(column: Any) -> str: + kind = _kind(column.dtype) + missing = _missing(column) + described = f"{missing} missing" + if kind in ("numeric", "temporal"): + # A column with nothing left to take a range over says only how much + # is missing. + properties = ( + [described] + if missing == len(column) + else [ + f"range [{_value(column.min())}, {_value(column.max())}]", + described, + ] + ) + elif kind == "boolean": + true = int(column.sum()) + properties = [ + f"{true} True", + f"{len(column) - missing - true} False", + described, + ] + else: + properties = [described, _describe_values(column)] + return f"{column.dtype} with {_flatten(properties)}" + + +# pandas dtypes carry a numpy `kind` character; polars dtypes answer questions +# about themselves instead. Neither library is imported here, because both are +# optional wherever a frame reaches commons. +def _kind(dtype: Any) -> str: + kind = getattr(dtype, "kind", None) + if kind is not None: + if kind in "iuf": + return "numeric" + if kind == "b": + return "boolean" + if kind in "Mm": + return "temporal" + return "other" + if dtype.is_numeric(): + return "numeric" + if dtype.is_temporal(): + return "temporal" + if str(dtype) == "Boolean": + return "boolean" + return "other" + + +def _missing(column: Any) -> int: + if hasattr(column, "isna"): + return int(column.isna().sum()) + return int(column.null_count()) + + +# Like ellmer: the values themselves only when there are few and they are +# short, so a column of free text stays a count rather than a wall of prompt. +def _describe_values(column: Any) -> str: + values = _unique(column) + described = _count(len(values), "unique value") + quoted = [f'"{value}"' for value in values] + if 0 < len(values) <= 10 and sum(len(value) for value in quoted) < 200: + described = f"{described} ({', '.join(quoted)})" + return described + + +def _unique(column: Any) -> list[Any]: + if hasattr(column, "dropna"): + return list(column.dropna().unique()) + return column.drop_nulls().unique(maintain_order=True).to_list() + + +# A timestamp at midnight is a date as far as the model is concerned, and the +# time of day is noise in a range. +def _value(value: Any) -> str: + isoformat = getattr(value, "isoformat", None) + if isoformat is None: + return str(value) + return isoformat().removesuffix("T00:00:00").replace("T", " ") + + +def _flatten(properties: list[str]) -> str: + if len(properties) == 1: + return properties[0] + return f"{', '.join(properties[:-1])}, and {properties[-1]}" diff --git a/pkg-py/src/commons/_handles.py b/pkg-py/src/commons/_handles.py new file mode 100644 index 00000000..34abe235 --- /dev/null +++ b/pkg-py/src/commons/_handles.py @@ -0,0 +1,57 @@ +"""Conversation-scoped store of tool results. + +A later `run_python` call reaches an earlier result as a plain variable +(`r1`, `r2`, ...), so a tool's output can be built on rather than repeated. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any + +from ._frames import describe_frame, is_frame + +__all__ = ["HandleStore"] + + +# Enough rows to work with, few enough that a runaway query cannot fill the +# conversation's memory. +MAX_HANDLE_ROWS = 10_000 + + +@dataclass +class HandleStore: + max_rows: int = MAX_HANDLE_ROWS + _values: dict[str, Any] = field(default_factory=dict) + + def register(self, value: Any) -> str | None: + """Store a result and return the note telling the model how to reach it. + + Values that are not frames are stored too, so a scalar measure result + stays available for further derivation. + """ + if value is None: + return None + handle = f"r{len(self._values) + 1}" + if not is_frame(value): + self._values[handle] = value + return _note(handle) + + truncated = len(value) > self.max_rows + if truncated: + value = value.head(self.max_rows) + self._values[handle] = value + capped = ( + f" Only the first {self.max_rows:,} rows are stored." if truncated else "" + ) + return f"{_note(handle)}{capped}\n{describe_frame(value)}" + + def ids(self) -> list[str]: + return list(self._values) + + def get(self, handle: str) -> Any: + return self._values[handle] + + +def _note(handle: str) -> str: + return f"Available to `run_python` as `{handle}`." diff --git a/pkg-py/tests/test_handles.py b/pkg-py/tests/test_handles.py new file mode 100644 index 00000000..6c21da84 --- /dev/null +++ b/pkg-py/tests/test_handles.py @@ -0,0 +1,181 @@ +"""Handles: tool results a later `run_python` call can reach by name.""" + +from typing import Any + +import pytest + +from commons._handles import HandleStore + +from ._shared import load_shared_fixture + +pd = pytest.importorskip("pandas") +pl = pytest.importorskip("polars") + + +def test_a_value_that_is_not_a_frame_is_registered_and_reachable(): + store = HandleStore() + + note = store.register(6) + + assert note == "Available to `run_python` as `r1`." + assert store.ids() == ["r1"] + assert store.get("r1") == 6 + + +def test_nothing_is_registered_for_a_missing_value(): + store = HandleStore() + + assert store.register(None) is None + assert store.ids() == [] + + +def _register(store: HandleStore, value: object) -> str: + note = store.register(value) + assert note is not None + return note + + +def test_a_frame_note_opens_with_its_shape(): + frame = pd.DataFrame({"region": ["north", "south"], "revenue": [1.5, 9.0]}) + + note = _register(HandleStore(), frame) + + assert note.splitlines()[:2] == [ + "Available to `run_python` as `r1`.", + "A data frame with 2 rows and 2 columns:", + ] + + +def test_a_numeric_column_reports_its_range_and_missing_count(): + frame = pd.DataFrame({"revenue": [1.5, 9.0, None]}) + + note = _register(HandleStore(), frame) + + dtype = frame["revenue"].dtype + assert f"* revenue: {dtype} with range [1.5, 9.0], and 1 missing" in note + + +def test_a_string_column_reports_its_values_when_there_are_few(): + frame = pd.DataFrame({"region": ["north", "south", None]}) + + note = _register(HandleStore(), frame) + + dtype = frame["region"].dtype + assert ( + f'* region: {dtype} with 1 missing, and 2 unique values ("north", "south")' + in note + ) + + +def test_a_boolean_column_reports_how_many_are_true(): + frame = pd.DataFrame({"flag": [True, False, True]}) + + note = _register(HandleStore(), frame) + + dtype = frame["flag"].dtype + assert f"* flag: {dtype} with 2 True, 1 False, and 0 missing" in note + + +def test_a_datetime_column_reports_a_readable_range(): + frame = pd.DataFrame({"when": pd.to_datetime(["2020-01-01", "2020-01-03"])}) + + note = _register(HandleStore(), frame) + + assert "with range [2020-01-01, 2020-01-03]" in note + + +def test_a_polars_frame_is_described_in_the_same_terms(): + frame = pl.DataFrame( + {"region": ["north", "south", None], "revenue": [1.5, 9.0, None]} + ) + + note = _register(HandleStore(), frame) + + assert "A data frame with 3 rows and 2 columns:" in note + assert ( + '* region: String with 1 missing, and 2 unique values ("north", "south")' + in note + ) + assert "* revenue: Float64 with range [1.5, 9.0], and 1 missing" in note + + +def test_a_wide_frame_describes_fifty_columns_and_counts_the_rest(): + frame = pd.DataFrame({f"c{index}": [index] for index in range(55)}) + + note = _register(HandleStore(), frame) + + assert "55 columns:" in note + assert "* c49: " in note + assert "* c50: " not in note + assert "and 5 more columns" in note + + +def test_a_single_row_frame_reads_as_one_row(): + frame = pd.DataFrame({"revenue": [1.5]}) + + note = _register(HandleStore(), frame) + + assert "A data frame with 1 row and 1 column:" in note + + +def test_a_long_frame_is_stored_truncated_and_says_so(): + frame = pd.DataFrame({"n": [1, 2, 3, 4]}) + store = HandleStore(max_rows=2) + + note = _register(store, frame) + + assert note.startswith( + "Available to `run_python` as `r1`. Only the first 2 rows are stored.\n" + ) + assert "A data frame with 2 rows and 1 column:" in note + assert len(store.get("r1")) == 2 + + +def test_shared_registration_cases(): + section = load_shared_fixture("handles")["registrations"] + assert section["cases"] + + for case in section["cases"]: + store = HandleStore(max_rows=section["max_rows"]) + for spec, expected in zip(case["values"], case["expected"], strict=True): + note = store.register(_registered_value(spec)) + handle = expected["handle"] + if handle is None: + assert note is None, case["name"] + continue + assert note is not None and f"`{handle}`" in note, case["name"] + assert (section["truncation_note"] in note) is expected["truncated"], case[ + "name" + ] + if "stored_rows" in expected: + assert len(store.get(handle)) == expected["stored_rows"], case["name"] + assert store.ids() == [ + expected["handle"] + for expected in case["expected"] + if expected["handle"] is not None + ], case["name"] + + +def _registered_value(spec: dict[str, Any]) -> Any: + if spec["kind"] == "nothing": + return None + if spec["kind"] == "scalar": + return 6 + return pd.DataFrame({"n": range(int(spec["rows"]))}) + + +def test_a_column_with_no_values_left_has_no_range(): + frame = pd.DataFrame({"revenue": pd.Series([None, None], dtype="float64")}) + + note = _register(HandleStore(), frame) + + assert "* revenue: float64 with 2 missing" in note + assert "range" not in note + + +def test_one_of_a_kind_reads_as_one_unique_value(): + frame = pd.DataFrame({"region": ["north", "north"]}) + + note = _register(HandleStore(), frame) + + assert '1 unique value ("north")' in note diff --git a/pkg-r/tests/testthat/fixtures/shared/handles.json b/pkg-r/tests/testthat/fixtures/shared/handles.json new file mode 100644 index 00000000..b6f01b64 --- /dev/null +++ b/pkg-r/tests/testthat/fixtures/shared/handles.json @@ -0,0 +1,62 @@ +{ + "description": "Handles: the ids a conversation hands out for tool results, and the row cap on a stored frame. The note's first sentence differs between the packages, because the tool that reaches a handle is `run_r` in R and `run_python` in Python, so what is pinned here is the id sequence, the cap, and the sentence that states it. The source is tests/shared/handles.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", + "registrations": { + "description": "Each case registers values in order against one store and states the handle each one gets back. A frame longer than the cap is stored truncated, and the note says so in these words. `rows` builds a frame of that many rows; `scalar` is a value that is not a frame, registered so that a measure result stays available for further derivation; `nothing` is a tool that produced no value, which takes no handle.", + "max_rows": 3, + "truncation_note": "Only the first 3 rows are stored.", + "cases": [ + { + "name": "frames and scalars share one sequence of ids", + "values": [ + { + "kind": "frame", + "rows": 2 + }, + { + "kind": "scalar" + }, + { + "kind": "frame", + "rows": 5 + } + ], + "expected": [ + { + "handle": "r1", + "truncated": false, + "stored_rows": 2 + }, + { + "handle": "r2", + "truncated": false + }, + { + "handle": "r3", + "truncated": true, + "stored_rows": 3 + } + ] + }, + { + "name": "a value that is not there takes no handle", + "values": [ + { + "kind": "nothing" + }, + { + "kind": "scalar" + } + ], + "expected": [ + { + "handle": null + }, + { + "handle": "r1", + "truncated": false + } + ] + } + ] + } +} diff --git a/pkg-r/tests/testthat/test-handles.R b/pkg-r/tests/testthat/test-handles.R new file mode 100644 index 00000000..3427b64c --- /dev/null +++ b/pkg-r/tests/testthat/test-handles.R @@ -0,0 +1,51 @@ +test_that("both packages hand out the same handles", { + section <- shared_fixture("handles")$registrations + expect_gt(length(section$cases), 0) + + for (case in section$cases) { + store <- new_handle_store() + + for (index in seq_along(case$values)) { + spec <- case$values[[index]] + expected <- case$expected[[index]] + value <- switch( + spec$kind, + nothing = NULL, + scalar = 6L, + data.frame(n = seq_len(spec$rows)) + ) + + note <- register_handle(store, value, max_rows = section$max_rows) + + if (is.null(expected$handle)) { + expect_null(note, info = case$name) + next + } + expect_match( + note, + paste0("`", expected$handle, "`"), + fixed = TRUE, + info = case$name + ) + expect_identical( + grepl(section$truncation_note, note, fixed = TRUE), + expected$truncated, + info = case$name + ) + if (!is.null(expected$stored_rows)) { + expect_identical( + nrow(get_handle(store, expected$handle)), + as.integer(expected$stored_rows), + info = case$name + ) + } + } + + registered <- Filter(function(step) !is.null(step$handle), case$expected) + expect_identical( + handle_ids(store), + vapply(registered, function(step) step$handle, character(1)), + info = case$name + ) + } +}) diff --git a/tests/shared/README.md b/tests/shared/README.md index aca4f1c2..94f93e25 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -26,6 +26,7 @@ The Python suite reads this directory directly. The R suite cannot. `testthat` n - **The provenance and citation behavior.** The `derive_provenance_tag()` truth table, `normalize_citation()` input/output pairs, `match_citation()` verdicts including both guards (10-character minimum, only-the-quote-verifies), `parse_commons_citation()` well-formed and malformed bodies. Also the chunk-invariance cases of the streaming scanner. The scanner is a pure chunks-in/string-out function, so it is ideal fixture material. - **System prompt rendering.** The template lives in `prompts/`, not here, but what each renderer makes of it does: `prompt-render.json` holds the data a renderer receives and the prompt it must produce. A shared template pins the words, not the rendering. - **Where the citation request lands.** `citation-request.json` pins the reminder to the first tool result of a user turn whose output has to be cited: which value shapes it joins and how, and which turns start a new request. The wording lives in `prompts/citation-request.md`, so the fixture uses a stand-in for it. +- **Handles.** `handles.json` pins the ids a conversation hands out for tool results (`r1`, `r2`, ...), that a value which is not a frame gets one too, and the row cap with the sentence that states it. The rest of the note differs by package, since the tool that reaches a handle is `run_r` in R and `run_python` in Python. - **The citation dialect and display copy.** The `` grammar and the `PROVENANCE_DISPLAY` strings, so that both UIs say the same words. - **The definitions interface.** `definition-export/` holds the shared fixtures: 14 data dictionaries in data-dict's YAML format, each declaring table-level `definitions` whose expressions use data-dict's expression language — 3 valid files (42 definitions) and 11 invalid ones — read by both commons implementations. `definitions.json` pins what both packages agree to produce from them, in three sections: - `export_records` — the expected export for each valid definition: its SQL translation, its inferred kind and type, and the columns and definitions it references. Generated from the data-dict binary at the pinned commit by `scripts/generate-definitions-fixture.sh`, which refuses to run against a binary built from anything else. Never hand-edit. diff --git a/tests/shared/handles.json b/tests/shared/handles.json new file mode 100644 index 00000000..b6f01b64 --- /dev/null +++ b/tests/shared/handles.json @@ -0,0 +1,62 @@ +{ + "description": "Handles: the ids a conversation hands out for tool results, and the row cap on a stored frame. The note's first sentence differs between the packages, because the tool that reaches a handle is `run_r` in R and `run_python` in Python, so what is pinned here is the id sequence, the cap, and the sentence that states it. The source is tests/shared/handles.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", + "registrations": { + "description": "Each case registers values in order against one store and states the handle each one gets back. A frame longer than the cap is stored truncated, and the note says so in these words. `rows` builds a frame of that many rows; `scalar` is a value that is not a frame, registered so that a measure result stays available for further derivation; `nothing` is a tool that produced no value, which takes no handle.", + "max_rows": 3, + "truncation_note": "Only the first 3 rows are stored.", + "cases": [ + { + "name": "frames and scalars share one sequence of ids", + "values": [ + { + "kind": "frame", + "rows": 2 + }, + { + "kind": "scalar" + }, + { + "kind": "frame", + "rows": 5 + } + ], + "expected": [ + { + "handle": "r1", + "truncated": false, + "stored_rows": 2 + }, + { + "handle": "r2", + "truncated": false + }, + { + "handle": "r3", + "truncated": true, + "stored_rows": 3 + } + ] + }, + { + "name": "a value that is not there takes no handle", + "values": [ + { + "kind": "nothing" + }, + { + "kind": "scalar" + } + ], + "expected": [ + { + "handle": null + }, + { + "handle": "r1", + "truncated": false + } + ] + } + ] + } +} From 37681f2333fec99901ac0c6a71e065f653c2456e Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 7 Sep 2026 10:16:05 -0600 Subject: [PATCH 2/2] fix(py): describe frames positionally, tolerate unreadable ones Review findings on the handle store: - describe_frame() reads columns by position, so a pandas frame with duplicate column names describes instead of crashing register(). - A column of unhashable values (lists, arrays) omits its unique count rather than failing the whole description, and unique values are quoted with json.dumps() so embedded quotes and newlines escape. - is_frame() also requires len() and [], so a value that merely has columns (a database table, say) is no longer mistaken for a frame; register() still stores a frame-like value it cannot read, just without a description, so a strange result is never lost. - The store's dataclass field stays out of repr and ==, so logging a store does not dump frames and comparing stores does not raise. - The shared fixture pins the note's opening sentence as a template (only the tool name differs by package) and gains a case at exactly the row cap; both suites assert the note's whole first line. - Tests now cover the default 10,000-row cap, the unique-value caps, empty frames, all-True booleans, and the 50-column boundary; R gains the missing-store and empty-store cases. --- pkg-py/src/commons/_frames.py | 34 ++++- pkg-py/src/commons/_handles.py | 18 ++- pkg-py/tests/test_handles.py | 140 +++++++++++++++++- .../testthat/fixtures/shared/handles.json | 21 ++- pkg-r/tests/testthat/test-handles.R | 23 ++- tests/shared/README.md | 2 +- tests/shared/handles.json | 21 ++- 7 files changed, 231 insertions(+), 28 deletions(-) diff --git a/pkg-py/src/commons/_frames.py b/pkg-py/src/commons/_frames.py index 4195c9c5..4c8f3de1 100644 --- a/pkg-py/src/commons/_frames.py +++ b/pkg-py/src/commons/_frames.py @@ -7,13 +7,18 @@ from __future__ import annotations +import json from typing import Any __all__ = ["describe_frame", "is_frame"] +# len() and [] are what describing a frame needs, so a value that merely has +# columns — a database table, say — is not one. def is_frame(value: Any) -> bool: - return hasattr(value, "__dataframe__") or hasattr(value, "columns") + return ( + hasattr(value, "__dataframe__") or hasattr(value, "columns") + ) and hasattr(value, "__len__") and hasattr(value, "__getitem__") # ellmer's `df_schema()` describes a frame for the R agent; this describes one @@ -28,13 +33,23 @@ def describe_frame(frame: Any, max_columns: int = MAX_SUMMARY_COLUMNS) -> str: shape = f"{_count(len(frame), 'row')} and {_count(len(names), 'column')}" lines = [f"A data frame with {shape}:"] lines += [ - f"* {name}: {_describe_column(frame[name])}" for name in names[:max_columns] + f"* {name}: {_describe_column(_column_at(frame, position))}" + for position, name in enumerate(names[:max_columns]) ] if len(names) > max_columns: lines.append(f"and {len(names) - max_columns} more columns") return "\n".join(lines) +# By position rather than by name, because pandas allows duplicate column +# names, and a name then selects a frame rather than a column. +def _column_at(frame: Any, position: int) -> Any: + iloc = getattr(frame, "iloc", None) + if iloc is not None: + return iloc[:, position] + return frame[:, position] + + def _count(number: int, noun: str) -> str: return f"{number:,} {noun}" if number == 1 else f"{number:,} {noun}s" @@ -62,7 +77,10 @@ def _describe_column(column: Any) -> str: described, ] else: - properties = [described, _describe_values(column)] + properties = [described] + unique = _describe_values(column) + if unique is not None: + properties.append(unique) return f"{column.dtype} with {_flatten(properties)}" @@ -96,10 +114,14 @@ def _missing(column: Any) -> int: # Like ellmer: the values themselves only when there are few and they are # short, so a column of free text stays a count rather than a wall of prompt. -def _describe_values(column: Any) -> str: - values = _unique(column) +def _describe_values(column: Any) -> str | None: + try: + values = _unique(column) + except TypeError: + # Unhashable values, like a column of lists, have no unique count. + return None described = _count(len(values), "unique value") - quoted = [f'"{value}"' for value in values] + quoted = [json.dumps(str(value), ensure_ascii=False) for value in values] if 0 < len(values) <= 10 and sum(len(value) for value in quoted) < 200: described = f"{described} ({', '.join(quoted)})" return described diff --git a/pkg-py/src/commons/_handles.py b/pkg-py/src/commons/_handles.py index 34abe235..106be36d 100644 --- a/pkg-py/src/commons/_handles.py +++ b/pkg-py/src/commons/_handles.py @@ -22,7 +22,8 @@ @dataclass class HandleStore: max_rows: int = MAX_HANDLE_ROWS - _values: dict[str, Any] = field(default_factory=dict) + # Frames in the store would flood a repr, and == on one raises. + _values: dict[str, Any] = field(default_factory=dict, repr=False, compare=False) def register(self, value: Any) -> str | None: """Store a result and return the note telling the model how to reach it. @@ -37,14 +38,21 @@ def register(self, value: Any) -> str | None: self._values[handle] = value return _note(handle) - truncated = len(value) > self.max_rows - if truncated: - value = value.head(self.max_rows) + try: + truncated = len(value) > self.max_rows + if truncated: + value = value.head(self.max_rows) + description = describe_frame(value) + except (TypeError, AttributeError): + # is_frame is duck-typed, so a value that quacks like a frame but + # cannot be read like one is still stored, only undescribed. + self._values[handle] = value + return _note(handle) self._values[handle] = value capped = ( f" Only the first {self.max_rows:,} rows are stored." if truncated else "" ) - return f"{_note(handle)}{capped}\n{describe_frame(value)}" + return f"{_note(handle)}{capped}\n{description}" def ids(self) -> list[str]: return list(self._values) diff --git a/pkg-py/tests/test_handles.py b/pkg-py/tests/test_handles.py index 6c21da84..ed8ee97d 100644 --- a/pkg-py/tests/test_handles.py +++ b/pkg-py/tests/test_handles.py @@ -143,10 +143,16 @@ def test_shared_registration_cases(): if handle is None: assert note is None, case["name"] continue - assert note is not None and f"`{handle}`" in note, case["name"] - assert (section["truncation_note"] in note) is expected["truncated"], case[ - "name" - ] + assert note is not None, case["name"] + opening = ( + section["note_template"] + .replace("{tool}", "run_python") + .replace("{handle}", handle) + ) + first_line = note.splitlines()[0] + assert first_line == opening + ( + f" {section['truncation_note']}" if expected["truncated"] else "" + ), case["name"] if "stored_rows" in expected: assert len(store.get(handle)) == expected["stored_rows"], case["name"] assert store.ids() == [ @@ -179,3 +185,129 @@ def test_one_of_a_kind_reads_as_one_unique_value(): note = _register(HandleStore(), frame) assert '1 unique value ("north")' in note + + +def test_the_default_cap_is_ten_thousand_rows(): + frame = pd.DataFrame({"n": range(10_001)}) + store = HandleStore() + + note = _register(store, frame) + + assert "Only the first 10,000 rows are stored." in note + assert len(store.get("r1")) == 10_000 + + +def test_a_frame_with_duplicate_column_names_is_described(): + frame = pd.DataFrame([[1, 2], [3, 4]], columns=["a", "a"]) + + note = _register(HandleStore(), frame) + + assert note.count("* a: ") == 2 + + +def test_a_column_of_unhashable_values_reports_no_unique_count(): + frame = pd.DataFrame({"j": [[1, 2], [3, 4]]}) + + note = _register(HandleStore(), frame) + + assert "* j: object with 0 missing" in note + assert "unique" not in note + + +def test_a_value_that_only_has_columns_is_not_a_frame(): + class Table: + columns = ("a", "b") + + store = HandleStore() + + note = store.register(Table()) + + assert note == "Available to `run_python` as `r1`." + assert store.ids() == ["r1"] + + +def test_an_unreadable_frame_is_stored_without_a_description(): + class OddFrame: + columns = ("a",) + + def __len__(self) -> int: + return 1 + + def __getitem__(self, key: object) -> object: + raise TypeError("cannot read columns") + + store = HandleStore() + + note = store.register(OddFrame()) + + assert note == "Available to `run_python` as `r1`." + assert store.ids() == ["r1"] + + +def test_many_unique_values_stay_a_count(): + frame = pd.DataFrame({"s": [f"value {index}" for index in range(11)]}) + + note = _register(HandleStore(), frame) + + assert "11 unique values" in note + assert '"value' not in note + + +def test_long_unique_values_stay_a_count(): + frame = pd.DataFrame({"s": ["x" * 150, "y" * 150]}) + + note = _register(HandleStore(), frame) + + assert "2 unique values" in note + assert "xxx" not in note + + +def test_a_unique_value_with_quotes_is_escaped(): + frame = pd.DataFrame({"s": ['say "hi"', "bye"]}) + + note = _register(HandleStore(), frame) + + assert '"say \\"hi\\""' in note + + +def test_an_empty_frame_is_described(): + frame = pd.DataFrame({"n": pd.Series([], dtype="float64")}) + + note = _register(HandleStore(), frame) + + assert "A data frame with 0 rows and 1 column:" in note + assert "* n: float64 with 0 missing" in note + + +def test_a_frame_with_no_columns_is_described(): + note = _register(HandleStore(), pd.DataFrame()) + + assert "A data frame with 0 rows and 0 columns:" in note + + +def test_a_boolean_column_that_is_all_true_reports_no_false(): + frame = pd.DataFrame({"flag": [True, True]}) + + note = _register(HandleStore(), frame) + + dtype = frame["flag"].dtype + assert f"* flag: {dtype} with 2 True, 0 False, and 0 missing" in note + + +def test_exactly_fifty_columns_are_all_described(): + frame = pd.DataFrame({f"c{index}": [index] for index in range(50)}) + + note = _register(HandleStore(), frame) + + assert "* c49: " in note + assert "more columns" not in note + + +def test_fifty_one_columns_counts_one_more(): + frame = pd.DataFrame({f"c{index}": [index] for index in range(51)}) + + note = _register(HandleStore(), frame) + + assert "* c49: " in note + assert "* c50: " not in note + assert "and 1 more columns" in note diff --git a/pkg-r/tests/testthat/fixtures/shared/handles.json b/pkg-r/tests/testthat/fixtures/shared/handles.json index b6f01b64..0bc45291 100644 --- a/pkg-r/tests/testthat/fixtures/shared/handles.json +++ b/pkg-r/tests/testthat/fixtures/shared/handles.json @@ -1,8 +1,9 @@ { - "description": "Handles: the ids a conversation hands out for tool results, and the row cap on a stored frame. The note's first sentence differs between the packages, because the tool that reaches a handle is `run_r` in R and `run_python` in Python, so what is pinned here is the id sequence, the cap, and the sentence that states it. The source is tests/shared/handles.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", + "description": "Handles: the ids a conversation hands out for tool results, and the row cap on a stored frame. The note's first sentence differs between the packages only in the tool that reaches a handle — `run_r` in R, `run_python` in Python — so what is pinned here is the id sequence, that sentence with the tool name left as a placeholder, the cap, and the sentence that states it. The source is tests/shared/handles.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", "registrations": { - "description": "Each case registers values in order against one store and states the handle each one gets back. A frame longer than the cap is stored truncated, and the note says so in these words. `rows` builds a frame of that many rows; `scalar` is a value that is not a frame, registered so that a measure result stays available for further derivation; `nothing` is a tool that produced no value, which takes no handle.", + "description": "Each case registers values in order against one store and states the handle each one gets back. A frame longer than the cap is stored truncated, and the note says so in these words; a frame exactly at the cap is stored whole. `rows` builds a frame of that many rows; `scalar` is a value that is not a frame, registered so that a measure result stays available for further derivation; `nothing` is a tool that produced no value, which takes no handle. `note_template` is the note's opening sentence, with `{tool}` and `{handle}` filled in by each package; when a frame is truncated, the truncation note follows on the same line.", "max_rows": 3, + "note_template": "Available to `{tool}` as `{handle}`.", "truncation_note": "Only the first 3 rows are stored.", "cases": [ { @@ -37,6 +38,22 @@ } ] }, + { + "name": "a frame exactly at the cap is stored whole", + "values": [ + { + "kind": "frame", + "rows": 3 + } + ], + "expected": [ + { + "handle": "r1", + "truncated": false, + "stored_rows": 3 + } + ] + }, { "name": "a value that is not there takes no handle", "values": [ diff --git a/pkg-r/tests/testthat/test-handles.R b/pkg-r/tests/testthat/test-handles.R index 3427b64c..49be238a 100644 --- a/pkg-r/tests/testthat/test-handles.R +++ b/pkg-r/tests/testthat/test-handles.R @@ -21,15 +21,17 @@ test_that("both packages hand out the same handles", { expect_null(note, info = case$name) next } - expect_match( - note, - paste0("`", expected$handle, "`"), - fixed = TRUE, - info = case$name - ) + opening <- section$note_template + opening <- gsub("{tool}", "run_r", opening, fixed = TRUE) + opening <- gsub("{handle}", expected$handle, opening, fixed = TRUE) + expected_first <- if (expected$truncated) { + paste(opening, section$truncation_note) + } else { + opening + } expect_identical( - grepl(section$truncation_note, note, fixed = TRUE), - expected$truncated, + strsplit(note, "\n", fixed = TRUE)[[1]][[1]], + expected_first, info = case$name ) if (!is.null(expected$stored_rows)) { @@ -49,3 +51,8 @@ test_that("both packages hand out the same handles", { ) } }) + +test_that("a missing store registers nothing, and an empty store has no ids", { + expect_null(register_handle(NULL, 1)) + expect_identical(handle_ids(new_handle_store()), character()) +}) diff --git a/tests/shared/README.md b/tests/shared/README.md index 94f93e25..bc15fecb 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -26,7 +26,7 @@ The Python suite reads this directory directly. The R suite cannot. `testthat` n - **The provenance and citation behavior.** The `derive_provenance_tag()` truth table, `normalize_citation()` input/output pairs, `match_citation()` verdicts including both guards (10-character minimum, only-the-quote-verifies), `parse_commons_citation()` well-formed and malformed bodies. Also the chunk-invariance cases of the streaming scanner. The scanner is a pure chunks-in/string-out function, so it is ideal fixture material. - **System prompt rendering.** The template lives in `prompts/`, not here, but what each renderer makes of it does: `prompt-render.json` holds the data a renderer receives and the prompt it must produce. A shared template pins the words, not the rendering. - **Where the citation request lands.** `citation-request.json` pins the reminder to the first tool result of a user turn whose output has to be cited: which value shapes it joins and how, and which turns start a new request. The wording lives in `prompts/citation-request.md`, so the fixture uses a stand-in for it. -- **Handles.** `handles.json` pins the ids a conversation hands out for tool results (`r1`, `r2`, ...), that a value which is not a frame gets one too, and the row cap with the sentence that states it. The rest of the note differs by package, since the tool that reaches a handle is `run_r` in R and `run_python` in Python. +- **Handles.** `handles.json` pins the ids a conversation hands out for tool results (`r1`, `r2`, ...), that a value which is not a frame gets one too, the note's opening sentence, and the row cap with the sentence that states it. The opening sentence is pinned as a template with the tool name left as a placeholder, since the tool that reaches a handle is `run_r` in R and `run_python` in Python. - **The citation dialect and display copy.** The `` grammar and the `PROVENANCE_DISPLAY` strings, so that both UIs say the same words. - **The definitions interface.** `definition-export/` holds the shared fixtures: 14 data dictionaries in data-dict's YAML format, each declaring table-level `definitions` whose expressions use data-dict's expression language — 3 valid files (42 definitions) and 11 invalid ones — read by both commons implementations. `definitions.json` pins what both packages agree to produce from them, in three sections: - `export_records` — the expected export for each valid definition: its SQL translation, its inferred kind and type, and the columns and definitions it references. Generated from the data-dict binary at the pinned commit by `scripts/generate-definitions-fixture.sh`, which refuses to run against a binary built from anything else. Never hand-edit. diff --git a/tests/shared/handles.json b/tests/shared/handles.json index b6f01b64..0bc45291 100644 --- a/tests/shared/handles.json +++ b/tests/shared/handles.json @@ -1,8 +1,9 @@ { - "description": "Handles: the ids a conversation hands out for tool results, and the row cap on a stored frame. The note's first sentence differs between the packages, because the tool that reaches a handle is `run_r` in R and `run_python` in Python, so what is pinned here is the id sequence, the cap, and the sentence that states it. The source is tests/shared/handles.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", + "description": "Handles: the ids a conversation hands out for tool results, and the row cap on a stored frame. The note's first sentence differs between the packages only in the tool that reaches a handle — `run_r` in R, `run_python` in Python — so what is pinned here is the id sequence, that sentence with the tool name left as a placeholder, the cap, and the sentence that states it. The source is tests/shared/handles.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared.sh. Edit the source and re-run that script.", "registrations": { - "description": "Each case registers values in order against one store and states the handle each one gets back. A frame longer than the cap is stored truncated, and the note says so in these words. `rows` builds a frame of that many rows; `scalar` is a value that is not a frame, registered so that a measure result stays available for further derivation; `nothing` is a tool that produced no value, which takes no handle.", + "description": "Each case registers values in order against one store and states the handle each one gets back. A frame longer than the cap is stored truncated, and the note says so in these words; a frame exactly at the cap is stored whole. `rows` builds a frame of that many rows; `scalar` is a value that is not a frame, registered so that a measure result stays available for further derivation; `nothing` is a tool that produced no value, which takes no handle. `note_template` is the note's opening sentence, with `{tool}` and `{handle}` filled in by each package; when a frame is truncated, the truncation note follows on the same line.", "max_rows": 3, + "note_template": "Available to `{tool}` as `{handle}`.", "truncation_note": "Only the first 3 rows are stored.", "cases": [ { @@ -37,6 +38,22 @@ } ] }, + { + "name": "a frame exactly at the cap is stored whole", + "values": [ + { + "kind": "frame", + "rows": 3 + } + ], + "expected": [ + { + "handle": "r1", + "truncated": false, + "stored_rows": 3 + } + ] + }, { "name": "a value that is not there takes no handle", "values": [