DataFrame builders, conversions, and the cookbook recipe (issue #6, PR 2) by craigmcchesney · Pull Request #45 · osprey-dcs/dp-python-lib

craigmcchesney · 2026-09-09T20:42:22Z

Phases 2–4 of issue #6, completing the ticket. Plan: plan/tickets/6/plan.md.

Stacked on #44 — based on that branch, so review it first; this PR retargets to main once #44 merges.

Phase 2 — `data_frame.py`

Builders for common.DataFrame, the shared time-series payload. It is also ingestion's ingestionDataFrame, so #17 extends this module rather than forking it.

sampling_clock() / timestamp_list() / timestamp_count() relocated here from sample_status_client now that calculations frames are a second caller, and re-exported from it so existing imports keep working.
Typed scalar column builders; data_column(), the escape hatch whose None entries become unset oneofs — the only way to express a gap on a shared axis; and the provenance helpers.
data_frame() routes columns by type and enforces the server's shape rules client-side (non-blank names, non-empty values, count match, name uniqueness across all types) so an error names the offending column. Size caps stay server-side: they are deployment policy, and duplicating numbers that can change is how clients drift.
Array/image/struct/serialized builders are deliberately Add ingestion API client (full surface: ingestData + streaming) #17's; hand-built columns of those kinds pass through today.

Phase 3 — `data_frame_conversions.py`

Pure Python (no extras): data_frame_timestamps(), column_values() — written standalone so the bucket query (#16) can reuse it — data_frame_columns(), column_metadata_dict().

Behind [analysis]: data_frame_to_pandas(), data_frame_from_pandas(), and the calculations_* bridges.

Two decisions worth a look:

The pandas direction always emits a TimestampList, never an inferred SamplingClock. A clock is only correct if the spacing is exactly uniform at nanosecond precision, and inferring that from an index that merely looks regular would quietly move timestamps.
NaN is fail-loud, with a message pointing at the two ways to express a gap. A dense typed column cannot represent one, so the alternatives are inventing a value or dropping a row.

Phase 4 — cookbook and docs

doc/cookbook/datasets-and-annotations.md continues the shared worked example (a dataset over the CXI_3443 shift's first hour, an orbit-drift annotation, a 1 Hz RMS calculation with provenance, the export). Plus the cookbook README entry, the README move of the three Annotation Service bullets from TODO to Current state, and the CLAUDE.md usage section promised in PR 1.

Two things found while building this

A naming collision I introduced. Exporting the data_frame() function from dp_python_lib.client shadowed the data_frame module, so both import forms returned the function — breaking the from dp_python_lib.client import data_frame as dfb form the plan's own reference snippet uses. The function is no longer re-exported at package level; reach it as dfb.data_frame(...).
An array column with no dims was treated as an empty product of 1, silently making every element its own sample. Caught by a test expecting a rejection; fixed in both the write and read paths.

Verification

699 tests with the [analysis] extra; 601 pass and 56 skip cleanly without it, verified in a real pandas-free venv rather than assumed.
25 integration tests + 12 subtests against dp-service fddf692, now including builder-made calculations read back on a sub-second axis — so the round trip exercises nanosecond arithmetic rather than whole seconds a float could also represent — provenance survival, and four export cases.
All 101 cookbook snippets type-check. The CLAUDE.md snippet was extracted and run against the live server, not just type-checked: save, query, batch fetch, calculations read-back in both plain Python and pandas, export, and teardown all succeed end to end.
ruff lint and format clean.

Not in this PR

Array/image/struct column builders (#17), bucket-query conversions (#16), and the attributes() / optional-criteria back-port to the older helpers (#40 / #41) — all out of scope per the plan.

🤖 Generated with Claude Code

https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

Adds the calculations construction and read-back layer the Phase 1 clients accept, per plan D6/D7. data_frame.py (no optional dependencies): - sampling_clock() / timestamp_list() / timestamp_count() relocated here from sample_status_client now that calculations frames are a second caller, and re-exported from it so existing imports keep working. - Typed scalar column builders, the legacy data_column() escape hatch (a None entry becomes an unset oneof -- the only way to express a gap on a shared axis), and the provenance helpers. - data_frame() routes columns by type and enforces the server's SHAPE rules client-side, so an error names the offending column instead of bouncing the whole save. Size caps stay server-side: they are deployment policy. - Array/image/struct/serialized builders are deliberately #17's; hand-built columns of those kinds pass through data_frame() today. data_frame_conversions.py: - Pure Python: data_frame_timestamps() (integer nanoseconds throughout), column_values() (standalone, so the bucket query #16 can reuse it; array columns reshape to one list per sample), data_frame_columns(), column_metadata_dict(). - Behind [analysis]: data_frame_to_pandas() (UTC index from int64 nanos), data_frame_from_pandas() (dtype -> typed column, NaN fail-loud since a dense typed column cannot express a gap), and the calculations bridges. The pandas direction always emits a TimestampList rather than inferring a SamplingClock: a clock is only correct if the spacing is exactly uniform at nanosecond precision, and guessing would move timestamps. Two things surfaced while building this: - The data_frame() *function* is deliberately NOT re-exported from dp_python_lib.client. Binding that name shadows the data_frame *module*, so both import forms hand back the function -- breaking the documented `from dp_python_lib.client import data_frame as dfb` usage. - An array column with no dims must be treated as underivable, not as an empty product of 1, which would silently make every element its own sample. Fixed in both the write and read paths. 87 new unit tests: 657 total with the [analysis] extra, and 601 passing with 56 skipping cleanly without it (verified in a venv that has no pandas). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

…ase 4) Completes issue #6 with the documentation and the remaining integration coverage. doc/cookbook/datasets-and-annotations.md continues the cookbook's shared worked example -- a dataset over the first hour of the CXI_3443 shift, an orbit-drift annotation, a 1 Hz RMS calculation carrying provenance back to the source PV, and the export -- and covers the things most likely to bite: - The archive-existence rule, including why its error text ("no PV metadata found for names") misdescribes the check it failed. - Calculations belonging to their annotation, so a full-replace save without them deletes them. - Gaps: the typed columns are dense, so data_column() or a separate frame. - Why the pandas direction never infers a SamplingClock, and why NaN is fail-loud. - That there is no download: file_path is a server-side path. Integration test grew the Phase 4 legs: builder-made calculations read back through data_frame_conversions on a sub-second axis (so the round trip exercises nanosecond arithmetic rather than whole seconds a float could also represent), provenance survival, and four export cases. 25 tests, 12 subtests, all passing against dp-service fddf692. Also: README moves the three Annotation Service bullets from TODO to Current state; CLAUDE.md gains the usage section promised in PR 1; the cookbook README lists the recipe and extends the worked-example description; and the snippet checker's preamble gains the new names plus the recipe's shared handles. The CLAUDE.md snippet was extracted and RUN against the live server, not just type-checked -- save, query, batch fetch, calculations read-back in both plain Python and pandas, export, and teardown all succeed end to end. 699 tests pass with the [analysis] extra; 601 pass and 56 skip cleanly without it. ruff, format, and all 101 cookbook snippets clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

Two corrections that only apply once data_frame.py exists on this branch, and so could not be made on the PR 1 branch: - calculations(): PR 1 reworded this docstring to say the data_frame builders "arrive in the follow-up PR", which is true there and stale here. Point at the builders directly now that they exist. - datasets-and-annotations cookbook: record the DataBlock interval semantics established by reading dp-service. The range is half-open [begin, end) like everything else in the library, so back-to-back blocks cover the boundary sample exactly once -- but HDF5 export is bucket-granular and writes overlapping buckets whole and untrimmed, so it can include out-of-range samples and can write a straddling bucket twice. That exception was documented nowhere. Also note the new bare-string rejection in data_block(). 670 unit tests pass; ruff lint and format clean; cookbook snippet checker passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

Copilot

🟡 Changes recommended

Frame validation and timestamp conversion contain correctness defects, and the cookbook has broken identifiers and version claims.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Completes issue #6 with shared DataFrame construction/conversion APIs and supporting documentation.

Changes:

Adds typed DataFrame builders, provenance helpers, and pandas/Python conversions.
Relocates shared timestamp helpers while preserving imports.
Expands integration coverage and cookbook documentation.

File summaries

File	Description
`src/dp_python_lib/client/data_frame.py`	Adds DataFrame builders and validation.
`src/dp_python_lib/client/data_frame_conversions.py`	Adds Python and pandas conversions.
`src/dp_python_lib/client/sample_status_client.py`	Relocates timestamp helpers.
`src/dp_python_lib/client/annotations_client.py`	Updates calculations guidance.
`src/dp_python_lib/client/__init__.py`	Exports builder helpers.
`tests/unit/test_data_frame.py`	Tests builders and validation.
`tests/unit/test_data_frame_conversions.py`	Tests conversion behavior.
`tests/integration/test_datasets_annotations_integration.py`	Adds calculations and export coverage.
`doc/cookbook/datasets-and-annotations.md`	Adds the worked recipe.
`doc/cookbook/README.md`	Lists the new recipe.
`.dev/tools/check-cookbook-snippets.py`	Extends snippet-checking context.
`README.md`	Documents the completed clients.
`CLAUDE.md`	Records APIs and invariants.
`plan/tickets/6/plan.md`	Records implementation status.

Review details

Suppressed comments (2)

doc/cookbook/datasets-and-annotations.md:222

Subsequent read/update snippets use annotation_id, but the creation snippet defines only saved_annotation_id; users following this continuous recipe will get NameError. Use the same handle name that the later snippets consume.

saved_annotation_id = result.annotation_id
assert saved_annotation_id is not None

doc/cookbook/datasets-and-annotations.md:250

The provenance and export snippets later call calculations_spec(calculations_id) and calculations_source(calculations_id, ...), but this snippet defines only saved_calculations_id. Align the variable name so the documented end-to-end recipe is executable without the snippet checker's hidden preamble.

saved_calculations_id = result.calculations_id
assert saved_calculations_id is not None

Files reviewed: 14/14 changed files
Comments generated: 7
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Five findings from the review of PR #45, two of them defects. Write and read paths disagreed on ragged array columns. data_frame() computed an array column's sample count with floor division, so a value count that was not a whole multiple of prod(dims) rounded into a passing count -- building a frame that data_frame_conversions then refused to read back (5 values with a per-sample size of 2 became "2 samples"). The write path now applies the same whole-multiple rule the read path already had, so what one accepts the other can always read. A regression test asserts both reject the identical input, with prod(dims) extracted into _array_sample_size() now that two checks need it. column_metadata_dict() dropped provenance time ranges. pv_source() and calculations_source() both take an optional (begin, end) narrowing which part of the source was used -- the substantive half of provenance for a derived column -- and it never survived read-back. It is now reported as 'time_range' in epoch nanoseconds, matching the module's integer-nanosecond convention. While there, each source now carries only the origin arm actually set: a pvName source has no 'calculations_column' key and vice versa, rather than the unset arm appearing as an empty string. Absent stays absent, as it does for confidence and reason in the sample status conversions. The integration test asserts the range survives a real server round trip. Three smaller ones: - data_frame_from_pandas() on duplicate column labels failed deep inside as pandas' "truth value of a Series is ambiguous" (df[name] returns a DataFrame, not a Series). It now rejects them up front, naming the offender -- concat and merge produce duplicates easily by accident. - A pandas round trip preserves every value, dtype, and timestamp but not column order, because a DataFrame stores each column kind in its own repeated field. True but undocumented, and easy to read as a bug; now in both docstrings and the cookbook, and pinned by a test rather than left incidental. - data_column() accepted np.float64 (a float subclass) but rejected np.int64 and np.bool_, which subclass nothing it matched -- an arbitrary split for a caller coming from pandas. It now maps by numbers.Integral / numbers.Real, with NumPy's bool matched by type module and name so this module keeps its no-NumPy dependency (the type is bool_ under NumPy 1.x, bool under 2.x). _timestamp_to_nanos is defined locally rather than imported from sample_status_conversions: that copy is private, and reaching across for it would make this module's dependencies read as though it needed the sample status API. 680 unit tests pass (619 pass, 61 skip cleanly in a venv without the [analysis] extra); 25 integration tests and 12 subtests pass against the live server; ruff lint and format clean; all 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

Follow-up to the PR #45 review fixes. The provenance fix needed a Timestamp -> epoch-nanoseconds conversion, and I added a private copy to data_frame_conversions, justifying it as avoiding a cross-module private import. That was the wrong call twice over: the justification cited a CLAUDE.md note that is scoped to query_support.py's shared query helpers rather than being a general rule, and the note's actual lesson is "shared helpers belong in a shared module", not "duplicate them". The conversion already existed privately in query_conversions (as _timestamp_to_epoch_nanos) and sample_status_conversions (as _timestamp_to_nanos), so my copy made three spellings of one arithmetic identity. It is now one public to_epoch_nanos() beside to_timestamp() in machine_config_client -- the same conversion in the other direction, in the module that data_frame.py, dataset_client.py, query_client.py, and sample_status_client.py already import to_timestamp() from. NANOS_PER_SECOND is public there too, since sample_status_conversions needs it for the inverse divmod. Both existing modules keep their internal names as one-line aliases, so no call site changed. The module name does not advertise a time-conversion role, but four modules already depend on it for exactly that; moving to_timestamp() somewhere better named is a separate change that would touch every one of them. 683 unit tests pass (622 pass, 61 skip without the [analysis] extra); 42 integration tests and 12 subtests pass against the live server; ruff clean; 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

to_timestamp() was defined in machine_config_client because that was the first module to need it, and six others grew imports of it from there -- dataset_client, query_client, sample_status_client, data_frame, query_conversions, sample_status_conversions -- so datasets, queries, and DataFrames all read as though they depended on the machine configuration API. The previous commit added to_epoch_nanos() beside it, which made a misattributed home the shared home for both directions. Both now live in client/time_conversions.py, along with the TimestampInput alias and NANOS_PER_SECOND. It is a leaf: it imports only stdlib and the generated protos, so any client module can use it with no risk of a cycle, which is what makes it a safe home for conversions everything needs. Every internal caller is repointed at the new module; machine_config_client is now just another caller. No back-compat shim: the feature is unreleased, and a re-export would preserve exactly the misleading imports this removes. The public package surface is unchanged except that TimestampInput is now exported too, which it should have been -- it is the declared type of many public parameters. The converter unit tests move to tests/unit/test_time_conversions.py, next to the module they cover. Done inside this ticket rather than left as a follow-up, at the user's request: the feature is unreleased, so shipping it as-is would put the wart in the release and make the fix a breaking change afterwards. 725 tests pass (683 unit, 42 integration + 12 subtests against the live server; 622 pass and 61 skip without the [analysis] extra); ruff clean; 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

Six findings from Copilot's review of f101ff6. A seventh -- array columns accepted by floor division -- duplicated a finding already fixed in a6e1865. ImageColumn was unusable. _column_sample_count() fell through to len(column.values) for it, but an ImageColumn keeps one payload per sample in `images` and has no `values` field at all, so every prebuilt image column -- a kind data_frame() documents as supported and routes to imageColumns -- raised AttributeError instead of being assembled. A pandas index in any unit but nanoseconds was serialized wrong. _timestamps_from_index() read the index through a raw int64 view, which is expressed in the index's OWN storage unit; pandas 2+ keeps second, millisecond, and microsecond resolutions, so a datetime64[us] index produced timestamps 1000x too early with no error. It now reads Timestamp.value, which is nanoseconds whatever the unit, and rejects NaT, whose integer form is the int64 minimum and would otherwise serialize as a real instant. This one had a test that should have caught it: the round trip compared raw int64 views on both sides, so it passed while both were equally wrong. It now compares the instants. date_range() returns a microsecond-unit index under pandas 3, so the fixture was already exercising the broken path. Four smaller ones: - Column names were checked for emptiness, but the documented rule is non-blank; " " passed. Applied in data_frame() and in the builders. - data_frame_timestamps() documented that it rejects an empty axis and did not: expand_data_timestamps() returns [] for a set-but-empty timestampList, whose oneof still reports as set, so a corrupt frame became a zero-row table instead of failing loudly. - Array dims were used to delimit samples and then discarded, leaving [2, 2] and [4] indistinguishable, though plan D7 asks for the dims alongside the values. Added column_dimensions() / data_frame_column_dimensions() rather than changing column_values()'s one-entry-per-sample contract, which the cookbook and integration tests depend on. - The cookbook's first snippet bound `saved_id` while every later snippet read `dataset_id`, and likewise for the annotation and calculations ids. Every snippet type-checked because the checker preamble pre-seeds those names, so the recipe was broken only when read end to end -- which is how a reader reads it. The preamble now records that seeding a name cannot prove the recipe binds it. 734 tests pass (692 unit, 42 integration + subtests against the live server); ruff clean; 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

Copilot

🟡 Changes recommended

Timestamp precision, malformed clock validation, and cookbook lifecycle errors remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 22/22 changed files
Comments generated: 4
Review effort level: Balanced

… PR 2) Four findings from Copilot's second pass over 86621d1, all real. to_timestamp() lost sub-microsecond precision on EVERY datetime. The datetime branch computed value.timestamp() and converted the float, but a float64 cannot hold present-day epoch seconds at sub-microsecond resolution: 99.7% of microsecond-precision datetimes came back with a wrong nanosecond field, by up to ~119 ns. 2026-07-14T18:00:00.000001Z produced 954 ns instead of 1000. That is the exact-match contract the whole library rests on -- sample-status matching is by exact timestamp at nanosecond precision, and provenance time ranges are compared the same way -- so this quietly undercut the property several modules take pains to preserve. It predates this ticket; moving the converters into time_conversions.py is what put the code under review. The datetime path is now integer arithmetic off the timedelta from the epoch, which is lossless because a datetime's own resolution is exactly microseconds. Verified exact across 50,000 random microsecond values and every 977th microsecond of a second. Pre-1970 datetimes still raise, now with a message naming the instant. The int/float epoch-seconds path is untouched. Three others: - timestamp_count() accepted a hand-built SamplingClock with periodNanos == 0, which expand_data_timestamps() rejects -- the same write/read asymmetry as the earlier array-dims finding. Every sample after the first would have carried the first sample's timestamp. - The cookbook's calculations save passed no annotation_id, so it created a SECOND annotation while the recipe kept reading the first, calculation-free one; every later get_annotation() read the wrong record. It now rebinds both handles and says why, pointing at the update recipe for the other intent. - The cookbook's replace discarded its result, but a replace carrying calculations stores a new object and deletes the old one, leaving the calculations_id the later export used dangling. It now captures and rebinds. Both cookbook findings share a root cause with the earlier saved_id one: snippets are type-checked in isolation against a seeded preamble, which cannot see that a SEQUENCE of snippets is incoherent. Checked this time by parsing the recipe as one continuous script and confirming each handle is rebound before its next use. 740 tests pass (698 unit, 42 integration + subtests against the live server; 635 pass and 63 skip without the [analysis] extra); ruff clean; 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

Copilot

🔵 Needs a closer look

Pandas round trips currently widen protobuf dtypes and silently discard column metadata and provenance.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/dp_python_lib/client/data_frame_conversions.py:341

Constructing the pandas frame from untyped Python lists lets pandas widen FloatColumn to float64 and Int32Column to int64. A frame → pandas → frame round trip then emits DoubleColumn/Int64Column, contradicting this API's documented dtype-preservation guarantee. Build each Series/array with a dtype selected from the protobuf column type, and cover the float32/int32 path.
src/dp_python_lib/client/data_frame_conversions.py:510
This reverse conversion ignores df.attrs["column_metadata"], which data_frame_to_pandas() populated, so the documented calculations round trip silently strips every column's tags, attributes, and provenance. Preserve that metadata when rebuilding each column, or make the lossy behavior explicit and require metadata as an input rather than carrying attrs that are discarded.
doc/cookbook/datasets-and-annotations.md:6
The PR's verification section says the live tests used dp-service commit fddf692, while the linked issue states rel-1.16.0 is not yet released. Calling this recipe “verified against” that tag is therefore inaccurate; name the actual tested server commit and describe 1.16.0 as the target API version.

Files reviewed: 22/22 changed files
Comments generated: 0 new
Review effort level: Balanced

…#6) Three findings from Copilot's third pass over 70d2102, all real. The pandas round trip discarded every column's metadata. data_frame_to_pandas() parked each column's ColumnMetadata in df.attrs["column_metadata"], and data_frame_from_pandas() ignored it -- so a frame that went out through pandas came back with its tags, attributes, and provenance silently stripped. That is the record of where the numbers came from, which is the reason calculations are worth storing and a headline feature of this ticket; carrying the attrs and then dropping them was worse than never carrying them. Added column_metadata_from_dict(), the inverse of column_metadata_dict(), and threaded it through _column_from_series(). Provenance, both origin arms, and time ranges all survive. The pandas round trip also widened dtypes. Building the frame from untyped Python lists let pandas infer float64 for a FloatColumn and int64 for an Int32Column, so converting back emitted DoubleColumn/Int64Column and the frame changed shape on a trip it should have survived. Each Series is now constructed with the dtype its protobuf column type implies; kinds with no narrow equivalent (strings, arrays, images, DataColumn) are still left to inference. Together these make the round trip lossless: a frame now converts to pandas and back to BYTE EQUALITY, apart from the deliberate SamplingClock -> TimestampList axis change, which is the documented refusal to infer a clock from an index that merely looks regular. Tests pin both the equality and that deliberate exception, and a first attempt at the equality test failed by asserting the axis was preserved -- worth recording, since that is the one thing the round trip is designed not to do. Third: the recipe claimed "Verified against dp-grpc rel-1.16.0", a tag that does not exist -- the newest release is rel-1.15.0. It now separates the target API version (1.16.0, unreleased) from what was actually tested (a dp-service build from main at fddf692). doc/cookbook/sample-status.md carries the same overclaim; left alone as it is not this PR's file. 749 tests pass (707 unit, 42 integration + subtests against the live server; 637 pass and 70 skip without the [analysis] extra); ruff clean; 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

craigmcchesney · 2026-09-10T19:39:56Z

Third Copilot pass — all three findings addressed in `a29064d`

These came through as summary-level "previously missed" notes rather than inline threads, so replying here. All three were real; I verified each before fixing.

1. Metadata and provenance were discarded by the pandas round trip. data_frame_to_pandas() populated df.attrs["column_metadata"] and data_frame_from_pandas() ignored it, so a frame that went out through pandas came back with its tags, attributes, and provenance stripped. This was the worst of the three: provenance is the reason calculations are worth storing and a headline feature of this ticket, and carrying the attrs only to drop them is worse than never carrying them.

Fixed by adding column_metadata_from_dict() — the inverse of column_metadata_dict() — and threading it through _column_from_series(). Both origin arms and their time ranges survive.

2. Dtypes were widened. Building the pandas frame from untyped lists let inference turn FloatColumn into float64 and Int32Column into int64, so converting back emitted DoubleColumn/Int64Column. Each Series is now built with the dtype its column type implies; kinds with no narrow equivalent (strings, arrays, images, DataColumn) are still left to inference.

Together these make the round trip lossless: a frame now converts to pandas and back to byte equality, apart from the deliberate SamplingClock → TimestampList axis change. Worth noting my first attempt at that equality test failed — it asserted the axis was preserved, which is the one thing the conversion is designed not to do (inferring a clock from an index that merely looks regular would quietly move timestamps). Both the equality and that exception are now pinned by tests.

3. The version claim was inaccurate. You're right that rel-1.16.0 does not exist — the newest dp-grpc release is rel-1.15.0. The header now separates the target API version (1.16.0, unreleased) from what was actually tested (a dp-service build from main at fddf692), and the verification section says the same.

One deliberate omission: doc/cookbook/sample-status.md carries the identical "Verified against rel-1.16.0" overclaim. I left it alone — it is not this PR's file, and correcting it belongs with whoever reconciles the cookbook's version banners at release time. Flagging it so it is not lost.

Verification: 749 tests pass (707 unit, 42 integration + 12 subtests against a live server; 637 pass and 70 skip in a venv without the [analysis] extra), ruff lint and format clean, all 103 cookbook snippets check.

Copilot

🟡 Changes recommended

Enum columns lose their type and enumId during the documented pandas round trip.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/dp_python_lib/client/query_conversions.py:104

This reference points to machine_config_client.to_epoch_nanos, but that module does not define or import this symbol; the shared function now lives in time_conversions. Point the comment at the actual module so future callers do not follow a nonexistent API.
src/dp_python_lib/client/sample_status_conversions.py:29
This reference points to machine_config_client.to_epoch_nanos, but that module does not define or import this symbol; the shared function now lives in time_conversions. Point the comment at the actual module so future callers do not follow a nonexistent API.

Files reviewed: 22/22 changed files
Comments generated: 1
Review effort level: Balanced

Three findings from Copilot's fourth pass over a29064d. EnumColumn was the one column kind the previous commit's dtype work missed. Its values field is int32, so leaving it out of the narrow-dtype mapping widened the codes to int64 and rebuilt the column as an Int64Column -- and the enumId went with it, which is worse than the widening: the codes are bare integers, and the id is the only thing that says which enumeration names them. A round trip turned a labeled enum into anonymous numbers. The dtype mapping now covers EnumColumn, and the id rides in df.attrs["enum_ids"]. That is carried even under exclude_column_metadata=True, because unlike tags and provenance it is structural: without it the column cannot be rebuilt as an enum at all. _column_from_series() consults it ahead of EVERY dtype branch, not just the integer ones -- an enum id on a float or string column is a caller error worth naming rather than quietly producing the wrong column kind. Enum columns now round-trip to full frame equality, and a plain Int32Column sitting beside one is unaffected. The other two were my own stale references. Comments in query_conversions and sample_status_conversions still pointed at machine_config_client.to_epoch_nanos, which I wrote in 9e07226 and then invalidated in 4c6a8e1 by moving the function to time_conversions.py without updating the prose. Sweeping for the same mistake turned up two more the review had not flagged: a stale module attribution in this plan's own reference list, and a sentence in CLAUDE.md that an earlier edit had mangled mid-clause, leaving two statements spliced together. All four corrected. 754 tests pass (712 unit, 42 integration + subtests against the live server; 637 pass and 75 skip without the [analysis] extra); ruff clean; 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

craigmcchesney · 2026-09-10T20:06:22Z

Fourth Copilot pass — all three findings addressed in `94986fd`

1. Enum columns lost their kind and their id (inline thread, replied and resolved). EnumColumn.values is int32, so leaving it out of the narrow-dtype mapping widened the codes to int64 and rebuilt the column as an Int64Column — dropping enumId with it. The widening was the lesser half: the codes are bare integers, and the id is the only thing saying which enumeration names them, so the round trip turned a labeled enum into anonymous numbers.

Fixed both halves rather than declaring enum conversion lossy. The id rides in df.attrs["enum_ids"], carried even under exclude_column_metadata=True — unlike tags and provenance it is structural, since without it the column cannot be rebuilt as an enum at all. Enum columns now round-trip to full frame equality.

2 & 3. Stale references to machine_config_client.to_epoch_nanos in query_conversions.py and sample_status_conversions.py. Correct, and both were mine: I wrote those comments in 9e07226, then invalidated them in 4c6a8e1 by moving the function to time_conversions.py without updating the prose. Both now point at the real module.

Since that was a self-inflicted class of error, I swept for others of the same shape and found two more the review did not flag:

plan/tickets/6/plan.md still attributed to_timestamp() / TimestampInput to machine_config_client.py.
A sentence in CLAUDE.md had been mangled mid-clause by an earlier edit, splicing the to_epoch_nanos description into the middle of the datetime.timestamp() warning.

All four corrected.

Verification: 754 tests pass (712 unit, 42 integration + 12 subtests against a live server; 637 pass and 75 skip without the [analysis] extra), ruff lint and format clean, 103 cookbook snippets check.

Still outstanding and deliberately untouched: doc/cookbook/sample-status.md carries the same "Verified against rel-1.16.0" overclaim corrected here, and is not this PR's file.

Copilot

🟡 Changes recommended

Pre-built timestamp lists can bypass ordering validation, and image/structure conversions discard required structural metadata.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/dp_python_lib/client/data_frame.py:178

A hand-built TimestampList is only checked for emptiness here, so duplicate or decreasing timestamps bypass timestamp_list()'s strict-order validation and are accepted by both data_frame() and SampleStatusFrame. Such a frame cannot round-trip through data_frame_from_pandas(), which rejects the resulting non-increasing index. Apply the same consecutive timestamp check to pre-built axes before returning the count.

Files reviewed: 22/22 changed files
Comments generated: 1
Review effort level: Balanced

… (issue #6) Two findings from Copilot's fifth pass over 94986fd. A hand-built TimestampList bypassed the strict-ordering rule. timestamp_count() checked only that the list was non-empty, so duplicate or decreasing timestamps -- which timestamp_list() rejects outright -- sailed through data_frame() and SampleStatusFrame alike. Two samples claiming one instant is inexpressible in the sample-status identity model, and data_frame_from_pandas() refuses the index such an axis produces, so the result was a frame the library could not round-trip. This is the third instance of one shape, after the array dims and the zero periodNanos: a builder enforces a rule that the shared count/validate helper does not, so a pre-built message walks past it. timestamp_count() now applies the same check timestamp_list() does, and the docstring says the rule belongs to the axis rather than to the builder. ImageColumn and StructColumn lost their structural fields on read. column_values() yields the raw per-sample payloads, but an image's imageDescriptor (width, height, channels, encoding) and a struct's schemaId are what make those bytes interpretable at all -- without them the values are undecodable blobs. Neither was reachable from the conversion output. Added image_descriptor_dict() / column_schema_id() and the frame-level data_frame_image_descriptors() / data_frame_schema_ids(), following the precedent set for array dims and enum ids: the structural field travels in a companion accessor rather than being folded into column_values(), which keeps its "exactly one entry per sample, uniform across every column kind" contract. 761 tests pass (719 unit, 42 integration + subtests against the live server; 644 pass and 75 skip without the [analysis] extra). The stricter axis rule is shared with SampleStatusFrame, whose 96 tests still pass. ruff clean; 103 cookbook snippets check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn

craigmcchesney · 2026-09-10T20:15:46Z

Fifth Copilot pass — both findings addressed in `5ce9e07`

1. Pre-built TimestampList bypassed the strict-ordering rule (summary-level finding). timestamp_count() checked only that the list was non-empty, so duplicate or decreasing timestamps — which timestamp_list() rejects outright — sailed through data_frame() and SampleStatusFrame alike. Two samples claiming one instant is inexpressible in the sample-status identity model, and data_frame_from_pandas() refuses the index such an axis produces, so the result was a frame the library could not round-trip.

Worth naming the pattern: this is the third instance of one shape, after the array dims and the zero periodNanos. In each case a builder enforces a rule that the shared count/validate helper does not, so a pre-built message walks straight past it. timestamp_count() now applies the same check timestamp_list() does, and the docstring frames the rule as belonging to the axis rather than to the builder — which is the framing that would have prevented all three.

2. ImageColumn and StructColumn lost their structural fields (inline thread, replied and resolved). Verified against the proto: imageDescriptor and schemaId were unreachable from anything the converter returned, leaving the payloads as undecodable blobs. Added image_descriptor_dict() / column_schema_id() and their frame-level companions, following the precedent set for array dims and enum ids.

Verification: 761 tests pass (719 unit, 42 integration + 12 subtests against a live server; 644 pass and 75 skip without the [analysis] extra), ruff clean, 103 cookbook snippets check. The stricter axis rule is shared with SampleStatusFrame, whose 96 tests still pass unchanged.

Still outstanding and deliberately untouched: doc/cookbook/sample-status.md's "Verified against rel-1.16.0" overclaim, which is not this PR's file.

craigmcchesney and others added 3 commits September 10, 2026 11:42

craigmcchesney force-pushed the feature/issue-6-data-frame branch from 8f384bb to f101ff6 Compare September 10, 2026 17:43

craigmcchesney changed the base branch from feature/issue-6-datasets-annotations to main September 10, 2026 18:19

craigmcchesney requested a balanced review from Copilot September 10, 2026 18:19

craigmcchesney closed this Sep 10, 2026

craigmcchesney reopened this Sep 10, 2026

Copilot started reviewing on behalf of craigmcchesney September 10, 2026 18:20 View session