-
Notifications
You must be signed in to change notification settings - Fork 66
feat: Add validation to check to get_or_create_memory for idempotent requests #227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…uly idempotent experience
class CustomSummaryStrategy(BaseStrategy): | ||
"""Custom summary strategy with configurable consolidation. | ||
This strategy allows customization of consolidation using custom prompts and models. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really only consolidation? I didn't know that
def _camel_to_snake(name: str) -> str: | ||
"""Convert camelCase to snake_case.""" | ||
# Handle sequences of uppercase letters (like XMLHttpRequest -> xml_http_request) | ||
s1 = re.sub("(.)([A-Z][a-z]+)", r"\1_\2", name) | ||
return re.sub("([a-z0-9])([A-Z])", r"\1_\2", s1).lower() | ||
|
||
@staticmethod | ||
def normalize_field_names(data: Any) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im kind of surprised that there's nothing like this that comes with python out of the box (only kind of surprised)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do wonder if it would be better to just take a dependency for these things though (all the deep compare stuff)? It seems pretty complex (though I can also see the argument that this logic should never have to change, and we will never have to re-visit this logic, and this logic will actually be more stable if we directly implement this rather than take a dependency)
logger = logging.getLogger(__name__) | ||
|
||
|
||
class UniversalComparator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there's got to be a simpler way to evaluate equality of Strategies than this 400 line class. Would like to think about it a little more
Description
Enhanced the
get_or_create_memory
method to provide true idempotency by validating existing memory strategies against provided strategies. Added comprehensive strategy validation utilities with deep comparison capabilities, camelCase/snake_case normalization, and support for custom memory strategies.Type of Change
Testing
Checklist
Security Checklist
Breaking Changes
List any breaking changes and migration instructions:
N/A - Fully backward compatible. Validation only occurs when strategies are explicitly provided to
get_or_create_memory
.Additional Notes
Key Components Added:
strategy_validator.py
(395 lines): Universal comparison utilities with camelCase/snake_case normalizationCustomStrategy
model (119 lines): Support for advanced memory configurationsBehavior:
get_or_create_memory
, validates against existing memory configurationValueError
on strategy mismatch to prevent silent configuration driftFiles Modified: 8 files, +1,953 lines, -93 lines