perf: cache A2uiValidator instance on A2uiCatalog.validator property#1972
perf: cache A2uiValidator instance on A2uiCatalog.validator property#1972brucearctor wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces caching for A2uiValidator instances on A2uiCatalog to avoid redundant construction and blocking I/O in async contexts like Temporal workflows, accompanied by a comprehensive test suite. The reviewer identified a critical memory leak issue caused by the global _VALIDATOR_CACHE dictionary holding strong references to catalog instances via id(self). To resolve this, the reviewer recommended replacing the global cache with Python's standard functools.cached_property on the A2uiCatalog class, which naturally handles garbage collection. Additionally, the reviewer suggested updating the test helper to clear the cached property using object.__delattr__ and refactoring a generic try-except block in the tests to use pytest.raises for more robust error handling.
| # Module-level cache for A2uiValidator instances, keyed by catalog id(). | ||
| # Avoids re-constructing the jsonschema Registry (which does os.path I/O) | ||
| # on every .validator access. Critical for async contexts like Temporal | ||
| # workflows where blocking I/O halts the event loop. | ||
| _VALIDATOR_CACHE: Dict[int, Any] = {} |
There was a problem hiding this comment.
Using a global dictionary _VALIDATOR_CACHE to cache validators by id(self) introduces a severe memory leak. Because _VALIDATOR_CACHE holds a strong reference to the A2uiValidator instance, and the validator holds a strong reference to the A2uiCatalog instance, neither the validator nor the catalog can ever be garbage collected. In long-running applications (such as Temporal workers or web servers) where catalogs are dynamically loaded or pruned, this will lead to an eventual Out Of Memory (OOM) error.
Instead, we can use functools.cached_property, which is the standard, idiomatic Python way to cache properties on objects. It works perfectly on frozen dataclasses (since they have a __dict__ and cached_property bypasses __setattr__ by writing directly to __dict__). This ensures that the cached validator is garbage collected along with the catalog instance.
| # Module-level cache for A2uiValidator instances, keyed by catalog id(). | |
| # Avoids re-constructing the jsonschema Registry (which does os.path I/O) | |
| # on every .validator access. Critical for async contexts like Temporal | |
| # workflows where blocking I/O halts the event loop. | |
| _VALIDATOR_CACHE: Dict[int, Any] = {} | |
| from functools import cached_property |
There was a problem hiding this comment.
went ahead and addressed.
There was a problem hiding this comment.
@ditman @nan-yu — addressed all review feedback. The global _VALIDATOR_CACHE dict has been replaced with functools.cached_property, which eliminates the memory leak and id()-reuse hazard.
Also rebased on current main, updated the PR description to properly scope this as a performance optimization (the blocking I/O from #1971 was already resolved upstream — verified empirically), and updated tests to use TransportFormat instead of the deprecated A2uiSchemaManager.
There was a problem hiding this comment.
This PR looks great, thanks!
Just as a heads-up: we have a major refactor coming up for Catalog and Validators. We're moving away from caching the validator inside the A2uiCatalog class. Instead, developers will need to instantiate the validator independently, which will give them complete control over the validator instance.
|
I just noticed comment, which then alerted me to the gemini review. Can address. |
4bc13ca to
bd193d9
Compare
|
Looks like some things for me to cleanup |
bd193d9 to
69fa97d
Compare
|
I think this is OK now? |
|
Can you PTAL when you have some time @nan-yu? |
|
@brucearctor one last thing I forgot to point out! Do you mind adding a short entry under "Unreleased" on the CHANGELOG.md of the package? https://github.com/a2ui-project/a2ui/blob/main/agent_sdks/python/a2ui_agent/CHANGELOG.md That way our on-call person will know that there's changes that need to be released to pypi. Thanks again, and apologies for the delay! |
85cec49 to
f9aee2e
Compare
|
Done — added a CHANGELOG entry under "Unreleased". Thanks for the reminder, and no worries on the delay! |
A2uiCatalog.validator is a @Property that creates a new A2uiValidator on every access. This is wasteful — the validator for a given catalog is deterministic and can be safely cached. Use functools.cached_property to cache the validator directly on the instance. This is the idiomatic Python approach for frozen dataclasses: cached_property writes to __dict__ directly, bypassing __setattr__, and ties cache lifetime to the object — no global state, no memory leaks, no id()-reuse hazards. Includes 7 new tests covering: - Cache identity (same instance returned) - Cache isolation (different catalogs get different validators) - Import restriction survival - Valid/invalid payload handling on cached validators Refs a2ui-project#1971
f9aee2e to
24e9562
Compare
Summary
Cache the
A2uiValidatorinstance returned byA2uiCatalog.validatorusingfunctools.cached_propertyto avoid reconstructing it on every access.Motivation
A2uiCatalog.validatorwas a@propertythat created a newA2uiValidator(including its delegatedA2uiValidatorWrapperV10,Draft202012Validator,Registry, andFormatChecker) on every call. SinceA2uiCatalogis a frozen dataclass, the validator for a given catalog is deterministic and can be safely cached.Relationship to #1971
Issue #1971 reported blocking I/O during validator construction that hung Temporal workflows. That root cause has already been resolved on
main— theos.pathfile I/O calls in the referencing/registry chain were replaced withurllib.parse.urljoinstring operations, andRegistryis now built entirely from in-memory schemas viaResource.from_contents().Verified empirically: patching
builtins.open,os.path.exists,os.path.isfile,os.path.isdir,os.path.abspath, andos.path.realpathduring validator construction and validation confirms zero file I/O operations on currentmain.This PR is scoped as a pure performance optimization — avoiding repeated construction of the validator, registry, and format checker objects on every
.validatoraccess.Changes
agent_sdks/python/a2ui_agent/src/a2ui/schema/catalog.py@propertywith@cached_propertyonA2uiCatalog.validatorfrom functools import cached_propertyimportcached_propertywrites directly to__dict__, bypassing__setattr__on the frozen dataclass — no global state, no memory leaks, noid()-reuse hazardsagent_sdks/python/a2ui_agent/tests/schema/test_validator_caching.py(new)7 tests covering:
cached_propertycreates new instance)referencingblocked)pytest.raises)Test Results
Refs #1971