perf(pydantic): reuse TypeAdapters instead of building one per payload - #1724
Closed
PranavMishra28 wants to merge 1 commit into
Closed
perf(pydantic): reuse TypeAdapters instead of building one per payload#1724PranavMishra28 wants to merge 1 commit into
PranavMishra28 wants to merge 1 commit into
Conversation
Closes temporalio#1695. `PydanticJSONPlainPayloadConverter.from_payload` called `TypeAdapter(hint)` on every payload. Pydantic caches the core schema on the class for `BaseModel` subclasses, so that is cheap for plain models, but activity and workflow hints are frequently not classes — `Annotated` discriminated unions, `list[Union[...]]`, generics — and for those `TypeAdapter.__init__` rebuilds the schema each time and construction dominates validation. Reproduced the issue's shape before changing anything (3-member discriminated union, list-wrapped): build+validate per payload: 0.1375 ms validate only: 0.0015 ms ratio: 92x After, through the converter itself: 0.0058 ms per call, 199 cache hits to 1 miss. Adapters now come from a bounded `functools.lru_cache`, sized at 256 since a worker sees a small fixed set of signatures and the cache holds hints strongly. Hints that cannot be hashed fall back to per-call construction rather than failing conversion, which keeps `Annotated[..., <unhashable metadata>]` working. Tests: five cases, including a black-box one that counts `TypeAdapter` constructions across two conversions of one hint and reports "built 2 TypeAdapters for one hint" against the unfixed converter, so it pins the behavior rather than the presence of a cache.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69582a3948
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
There's already an open PR with ongoing discussion. #1703 |
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.
Closes #1695.
@tconley1428 you said on the issue that a small LRU cache by default seemed reasonable and asked whether there's any drawback to enabling it by default. This implements that, and the two drawbacks I could find are handled below rather than left implicit.
Problem
PydanticJSONPlainPayloadConverter.from_payloadcalledTypeAdapter(hint)on every payload. Pydantic caches the core schema on the class forBaseModelsubclasses, so that's cheap for plain models — but activity and workflow hints frequently aren't classes (Annotateddiscriminated unions,list[Union[...]], generics), and for thoseTypeAdapter.__init__rebuilds the schema every time.Reproduced @ZacharyHampton's shape before touching anything:
(Issue reported 57× on their box and 521× for pydantic-ai's
CallToolResult; same effect, machine-dependent multiple.)Measured through the converter after the change: 0.0058 ms per call, 199 cache hits to 1 miss.
The two drawbacks, and what this does about them
The cache holds type hints strongly. So it's bounded —
maxsize=256. A worker sees a small fixed set of activity and workflow signatures, so that covers real usage while stopping a pathological caller from retaining unbounded types. Happy to change the number or expose it if you'd rather it be configurable.Not every hint is hashable, so not every hint can be a cache key —
Annotated[list[X], SomeUnhashableMetadata()]raisesTypeErrorfromlru_cache. That falls back to per-call construction instead of failing the conversion, with a test covering it. This is the case I'd most want a second opinion on, since it's the one that would turn a working conversion into an error if handled naively.Caching is keyed on the hint alone, which is safe here because
to_json_optionsonly affects serialization, notfrom_payload.How tested
Five cases in
tests/contrib/pydantic/test_pydantic.py:test_repeated_conversion_builds_one_type_adapter— black-box: countsTypeAdapterconstructions across two conversions of one hint. Against the unfixed converter it reportsbuilt 2 TypeAdapters for one hint. This is the one that pins behavior; the others below assert on cache internals.type_hint is None→Anypath.tests/contrib/pydantic/21 passed.ruff check,ruff format --check, andmypyclean on the changed files.One note on scope: I did not touch
to_payload.SchemaSerializerconstruction there is already hoisted into__init__, so it doesn't have this problem.Developed with Claude Code; reviewed and tested by Pranav before marking ready for review.