feat(proxy): Add snapshot/live/hybrid modes and #12
Conversation
freeze - Introduce Mode literal and mode param to SerializationProxy and template APIs - Enforce snapshot immutability (MappingProxyType, optional list freezing) - Live/hybrid re-serialize children on access with subschema serializer LRU - Support external version_getter for cache coherence; preserve across refresh() - Expose mode, freeze, version_getter in @template and inline_template - Add integration tests for proxy behavior and modes Document snapshot semantics and rendering modes in README - Include py.typed for PEP 561 typing support
Summary of ChangesHello @srnnkls, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces snapshot, live, and hybrid modes to the SerializationProxy, a significant feature for controlling the trade-off between performance and data freshness. The changes are extensive, touching the core proxy logic, the public API, and documentation. The new tests are a great addition. My review focuses on a few key areas. The most critical issue is a discrepancy between the documented behavior of live/hybrid modes and the implementation, which currently only provides live updates for dictionary-like objects, not lists. The documentation for hybrid mode also contains a misleading example. Additionally, there are opportunities to reduce code duplication and improve error handling.
| from dataclasses import dataclass | ||
|
|
||
| @dataclass | ||
| class Config: | ||
| name: str # Static | ||
| count: int # Dynamic | ||
|
|
||
| @template( | ||
| "Name: {{ config.name }}, Count: {{ config.count }}", | ||
| mode="hybrid" | ||
| ) | ||
| class ConfigTemplate: | ||
| config: Config | ||
|
|
||
| config = Config(name="App", count=0) | ||
| t = ConfigTemplate(config=config) | ||
|
|
||
| str(t) # "Name: App, Count: 0" | ||
|
|
||
| # Mutate count (complex object field) | ||
| config.count = 5 | ||
| str(t) # "Name: App, Count: 5" (sees fresh count) | ||
|
|
||
| # Note: In hybrid mode, simple fields still snapshot, | ||
| # but objects are re-serialized |
There was a problem hiding this comment.
The example for hybrid mode is incorrect and misleading. It claims that mutating an integer field (count) is reflected immediately. However, the implementation (and the documentation text for hybrid mode) states that primitives are snapshotted. Therefore, mutations to config.count will not be visible without calling refresh(). This can lead to confusion for users. I suggest a corrected example that more accurately demonstrates the behavior of hybrid mode with a nested object.
from dataclasses import dataclass
@dataclass
class Status:
is_active: bool
@dataclass
class Config:
name: str # Static primitive
status: Status # Dynamic object
@template(
"Name: {{ config.name }}, Active: {{ config.status.is_active }}",
mode="hybrid"
)
class ConfigTemplate:
config: Config
config = Config(name="App", status=Status(is_active=False))
t = ConfigTemplate(config=config)
str(t) # "Name: App, Active: False"
# Mutate a field on the nested object
config.status.is_active = True
str(t) # "Name: App, Active: True" (sees fresh status)
# Mutate a primitive on the root object
config.name = "NewApp"
str(t) # "Name: App, Active: True" (still sees old name)| def _should_live_dump(mode: Mode, is_child: bool, is_dict: bool) -> bool: | ||
| """Determine if a field should be live-dumped based on mode. | ||
|
|
||
| Args: | ||
| mode: The proxy mode (snapshot/live/hybrid) | ||
| is_child: Whether this is a child field (not root) | ||
| is_dict: Whether the serialized value is a dict (complex object) | ||
|
|
||
| Returns: | ||
| True if should re-serialize on access, False if using snapshot | ||
|
|
||
| Semantics: | ||
| - snapshot: Never live-dump (use pre-computed snapshot) | ||
| - hybrid: Live-dump dict/object children only; primitives use snapshot | ||
| - live: Live-dump dict/object children (root currently still uses shallow snapshot for keys/len) | ||
| """ | ||
| if mode == "snapshot": | ||
| return False | ||
| if mode == "hybrid": | ||
| # Only live-dump complex objects (dicts), not primitives | ||
| return is_child and is_dict | ||
| # Live mode: live-dump all dicts | ||
| return is_dict |
There was a problem hiding this comment.
The current implementation of _should_live_dump only considers dictionaries (is_dict) as complex objects that should be re-serialized in live or hybrid mode. This contradicts the documentation in README.md which states that lists and dataclasses are also treated as complex objects and re-serialized on access. This means mutations to lists of objects will not be reflected, which is likely not the intended behavior.
The logic should be updated to consider all collection types as "complex". You should rename is_dict to is_complex and at the call sites in __getattr__ and __getitem__, the check should be is_complex_value = isinstance(snapshot_value, _COLLECTION_TYPES) instead of checking only for dicts.
def _should_live_dump(mode: Mode, is_child: bool, is_complex: bool) -> bool:
"""Determine if a field should be live-dumped based on mode.
Args:
mode: The proxy mode (snapshot/live/hybrid)
is_child: Whether this is a child field (not root)
is_complex: Whether the serialized value is a complex object (dict, list, etc.)
Returns:
True if should re-serialize on access, False if using snapshot
Semantics:
- snapshot: Never live-dump (use pre-computed snapshot)
- hybrid: Live-dump complex children only; primitives use snapshot
- live: Live-dump all complex children (root currently still uses shallow snapshot for keys/len)
"""
if mode == "snapshot":
return False
if mode == "hybrid":
# Only live-dump complex objects (dicts), not primitives
return is_child and is_complex
# Live mode: live-dump all complex objects
return is_complex| try: | ||
| return cast(int, self._external_version_getter(self.obj)) | ||
| except Exception: | ||
| # Fallback to internal version if external getter fails | ||
| pass |
There was a problem hiding this comment.
The try...except Exception: block with a pass statement silently ignores any and all errors from the user-provided version_getter. This can hide bugs in the user's code and make debugging difficult. It's better to log a warning when the getter fails, so the user is aware of the problem and the fallback to the internal version counter.
| try: | |
| return cast(int, self._external_version_getter(self.obj)) | |
| except Exception: | |
| # Fallback to internal version if external getter fails | |
| pass | |
| try: | |
| return cast(int, self._external_version_getter(self.obj)) | |
| except Exception as e: | |
| # Fallback to internal version if external getter fails | |
| import warnings | |
| warnings.warn(f"External version_getter failed with: {e}. Falling back to internal versioning.") | |
| pass |
| if self.freeze: | ||
| child_ser = _freeze_collections(child_ser) |
There was a problem hiding this comment.
The _freeze_collections function is called on child_ser here, and then it's called again inside _build. This is redundant and inefficient. The responsibility for freezing should be centralized in one place, preferably _build. This same issue exists in the snapshot path of this method and in __getitem__.
| from deigma.types import Template | ||
|
|
||
| T = TypeVar("T") | ||
| Mode = Literal["snapshot", "live", "hybrid"] |
There was a problem hiding this comment.
The Mode type alias (Literal["snapshot", "live", "hybrid"]) is defined in both src/deigma/proxy.py and src/deigma/template.py. This code duplication can lead to inconsistencies if the type needs to be updated in the future. Please remove this line and instead import Mode from proxy.py (e.g., by changing the import on line 21 to from deigma.proxy import SerializationProxy, Mode).
freeze