feat(py): handle store for tool results - #306
Merged
Merged
Conversation
jat255
changed the base branch from
jat255/m5-gege-citation-request
to
jat255/gege-citation-request
September 7, 2026 04:30
|
Preview deployed to Connect ( Deployed from commit 37681f2. |
|
Preview deployed to Connect ( Deployed from commit 37681f2. |
jat255
added
r
jat255
marked this pull request as ready for review
September 7, 2026 17:12
jat255
force-pushed
the
jat255/sevt-handle-store
branch
from
September 7, 2026 17:12
890b79c to
e0c6c57
Compare
jat255
removed
the
needs-manual-review
jat255
force-pushed
the
jat255/sevt-handle-store
branch
from
September 7, 2026 19:42
e0c6c57 to
84516a7
Compare
simonpcouch
approved these changes
Sep 7, 2026
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.
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.
jat255
force-pushed
the
jat255/sevt-handle-store
branch
from
September 7, 2026 19:59
84516a7 to
37681f2
Compare
|
Cleaned up 5 preview bundle(s) on https://dogfood.team.pct.posit.it: 368503, 368589, 368595, 368616, 368621 |
|
Cleaned up 5 preview bundle(s) on https://connect.staging.pct.posit.it: 2663, 2666, 2668, 2674, 2680 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tool results now get a name (
r1,r2, ...) that a laterrun_pythoncall can reach, so the model can build on an earlier result instead of recomputing it.Summary
HandleStore.register()stores a result and returns the note the model reads: which handle it got, whether the frame was truncated at 10,000 rows, and what the frame contains. Values that are not frames are stored too, so a scalar measure result stays available for further derivation, and a tool that produced nothing takes no handle.The frame description follows
ellmer::df_schema(), which is what the R agent sees: shape, then per column the dtype and missing count, with ranges for numeric and temporal columns, True and False counts for booleans, and the unique values for the rest when there are few enough and short enough to be worth printing. It caps at 50 columns for the reason ellmer does, so a wide frame cannot flood the prompt.That description lives in a new
_frames.py, which reads pandas and polars frames through what they offer rather than by importing either, since both stay optional. It also takes over the frame predicate_data_source.pyhad, now that two modules need it.Review notes
Nothing calls this yet, and that is the shape of the milestone rather than an oversight: the agent class and the tool bodies are blocked behind the layer classes, and
run_pythonbelongs to the next milestone. R shows where the calls will go:register_handle()at the measure, pool, andrun_sqlsites, andhandle_ids()withget_handle()feeding the worker namespace.tests/shared/handles.jsonpins only what both packages must agree on: the id sequence, that a non-frame value gets one, and the row cap with the sentence that states it. The rest of the note cannot be shared, because the tool that reaches a handle isrun_rin R andrun_pythonhere, and R's column description comes from ellmer while this one is ours.Two places where this deliberately reads better than R rather than matching it: the note says "1 row and 1 column" rather than ellmer's "1 rows and 1 columns", and a column with nothing left to take a range over reports only how much is missing instead of a range of nulls.
Testing
Python 991 pass, ruff and pyrefly clean. R 7063 pass, 0 fail. The frame description is tested against real pandas and polars frames, not mocks. I checked the shared fixture can fail rather than trivially pass, by perturbing the expected values in each package's copy.
R-side summary
No R code changes. The R package gains one test file,
test-handles.R, which drivesnew_handle_store(),register_handle(),handle_ids(), andget_handle()from the shared fixture. Those functions had no direct tests before; they were covered only through the tool tests that happen to register a handle, so this adds coverage rather than replacing any.The test passes
max_rowsexplicitly, so the truncation case runs on a five-row frame instead of building ten thousand rows. Nothing else in the R package is touched, and the generated fixture copy undertests/testthat/fixtures/shared/comes fromscripts/sync-shared.sh.