DataSet, Annotation, and Export clients (issue #6, PR 1) - #44
Merged
Conversation
Wraps the modernized DataSet / Annotation / Calculations / Export area of DpAnnotationService in the house client pattern, as three feature clients on the existing annotation facade (plan D1): - client.annotation.datasets (DataSetClient): save/get/query/iter/delete plus the get_datasets() batch fetch (D9), the DataSetQuery criterion helpers, and the data_block() builder. - client.annotation.annotations (AnnotationsClient): save/get/query/iter/delete plus get_calculations(), the AnnotationQuery helpers, and calculations(). - client.annotation.export (ExportClient): export_data(), the ExportFormat enum, and calculations_spec(). Every unary sender goes through ServiceApiClientBase._dispatch() (#14). patchDataSet / patchAnnotation are not wrapped: reserved placeholders. Two criterion-helper differences from the older #5/#9 helpers, both following the proto: attributes() accepts an absent values list as a key-only existence search, and criteria is optional because the server treats an empty list as match-all. Back-porting both to the existing helpers is #40 / #41. Client-side validation covers the server's shape rules only (D11): at most one TextCriterion, at least one export source, a non-empty pv_names, and begin < end on a data block. Plan updated with four triage findings made before implementation: - delete-not-found is a REJECT business error, not a silent success - the server never checks begin < end on a DataBlock, so data_block()'s check is the only one there is - a calculations SamplingClock is rejected when startTime.epochSeconds is 0 - the redundant sampling_clock() count is kept, matching SampleStatusFrame (D6); deriving it would fork the axis API by caller 149 new unit tests (570 total, all passing); ruff lint and format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn
…#6) Closes out Phase 1 with the wrapper-level round trip the plan puts in PR 1 (Q9), verified against an ecosystem built from dp-service fddf692: 18 tests, 12 subtests. Covers save/get round trips, every DataSetQuery and AnnotationQuery criterion (including the key-only attributes search), lowercase tag normalization, full-replace clearing omitted fields, paging with limit=1, a malformed page token, the get_datasets() batch fetch, calculations inline on getAnnotation versus id-only on queryAnnotations, get_calculations() click-through, delete-refused-while-referenced, and the delete cascade. Writing it surfaced a server behavior neither the proto nor the triage found: saveDataSet requires every PV named in a data block to already exist IN THE ARCHIVE. The error text says "no PV metadata found for names: [...]", but the check is a distinct on pvName over the buckets collection (MongoAnnotationHandler.validateSaveDataSetRequest -> MongoSyncQueryClient.executeQueryPvExistence), so saved PV metadata does not satisfy it. The test therefore ingests its own samples first, through the generated ingestion stub because IngestionClient wraps only registerProvider() until #17, and probes until the bucket is queryable -- ingestData() acks before the data is visible to the check. Recorded in the plan and CLAUDE.md, since it constrains every future example and test in this area. Also adds the CLAUDE.md Key Files entries and an invariants section for the new clients; the full usage section and cookbook recipe land in PR 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn
There was a problem hiding this comment.
🟡 Changes recommended
A newly added docstring in calculations() references a data_frame.data_frame() builder that is not present in this PR, which is misleading for current users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Phase 1 Python client support for the modernized Annotation Service DataSets / Annotations / Export APIs (issue #6) by introducing three new feature clients under the existing client.annotation facade, plus the associated builders/helpers and comprehensive unit + integration tests.
Changes:
- Introduce
DataSetClient,AnnotationsClient, andExportClient(plus criterion helpers, params/results, and builders likedata_block(),calculations(),calculations_spec()andExportFormat). - Wire the new feature clients into the
AnnotationClientfacade and re-export the new public API surface fromdp_python_lib.client. - Add extensive unit tests and a live-ecosystem integration test covering dataset/annotation/calculations round-trips and key server behaviors.
File summaries
| File | Description |
|---|---|
| tests/unit/test_export_client.py | Unit coverage for ExportClient, ExportFormat, calculations_spec(), params validation, and _dispatch error tiers. |
| tests/unit/test_dataset_client.py | Unit coverage for DataSetClient, DataSetQuery, data_block(), paging, and batch get_datasets(). |
| tests/unit/test_annotations_client.py | Unit coverage for AnnotationsClient, AnnotationQuery, calculations(), and CRUD/paging/error handling. |
| tests/unit/test_annotation_client.py | Pins facade wiring to ensure all feature clients are exposed and share the channel correctly. |
| tests/integration/test_datasets_annotations_integration.py | End-to-end integration test (requires running services) covering dataset/annotation/calculations workflows and delete semantics. |
| src/dp_python_lib/client/export_client.py | New export client + output-format enum + export request params/result wrappers. |
| src/dp_python_lib/client/dataset_client.py | New dataset client + criteria helpers + data_block() builder + paging and batch fetch utilities. |
| src/dp_python_lib/client/annotations_client.py | New annotations/calculations client + criteria helpers + calculations() builder and CRUD/paging. |
| src/dp_python_lib/client/annotation_client.py | Extend facade to expose .datasets, .annotations, and .export. |
| src/dp_python_lib/client/init.py | Re-export new clients/helpers/results for the public API surface. |
| plan/tickets/6/plan.md | Plan updates reflecting completed Phase 1 work and triage findings. |
| CLAUDE.md | Documentation updates to include the new clients and key invariants/behaviors. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Resolves the review findings on PR #44, plus the DataBlock interval question it raised. - calculations(): the docstring pointed at data_frame.data_frame(), which ships in PR 2. Reworded to describe building the frame directly and to name the builders as forthcoming (Copilot review comment). - get_datasets(): chunk the id list (ID_QUERY_CHUNK_SIZE, 100) instead of sending every id as one $in. The list comes from the dataSetIds of a whole page of annotations and is effectively unbounded, so a single criterion could exceed the gRPC max message size. A short result is now logged at WARNING: an id withheld for any other reason is indistinguishable from a dangling one, so the shortfall should not pass silently. - Moved _check_at_most_one_text_criterion out of dataset_client into a new query_support module as check_at_most_one_text_criterion. It was being imported private and cross-module by annotations_client, which made that module's dependencies misleading and the name easy to break. This also removes annotations_client's only dependency on dataset_client. - data_block(): document the range as half-open [begin, end). Established by reading dp-service, since annotation.proto does not say: export reaches the same bucket-overlap filter and the same per-sample TabularDataUtility.isRetained() that querySamples() does, and that function excludes a sample landing exactly on end_time. So back-to-back blocks cover the boundary sample exactly once, matching QueryParams. HDF5 export is the documented exception -- it writes overlapping buckets whole and untrimmed, so it can include out-of-range samples. - data_block(): reject a bare string for pv_names, which otherwise assigns one PV name per character and stores a silently wrong DataSet. - SaveDataSetRequestParams and SaveAnnotationRequestParams now validate the fields the server requires (name, owner_id, and data_blocks/dataset_ids), matching ExportDataRequestParams, which already validated its own. - CLAUDE.md: record the interval finding, the empty-string-vs-None rule on calculations_id and file_url, the params validation, the get_datasets chunking, and the new query_support module. 583 unit tests pass (13 new); 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
craigmcchesney
added a commit
that referenced
this pull request
Sep 10, 2026
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
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
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.
Phase 1 of issue #6 — the Python interface to the modernized DataSets / Annotations / Export API. Plan:
plan/tickets/6/plan.md.What's here
Three feature clients on the existing
client.annotationfacade (plan D1), covering all 10 implemented unary RPCs in the area:client.annotation.datasetssave_dataset/get_dataset/query_datasets/iter_datasets/delete_dataset, plusget_datasets(ids)batch fetch (D9)client.annotation.annotationssave_annotation/get_annotation/query_annotations/iter_annotations/delete_annotation/get_calculationsclient.annotation.exportexport_dataAlongside them: the
DataSetQuery/AnnotationQuerycriterion helpers, params and result classes, thedata_block()/calculations()/calculations_spec()builders, and theExportFormatenum. Every unary sender goes throughServiceApiClientBase._dispatch()(#14).patchDataSet/patchAnnotationare not wrapped — reserved "not implemented" placeholders.Two deliberate differences from the older #5/#9 criterion helpers, both following the proto:
attributes(key)accepts an absentvalueslist as a key-only existence search, andcriteriais optional because the server treats an empty list as match-all. Back-porting both to the five existing helpers is #40 / #41.Triage findings folded into the plan
Four corrections were made before implementation, and one more while writing the integration test:
saveDataSetrequires every PV in a data block to already exist in the archive. The error saysno PV metadata found for names: [...], but the check is adistinctonpvNameover the buckets collection — saved PV metadata does not satisfy it. Neither the proto nor the ticket mentions this. It constrains every future example and test in this area, so it is recorded in both the plan andCLAUDE.md.REJECTbusiness error, not a silent success, on both delete methods.begin < endon aDataBlock— only that each bound is non-zero — sodata_block()'s check is the only one there is.SamplingClockis rejected whenstartTime.epochSecondsis 0, which is a fixture trap worth knowing.sampling_clock()countis kept (D6): deriving it would fork the axis API by caller, andSampleStatusFramealready set the house precedent. Decided now because it is breaking to change later.Verification
fddf692. The test ingests its own samples first (through the generated ingestion stub, sinceIngestionClientwraps onlyregisterProvider()until Add ingestion API client (full surface: ingestData + streaming) #17) and probes until the bucket is queryable, becauseingestData()acks before the data is visible tosaveDataSet's check.attr-definederrors on the new files, identical in kind to those on the existing shipped clients (the generated stubs carry no type information). Not in CI.Not in this PR
Per the plan's two-PR split (Q9), PR 2 carries Phases 2–4:
data_frame.pybuilders and the pure-Python read side, the pandas bridges under[analysis], thedatasets-and-annotations.mdcookbook recipe, and the README /CLAUDE.mdusage section. The ticket stays open until that merges.🤖 Generated with Claude Code
https://claude.ai/code/session_019he3UCsAnqTDE2VQ73Djwn