DB cache in form of decorator #534
Conversation
|
Warning Rate limit exceeded@kongzii has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces a new caching mechanism using a PostgreSQL database through the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kongzii
force-pushed
the
peter/decorator/cache
branch
2 times, most recently
from
b5b3986 to
a4a2781
Compare
kongzii
force-pushed
the
peter/decorator/cache
branch
from
a4a2781 to
e92251b
Compare
| ... | ||
|
|
||
|
|
||
| def db_cache( |
There was a problem hiding this comment.
Just add @db_cache to any function or method and it should work like magic!
Optionally, specify some of the optional keyword arguments.
It supports:
- any input/output type -- native types, lists, pydantic models, nested combinations, etc.
- it stores input arguments in DB
- it stores the output in DB (along with input we can use it to gather data)
- it handles if the pydantic model isn't backward compatible with cached value, and it will re-compute the result
- decorated methods are properly typed
- we can specify ignored arguments/types, so we don't cache based on APIKeys for example
| SQLModel.metadata.create_all(engine) | ||
|
|
||
| @wraps(func) | ||
| def wrapper(*args: Any, **kwargs: Any) -> Any: |
There was a problem hiding this comment.
In this PR, there are some Any types that I usually fight against, but here they are really necessary, or it would be super hard to type it correctly at least, but because it's all used inside of db_cache, which in the end is cast to the type of decorated function anyway, it shouldn't matter.
| @tenacity.retry(stop=tenacity.stop_after_attempt(3), wait=tenacity.wait_fixed(1)) | ||
| @observe() | ||
| @db_cache |
There was a problem hiding this comment.
Beware! Order matters here.
If we do
@observe()
@db_cache
def something(): ...
then all calls will be observed in Langfuse.
If we do
@db_cache
@observe()
def something(): ...
Then only the first (non-cached) call is observed in Langfuse.
| @@ -55,7 +54,7 @@ | |||
| def analyse_news_relevance( | |||
| raw_content: str, | |||
| question: str, | |||
| date_of_interest: datetime, | |||
| date_of_interest: date, | |||
There was a problem hiding this comment.
Changed to date to not invalidate cache because of difference in seconds or hours (or even milliseconds!)
| results = get_relevant_news_since( | ||
| question=question, | ||
| days_ago=days_ago, | ||
| news_since=news_since, |
There was a problem hiding this comment.
Passing date into get_relevant_news_since (and therefore tavily_search) to have cache based on absolute values, not relative ones. E.g. if we would call days_ago=1 for a week, then it could be always used from cache, because the argument is still the same, yet the date of 1 day ago is different!
| @@ -140,7 +138,6 @@ def get_certified_relevant_news_since_cached( | |||
| question: str, | |||
| days_ago: int, | |||
| cache: RelevantNewsResponseCache, | |||
There was a problem hiding this comment.
I left the implementation of RelevantNewsResponseCache because there is news-specific logic that was just too hard to do in a general way using decorator. Evan's explanation:
| @@ -23,62 +16,3 @@ class TavilyResponse(BaseModel): | |||
| images: list[str] | |||
| results: list[TavilyResult] | |||
| response_time: float | |||
|
|
|||
|
|
|||
| class TavilyResponseModel(SQLModel, table=True): | |||
There was a problem hiding this comment.
Tavily cache can be removed completely, I'll just re-fill the new database table with old data
|
Also did a script to refill the data: #540 But it's too hacky to be merged, so just closed the PR to keep it at least somewhere. |
| def convert_pydantic_to_dict(value: Any) -> Any: | ||
| """ | ||
| Convert Pydantic models to dictionaries, including if they are in nested structures. | ||
| """ | ||
| if isinstance(value, BaseModel): | ||
| return value.model_dump() | ||
| elif isinstance(value, dict): | ||
| return {k: convert_pydantic_to_dict(v) for k, v in value.items()} | ||
| elif isinstance(value, (list, tuple)): | ||
| return type(value)(convert_pydantic_to_dict(v) for v in value) | ||
| elif isinstance(value, set): | ||
| return {convert_pydantic_to_dict(v) for v in value} | ||
| else: | ||
| return value |
There was a problem hiding this comment.
For simplicity - can't we always store JSON objects in the DB (in the value column)? This way, we can then afterwards do model_validate_json, wrapped by try-catch and handle it appropriately.
I don't 100% like this approach of having many types with if-elif-else - ultimately this is a design decision depending on personal taste, but from my understanding, JSON would be enough, i.e. value should not be Any, instead str (JSON-compatible).
There was a problem hiding this comment.
There are two separate things: what you wrote here and on Slack:
can't we always store JSON objects in the DB (in the value column)
This PR is doing exactly that, storing args and result as JSONB in Postgres. Thanks to that, values in the database are properly formatted, and we can even query them as normal columns and filter by them:
However! You made me think and indeed, convert_pydantic_to_dict can be removed fully, simplified in this commit.
For ex, here I would find amazing if we simply did model_validate_json instead of the complex logic here - if it fails, then an error is thrown.
The issue here is that the current implementation also handles nested structures and non-pydantic structures. Let's say the output type is list[AgentMarket], then the JSONized form (doesn't matter if stored as JSONB or STRING in the database) will be like [{'id': '...', 'question': ...}, ...] and we can't call model_validate_json on lists, only on Pydantic models.
So convert_cached_output_to_pydantic is indeed necessary because the current implementation can handle all varies of outputs, as seen in the tests:
So it's either this complex implementation, or forcing us in the future to always return a single Pydantic model out of cached functions, which we already aren't doing:
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (17)
tests/conftest.py (1)
10-15: Consider adding cache cleanup and error handling.While the fixture implementation is generally good, consider these improvements:
- Add cleanup to prevent test pollution
- Add error handling for connection issues
- Document the fixture's purpose and usage
Here's a suggested implementation:
@pytest.fixture def keys_with_sqlalchemy_db_url( postgresql: psycopg.Connection, ) -> Generator[APIKeys, None, None]: + """Provides APIKeys instance with SQLAlchemy URL for testing DB cache. + + Args: + postgresql: Pytest PostgreSQL fixture providing connection details + + Yields: + APIKeys instance with DB URL and cache enabled + """ sqlalchemy_db_url = f"postgresql+psycopg2://{postgresql.info.user}:@{postgresql.info.host}:{postgresql.info.port}/{postgresql.info.dbname}" - yield APIKeys(SQLALCHEMY_DB_URL=SecretStr(sqlalchemy_db_url), ENABLE_CACHE=True) + try: + api_keys = APIKeys(SQLALCHEMY_DB_URL=SecretStr(sqlalchemy_db_url), ENABLE_CACHE=True) + yield api_keys + except Exception as e: + pytest.fail(f"Failed to setup database fixture: {e}") + finally: + # Clean up any cached data + from prediction_market_agent_tooling.tools.caches.db_cache import FunctionCache + with api_keys.get_db_session() as session: + session.query(FunctionCache).delete() + session.commit()prediction_market_agent_tooling/tools/google.py (1)
17-17: Excellent caching strategy for external API callsThe transition to database caching with a 1-day TTL is a robust choice for Google search results:
- Provides persistence across application restarts
- Reduces external API calls and associated costs
- Works well with the existing retry mechanism
Consider implementing a cache warming strategy for frequently searched queries to maintain fresh results without impacting user response times. This could be particularly useful given the 1-day TTL.
prediction_market_agent_tooling/tools/tavily/tavily_search.py (3)
35-38: Enhance error messages for better clarity.While the validation logic is correct, the error messages could be more descriptive to help users understand the requirements better.
Consider this improvement:
- raise ValueError("When topic is 'news', news_since must be provided") + raise ValueError("The 'news_since' date is required when topic='news' to specify the starting date for news search") - raise ValueError("When topic is 'general', news_since must be None") + raise ValueError("The 'news_since' parameter should not be provided when topic='general' as it only applies to news searches")
104-114: Add docstring to document the new parameter.The function would benefit from a docstring explaining the
news_sinceparameter and its usage.Consider adding:
def get_relevant_news_since( question: str, news_since: date, score_threshold: float = DEFAULT_SCORE_THRESHOLD, max_results: int = 3, ) -> list[TavilyResult]: + """ + Retrieve relevant news articles since a specific date. + + Args: + question: The search query to find relevant news. + news_since: The starting date from which to search for news. + score_threshold: Minimum relevance score (0-1) for including results. + max_results: Maximum number of results to return. + + Returns: + List of TavilyResult objects with score > score_threshold. + """
17-17: Consider cache invalidation strategy.The DB caching implementation looks solid, but consider implementing a cache invalidation strategy for cases where:
- The Tavily API response format changes
- Manual cache clearing is needed
- Cache size grows too large
This could be achieved through:
- Version tagging in cache keys
- Admin endpoints for cache management
- Periodic cleanup of old entries
prediction_market_agent_tooling/tools/relevant_news_analysis/relevant_news_analysis.py (3)
1-1: LGTM! Good architectural decision.Using
dateinstead ofdatetimeis a smart choice for caching as it prevents unnecessary cache invalidation due to time components (seconds, hours, milliseconds).
99-102: LGTM! Consider adding date validation.The changes improve caching effectiveness by using absolute dates instead of relative ones. This prevents cache misses when the same relative period (e.g., days_ago=1) is requested multiple times.
Consider adding validation to ensure news_since is not in the future:
news_since = date.today() - timedelta(days=days_ago) +if news_since > date.today(): + raise ValueError("Cannot search for news from the future") results = get_relevant_news_since(Also applies to: 119-119
Line range hint
140-140: LGTM! Good decision on keeping RelevantNewsResponseCache.Maintaining the specialized cache alongside the new DB cache is justified due to the news-specific logic that would be complex to generalize. This hybrid approach provides a good balance between reusability and specialized functionality.
prediction_market_agent_tooling/tools/is_predictable.py (1)
81-81: Consider configuring cache parameters for optimal performance.The
@db_cachedecorator is well-suited for this function as it makes expensive OpenAI API calls. However, consider explicitly configuring cache parameters like TTL (time-to-live) to prevent stale results and manage database growth.Example configuration:
-@db_cache +@db_cache(ttl_seconds=86400) # Cache for 24 hourstests/tools/test_db_cache.py (3)
10-27: Improve boolean comparison style while maintaining test clarity.The test logic is correct, but the boolean comparisons can be improved following Python's style guidelines.
Consider this style improvement:
- assert bools(True) == True - assert bools(True) == True - assert bools(False) == False + assert bools(True) + assert bools(True) + assert not bools(False)🧰 Tools
🪛 Ruff
23-23: Avoid equality comparisons to
True; useif bools(True):for truth checksReplace with
bools(True)(E712)
24-24: Avoid equality comparisons to
True; useif bools(True):for truth checksReplace with
bools(True)(E712)
25-25: Avoid equality comparisons to
False; useif not bools(False):for false checksReplace with
not bools(False)(E712)
253-298: Consider restructuring the cache invalidation test for better maintainability.While the test effectively validates cache invalidation behavior, it could be improved for clarity and maintainability.
Consider these improvements:
- Instead of redefining the model class, create two distinct model classes with different names:
class OriginalOutputModel(TestOutputModel): pass class ModifiedOutputModel(TestOutputModel): new_field: str
- Split the test into two clear phases with better documentation:
# Phase 1: Cache the original results @db_cache(api_keys=keys_with_sqlalchemy_db_url) def multiply_models_v1(a: TestInputModel, b: TestInputModel) -> OriginalOutputModel: nonlocal call_count call_count += 1 return OriginalOutputModel(result=a.value * b.value) # Phase 2: Verify cache invalidation with modified model @db_cache(api_keys=keys_with_sqlalchemy_db_url) def multiply_models_v2(a: TestInputModel, b: TestInputModel) -> ModifiedOutputModel: nonlocal call_count call_count += 1 return ModifiedOutputModel(result=a.value * b.value, new_field="modified")This approach would:
- Eliminate the need for type ignore comments
- Make the test's intention clearer
- Be more maintainable
1-320: Consider adding tests for additional scenarios.The test suite is comprehensive but could benefit from additional test cases for:
- Concurrent access:
@pytest.mark.asyncio async def test_postgres_cache_concurrent_access( keys_with_sqlalchemy_db_url: APIKeys, ) -> None: call_count = 0 @db_cache(api_keys=keys_with_sqlalchemy_db_url) async def cached_function(x: int) -> int: nonlocal call_count call_count += 1 return x * 2 # Simulate concurrent access tasks = [cached_function(1) for _ in range(10)] results = await asyncio.gather(*tasks) assert all(r == 2 for r in results) assert call_count == 1 # Should only be called once despite concurrent access
- Cache expiration:
def test_postgres_cache_expiration( keys_with_sqlalchemy_db_url: APIKeys, ) -> None: call_count = 0 @db_cache(api_keys=keys_with_sqlalchemy_db_url, max_age=timedelta(seconds=1)) def cached_function(x: int) -> int: nonlocal call_count call_count += 1 return x * 2 assert cached_function(1) == 2 assert call_count == 1 time.sleep(2) # Wait for cache to expire assert cached_function(1) == 2 assert call_count == 2 # Should be called again after expiration
- Error handling:
def test_postgres_cache_error_handling( keys_with_sqlalchemy_db_url: APIKeys, ) -> None: call_count = 0 @db_cache(api_keys=keys_with_sqlalchemy_db_url) def cached_function(x: int) -> int: nonlocal call_count call_count += 1 if x < 0: raise ValueError("Negative values not allowed") return x * 2 with pytest.raises(ValueError): cached_function(-1) with pytest.raises(ValueError): cached_function(-1) # Should not hit cache for errored executions assert call_count == 2🧰 Tools
🪛 Ruff
23-23: Avoid equality comparisons to
True; useif bools(True):for truth checksReplace with
bools(True)(E712)
24-24: Avoid equality comparisons to
True; useif bools(True):for truth checksReplace with
bools(True)(E712)
25-25: Avoid equality comparisons to
False; useif not bools(False):for false checksReplace with
not bools(False)(E712)
prediction_market_agent_tooling/tools/caches/db_cache.py (5)
97-97: Remove unnecessary f-string prefixThe log message on line 97 does not contain any placeholders, so the
fprefix is unnecessary and can be removed to simplify the code.Apply this diff to fix the issue:
-logger.warning( - f"SQLALCHEMY_DB_URL not provided in the environment, skipping function caching." -) +logger.warning( + "SQLALCHEMY_DB_URL not provided in the environment, skipping function caching." +)🧰 Tools
🪛 Ruff
97-97: f-string without any placeholders
Remove extraneous
fprefix(F541)
251-251: Fix typo in docstringThe word "recursivelly" on line 251 is misspelled. It should be "recursively".
Apply this diff:
-If this function returns a dictionary, it will be called recursivelly. +If this function returns a dictionary, it will be called recursively.
263-263: Correct typo in exception messageIn the exception message on line 263, "Unsuported" should be "Unsupported".
Apply this diff:
- f"Unsuported type for the default json serialize function, value is {y}." + f"Unsupported type for the default json serialize function, value is {y}."
160-160: Handle missing return type annotationsThe caching mechanism depends on the function's return type annotation to determine if the result is a Pydantic model. If a decorated function lacks a return type annotation, cached Pydantic models may not be reconstructed properly. Consider adding a check or warning for missing return type annotations to ensure correct caching behavior.
229-229: Ensure the wrapper function preserves type annotationsWhile casting the wrapper function to
FunctionThelps with type checking, it's important to ensure that the wrapper maintains the signature of the original function. Consider using a more precise approach to preserve type annotations and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (10)
prediction_market_agent_tooling/tools/caches/db_cache.py(1 hunks)prediction_market_agent_tooling/tools/google.py(1 hunks)prediction_market_agent_tooling/tools/is_invalid.py(2 hunks)prediction_market_agent_tooling/tools/is_predictable.py(3 hunks)prediction_market_agent_tooling/tools/relevant_news_analysis/relevant_news_analysis.py(5 hunks)prediction_market_agent_tooling/tools/tavily/tavily_models.py(0 hunks)prediction_market_agent_tooling/tools/tavily/tavily_search.py(4 hunks)prediction_market_agent_tooling/tools/tavily/tavily_storage.py(0 hunks)tests/conftest.py(1 hunks)tests/tools/test_db_cache.py(1 hunks)
💤 Files with no reviewable changes (2)
- prediction_market_agent_tooling/tools/tavily/tavily_models.py
- prediction_market_agent_tooling/tools/tavily/tavily_storage.py
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/tools/caches/db_cache.py
97-97: f-string without any placeholders
Remove extraneous f prefix
(F541)
tests/tools/test_db_cache.py
23-23: Avoid equality comparisons to True; use if bools(True): for truth checks
Replace with bools(True)
(E712)
24-24: Avoid equality comparisons to True; use if bools(True): for truth checks
Replace with bools(True)
(E712)
25-25: Avoid equality comparisons to False; use if not bools(False): for false checks
Replace with not bools(False)
(E712)
🔇 Additional comments (14)
tests/conftest.py (2)
1-8: LGTM! Clean and well-organized imports.
The imports are properly organized with clear separation between standard library, third-party, and local imports. All imports are relevant to the fixture's functionality.
14-14: Verify database connection security.
The connection string doesn't include a password. While this might be intentional for tests, we should verify this is the expected behavior.
✅ Verification successful
Database connection in tests is correctly configured without password
The connection string without a password is the expected behavior here because:
- This is a test fixture using pytest-postgresql which automatically creates a temporary database
- The
postgresqlfixture is injected by pytest-postgresql and provides connection details for this temporary database - The temporary database is configured for local testing without password authentication by design
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other database connection strings in the codebase that might indicate the expected pattern
rg -g '!*.pyc' -g '!*.git' -A 2 -B 2 'postgresql\+psycopg2:\/\/'
Length of output: 179
Script:
#!/bin/bash
# Search for database connection strings with proper regex escaping
rg -g '!*.pyc' -g '!*.git' -A 2 -B 2 'postgresql\+psycopg2://'
# Also check for any configuration files that might contain database settings
fd -e yml -e yaml -e ini -e conf -e cfg -e env | xargs cat 2>/dev/null
# Look for any test configuration files specifically
fd -e py -g '*test*' -g '*conf*' | xargs grep -l "postgresql" 2>/dev/null
Length of output: 2400
Script:
#!/bin/bash
# Let's check the specific test configuration file content
cat tests/conftest.py
# Also search for any test database fixtures or setup
rg -g '!*.pyc' -A 5 -B 5 'def postgresql_connection|pytest.fixture.*postgresql'
Length of output: 614
prediction_market_agent_tooling/tools/google.py (2)
9-9: LGTM: Clean import organization
The import statement follows Python's import organization conventions, properly separating standard library, third-party, and local imports.
Line range hint 17-54: Verify cache performance and error handling
The switch to database caching requires verification of:
- Error handling for both API and cache failures
- Performance impact compared to the previous in-memory implementation
- Impact on application startup time
✅ Verification successful
Let me gather more information about the caching implementation and error handling.
Based on the gathered information, I can now provide a final response as the code analysis shows comprehensive error handling and caching implementation:
Database caching implementation is robust and well-tested
The codebase demonstrates:
- Proper error handling for both API and cache failures through try-except blocks
- Graceful degradation when cache validation fails (treats as cache miss)
- Comprehensive test coverage for various data types and edge cases
- Performance optimization through configurable cache parameters:
max_agefor cache invalidationignore_argsfor selective cachingcache_nonefor controlling None value caching- Global cache toggle via
ENABLE_CACHE
The switch to database caching is a safe improvement that maintains reliability while adding persistence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling and cache implementation
# Test 1: Check for proper error handling in other functions using db_cache
echo "Checking error handling patterns..."
rg -A 3 "@db_cache" | rg -B 3 "try|except|raise"
# Test 2: Look for potential cache initialization code
echo "Checking cache initialization..."
ast-grep --pattern 'class $_(FunctionCache|Cache|DBCache) {
$$$
}'
# Test 3: Check for similar caching patterns across the codebase
echo "Checking caching patterns consistency..."
rg "@db_cache\(max_age=" -A 1
Length of output: 759
Script:
#!/bin/bash
# Check the db_cache implementation and its error handling
echo "Searching for db_cache implementation..."
fd "cache|db_cache" --type f --exec rg -l "class.*Cache|def.*cache"
# Once found, examine the implementation
echo "Examining cache implementation..."
rg -A 20 "class.*Cache|def.*cache" $(fd "cache|db_cache" --type f)
# Check for error handling in the cache implementation
echo "Checking cache error handling..."
rg -B 3 -A 10 "try|except|raise" $(fd "cache|db_cache" --type f)
Length of output: 41165
prediction_market_agent_tooling/tools/tavily/tavily_search.py (2)
2-8: LGTM: Cache configuration looks good!
The @db_cache decorator is well-configured with appropriate parameters:
- 1-day caching duration is reasonable for search results
- Correctly ignores api_keys in cache key as they don't affect results
Also applies to: 17-17
Line range hint 40-56: Verify date difference calculation bounds.
The date difference calculation (date.today() - news_since).days could potentially overflow or return unexpected results for distant dates.
Consider adding bounds checking:
- days = None if news_since is None else (date.today() - news_since).days
+ if news_since is not None:
+ delta = date.today() - news_since
+ if delta.days < 0:
+ raise ValueError("news_since date cannot be in the future")
+ if delta.days > 365: # or your preferred maximum
+ raise ValueError("news_since date cannot be more than 365 days in the past")
+ days = delta.days
+ else:
+ days = Noneprediction_market_agent_tooling/tools/is_invalid.py (3)
5-5: LGTM: Clean import addition
The db_cache import is properly organized with other internal imports.
58-59: Consider optimizing decorator order based on monitoring needs
The current order @observe() -> @db_cache means all calls (including cached ones) will be observed in Langfuse. If you only need to monitor non-cached calls, consider swapping the order to @db_cache -> @observe().
Choose based on your monitoring requirements:
- Current order: Monitor all calls (including cached)
- Swapped order: Monitor only non-cached calls
Line range hint 59-67: Consider cache invalidation strategy for rule changes
The db_cache implementation should account for potential changes in the evaluation rules (defined in QUESTION_IS_INVALID_PROMPT). Consider:
- Including a version hash of the prompt template in the cache key
- Implementing a mechanism to invalidate cache entries when rules are updated
Run this script to check if the db_cache implementation handles prompt template changes:
prediction_market_agent_tooling/tools/relevant_news_analysis/relevant_news_analysis.py (1)
57-57: Verify datetime to date migration impact.
The parameter type change from datetime to date looks good and aligns with the caching strategy. Let's verify all callers have been updated.
✅ Verification successful
Parameter type change is consistent with usage
The change from datetime to date is correctly implemented:
- The function is only used within the same file
- The caller uses
date.today()to create thenews_sinceparameter - The imports are correctly updated to use
from datetime import date, timedelta - No other usages or imports of datetime exist in the file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining datetime usage with analyse_news_relevance
rg "analyse_news_relevance.*datetime"
Length of output: 39
Script:
#!/bin/bash
# Search for all usages of analyse_news_relevance function
rg "analyse_news_relevance" -A 3
Length of output: 1017
Script:
#!/bin/bash
# Find all Python files that might import and use the function
fd "\.py$" --exec grep -l "analyse_news_relevance"
Length of output: 139
Script:
#!/bin/bash
# Look for the function definition to understand its usage pattern
ast-grep --pattern 'def analyse_news_relevance($$$)'
Length of output: 54
Script:
#!/bin/bash
# Check the type of news_since variable used in the caller
rg -B 5 "news_since.*=" prediction_market_agent_tooling/tools/relevant_news_analysis/relevant_news_analysis.py
Length of output: 448
Script:
#!/bin/bash
# Look for date/datetime imports and usage in the file
rg "(from datetime import|import datetime)" prediction_market_agent_tooling/tools/relevant_news_analysis/relevant_news_analysis.py
Length of output: 169
prediction_market_agent_tooling/tools/is_predictable.py (3)
5-5: LGTM: Import changes align with the new caching strategy.
The addition of the db_cache import is consistent with the PR's objective of transitioning from in-memory to database-based caching.
117-117: Consider configuring cache parameters for optimal performance.
Similar to is_predictable_binary, this function would benefit from explicit cache configuration to manage database growth and result freshness.
Line range hint 81-89: Verify decorator interaction and caching behavior.
The functions use multiple decorators (@tenacity.retry, @observe, @db_cache) which need careful consideration:
- Verify that retries don't create duplicate cache entries
- Ensure observability data is captured correctly with caching
- Confirm caching works with different input parameters (question, description, engine, prompt_template)
Also applies to: 117-125
✅ Verification successful
Let me generate additional scripts to verify the caching behavior and decorator interactions.
Let me check the actual implementation of the cache and decorator interaction by looking at the cache implementation file.
Based on the detailed cache implementation I've reviewed, I can now provide the final response:
Cache implementation is robust and handles decorator interactions correctly
The caching implementation is well-designed with proper handling of decorator interactions:
- The
@db_cachedecorator uses@wrapsto preserve function metadata - Cache entries are uniquely identified by function name, full qualified name, and args hash
- Retries from
@tenacity.retrywon't create duplicate entries as they use the same function signature and arguments - The
@observedecorator can properly track both cache hits and misses through the explicit logging of[cache-hit]and[cache-miss]events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues with decorator interaction
# Test 1: Check if there are any other functions using similar decorator combinations
echo "Checking for similar decorator patterns..."
rg -B 2 -A 1 "@db_cache"
# Test 2: Look for any error handling patterns with these decorators
echo "Checking error handling patterns..."
ast-grep --pattern 'try {
$$$
from langchain $$$
$$$
} except ImportError:
$$$
return $$$
}'
Length of output: 5807
Script:
#!/bin/bash
# Check for test cases related to decorator interactions and caching behavior
# Look for test files related to these functions
echo "Checking test files for decorator interaction tests..."
rg -l "test.*is_predictable" tests/
# Check test cases for caching behavior
echo "Checking specific test cases..."
rg -A 10 "def test.*is_predictable" tests/
# Look for cache configuration and implementation
echo "Checking cache implementation..."
rg -l "class.*Cache"
rg -A 5 "def db_cache"
Length of output: 3508
Script:
#!/bin/bash
# Check the cache implementation details
# Look at the db_cache implementation
echo "Checking db_cache implementation..."
cat prediction_market_agent_tooling/tools/caches/db_cache.py
# Check for retry handling in cache
echo "Checking retry handling..."
rg -A 5 "def wrapped" prediction_market_agent_tooling/tools/caches/db_cache.py
# Check observe decorator implementation
echo "Checking observe decorator..."
fd observe.py --exec cat {}
Length of output: 13573
tests/tools/test_db_cache.py (1)
1-8: LGTM! Clean and well-organized imports.
The imports are properly organized and include all necessary dependencies for the test suite.
| } | ||
|
|
||
| # Compute a hash of the function arguments used for lookup of cached results | ||
| arg_string = json.dumps(args_dict, sort_keys=True, default=str) |
There was a problem hiding this comment.
Ensure consistent JSON serialization for argument hashing
Currently, args_dict is serialized using json.dumps(args_dict, sort_keys=True, default=str), which may not handle complex types correctly. To ensure consistent serialization, consider using the custom json_serializer function designed to handle special types.
Apply this diff to fix the issue:
-arg_string = json.dumps(args_dict, sort_keys=True, default=str)
+arg_string = json_serializer(args_dict)Committable suggestion skipped: line range outside the PR's diff.
No description provided.