-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix: enforce positive memory_max_entries for in-memory cache
#9128
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
Fix: enforce positive memory_max_entries for in-memory cache
#9128
Conversation
memory_max_entries enforce positive integer constraintmemory_max_entries enforce positive integer constraint
memory_max_entries enforce positive integer constraintmemory_max_entries for in-memory cache
memory_max_entries for in-memory cachememory_max_entries for in-memory cache
chenmoneygithub
left a comment
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.
Thanks for the PR! Two minor comments, then we should be good to go!
Co-authored-by: Chen Qian <qianchen94era@gmail.com>
0f7a312 to
c0a85cc
Compare
chenmoneygithub
left a comment
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.
Thanks for addressing the comments! LGTM with 2 minor comments.
dspy/clients/__init__.py
Outdated
| disk_cache_dir: str | None = DISK_CACHE_DIR, | ||
| disk_size_limit_bytes: int | None = DISK_CACHE_LIMIT, | ||
| memory_max_entries: int | None = 1000000, | ||
| memory_max_entries: int | float = 1000000, |
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.
fyi: math.inf is float type so it should also accept it.
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.
yes this definitely reads weird in type hint, but that's sticking with cachetools' best practice, so acceptable.
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.
Ok. reverted. @chenmoneygithub
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.
nit: Maybe just float type is better than int | float or int?
https://github.com/python/typeshed/blob/main/stubs/cachetools/cachetools/__init__.pyi#L20
def __init__(self, maxsize: float, getsizeof: None = None) -> None: ...|
@chenmoneygithub |
Passing invalid
memory_max_entriestoCachecan crash cachetool with unclear errors. This PR validates it upfront and fails with clearer messages.Repros: