Skip to content

Add proper TypedDict types and TypeAdapter validation to datasets API client#1724

Open
dmontagu wants to merge 2 commits intomainfrom
dm/datasets-typed-api-client
Open

Add proper TypedDict types and TypeAdapter validation to datasets API client#1724
dmontagu wants to merge 2 commits intomainfrom
dm/datasets-typed-api-client

Conversation

@dmontagu
Copy link
Copy Markdown
Contributor

Summary

  • Replace all untyped dict[str, Any] return types in logfire/experimental/api_client.py with proper TypedDicts (DatasetSummary, DatasetDetail, CaseDetail, ExportedCase, ExportedDataset, EvaluatorSpec) derived from backend endpoint definitions
  • Add Pydantic TypeAdapters to validate/coerce API responses, converting string UUIDs and datetimes to proper UUID and datetime Python types
  • Add CaseData TypedDict for the add_cases parameter (replacing Sequence[dict[str, Any]])
  • Use Any for inputs/expected_output/metadata fields to support pydantic-evals non-dict values (e.g., Case[str, str, None])
  • Update datasets/__init__.py exports and test fixtures

Test plan

  • All 91 tests pass in test_datasets_client.py
  • All 3 tests pass in test_logfire_api.py
  • pyright reports 0 errors

… client

Replace untyped dict[str, Any] return types with proper TypedDicts derived from
backend endpoint definitions. Add Pydantic TypeAdapters to validate/coerce API
responses (string UUIDs → UUID, string datetimes → datetime). Add CaseData
TypedDict for add_cases parameter typing.
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Feb 20, 2026

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: b54b0c8
Status:🚫  Build failed.

View logs

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines +173 to +180
class ExportedCase(TypedDict):
"""A case in pydantic-evals compatible format, part of :class:`ExportedDataset`."""

name: str | None
inputs: Any
metadata: Any
expected_output: Any
evaluators: list[EvaluatorSpec] | None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 All ExportedCase fields are required (no NotRequired), which may be stricter than API

ExportedCase at logfire/experimental/api_client.py:173-180 makes all fields required (no NotRequired). If the backend ever omits optional fields like metadata or evaluators from the export response (rather than sending null), the TypeAdapter validation would fail with a ValidationError. The current test fixture FAKE_EXPORT_JSON includes all fields. This is worth verifying against the actual API contract — if the API can omit fields, some should use NotRequired.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@alexmojaki
Copy link
Copy Markdown
Collaborator

This will make it hard to change the backend endpoints (which we're currently likely to do) without breaking clients. Users might be forced to upgrade their SDK.

One option is to try to validate, and if it fails, emit a warning and return the raw dict.

Another option looks like this:

class _DatasetDetailDict(TypedDict):
    ...

class DatasetDetail:
    raw_data: _DatasetDetailDict

    @property
    def name(self):
        return self.raw_data["name"]

    @property
    def created_at(self):
        return datetime.fromisoformat(self.raw_data["created_at"])

Here raw_data could optionally still be eagerly validated in full and emit a warning or error if it fails. But most properties are expected to work even if some fields are missing or wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants