Add utc timezones to OmenBet timestamps #440
Conversation
WalkthroughThe pull request introduces changes to the handling of timestamps in the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
prediction_market_agent_tooling/markets/omen/data_models.py (2)
404-404: LGTM! Timezone awareness added tocreation_datetime.The addition of
tz=pytz.UTCto thedatetime.fromtimestamp()call is correct and consistent with the change made to theopening_datetimeproperty. This modification ensures that the returned datetime is timezone-aware, addressing the timestamp discrepancy issue mentioned in the PR description.For consistency, consider updating other datetime-related properties in this file (e.g.,
finalized_datetime) to usetz=pytz.UTCas well. This would ensure all datetime objects in the module are timezone-aware.
Inconsistent timezone handling in datetime properties.
The analysis revealed that
datetime.fromtimestampis invoked without specifyingtz=pytz.UTCon the following lines:
- Line 86:
return datetime.fromtimestamp(self.openingTimestamp)- Line 261:
return datetime.fromtimestamp(self.creationTimestamp)To ensure consistent timezone awareness throughout the module and prevent potential discrepancies, please update these instances by adding
tz=pytz.UTC.🔗 Analysis chain
Line range hint
1-516: Summary: UTC timezone awareness added to critical datetime properties.The changes to
opening_datetimeandcreation_datetimeproperties successfully address the PR objective by adding UTC timezone awareness. This will resolve the one-hour discrepancy issue mentioned in the PR description, improving the accuracy of bet matching within the OmenBet platform.To ensure comprehensive timezone handling throughout the module:
- Review all datetime-related properties and methods in this file (e.g.,
finalized_datetime,close_time, etc.) and consider addingtz=pytz.UTCwhere appropriate.- Update any datetime comparisons or calculations in the codebase to account for these now timezone-aware datetime objects.
- Consider adding a utility function for creating timezone-aware datetime objects to maintain consistency and reduce code duplication.
To identify other datetime-related properties that might benefit from similar changes, run the following script:
Review the output to ensure all relevant datetime properties are updated for consistency.
To successfully identify all datetime-related properties and methods in
prediction_market_agent_tooling/markets/omen/data_models.py, please run the following script:Please execute this script to gather the necessary information for a comprehensive verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all datetime-related properties and methods in the file echo "Datetime-related properties and methods:" rg --type python -n '(def|return).*datetime.*fromtimestamp' prediction_market_agent_tooling/markets/omen/data_models.py echo "\nDatetime import and usage:" rg --type python -n 'from datetime import datetime' -A 10 prediction_market_agent_tooling/markets/omen/data_models.pyLength of output: 461
Script:
#!/bin/bash # Description: Find all datetime-related properties and methods in the specified Python file echo "Datetime-related properties and methods:" rg --type py -n '(def|return).*datetime.*fromtimestamp' prediction_market_agent_tooling/markets/omen/data_models.py echo "\nDatetime import and usage:" rg --type py -n 'from datetime import datetime' -A 10 prediction_market_agent_tooling/markets/omen/data_models.pyLength of output: 913
prediction_market_agent_tooling/tools/langfuse_client_utils.py (1)
161-164: Improve log message formatting for readabilityConsider combining the log message into a single f-string for better readability and to ensure the message is formatted correctly.
Apply this diff to adjust the log message:
logger.info( - f"Closest trace to bet has timestamp {candidate_trace.timestamp}, " - f"but bet was created at {bet_timestamp}. Not matching" + f"Closest trace to bet has timestamp {candidate_trace.timestamp}, but bet was created at {bet_timestamp}. Not matching" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent_tooling/markets/omen/data_models.py (3 hunks)
- prediction_market_agent_tooling/tools/langfuse_client_utils.py (2 hunks)
🔇 Additional comments (1)
prediction_market_agent_tooling/markets/omen/data_models.py (1)
222-222: LGTM! Timezone awareness added toopening_datetime.The addition of
tz=pytz.UTCto thedatetime.fromtimestamp()call is correct and aligns with the PR objective. This change makes the returned datetime timezone-aware, which is crucial for accurate timestamp comparisons and prevents the one-hour discrepancy mentioned in the PR description.
| # Sanity check: Let's say the upper bound for time between | ||
| # `agent.process_market` being called and the bet being placed is 20 | ||
| # minutes | ||
| candidate_trace = traces_for_bet[closest_trace_index] | ||
| if abs(candidate_trace.timestamp - bet_timestamp).total_seconds() > 1200: | ||
| logger.info( | ||
| f"Closest trace to bet has timestamp {candidate_trace.timestamp}, " | ||
| f"but bet was created at {bet_timestamp}. Not matching" | ||
| ) | ||
| return None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize the time threshold for better maintainability
The hard-coded time threshold of 1200 seconds (20 minutes) can be made more maintainable by defining it as a constant or configurable parameter. This approach allows for easier adjustments in the future without modifying the core logic.
Apply this diff to refactor the time threshold:
+TIME_THRESHOLD_SECONDS = 1200 # Upper bound for time difference between agent processing and bet placement
candidate_trace = traces_for_bet[closest_trace_index]
-if abs(candidate_trace.timestamp - bet_timestamp).total_seconds() > 1200:
+if abs(candidate_trace.timestamp - bet_timestamp).total_seconds() > TIME_THRESHOLD_SECONDS:
logger.info(
f"Closest trace to bet has timestamp {candidate_trace.timestamp}, "
f"but bet was created at {bet_timestamp}. Not matching"
)
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Sanity check: Let's say the upper bound for time between | |
| # `agent.process_market` being called and the bet being placed is 20 | |
| # minutes | |
| candidate_trace = traces_for_bet[closest_trace_index] | |
| if abs(candidate_trace.timestamp - bet_timestamp).total_seconds() > 1200: | |
| logger.info( | |
| f"Closest trace to bet has timestamp {candidate_trace.timestamp}, " | |
| f"but bet was created at {bet_timestamp}. Not matching" | |
| ) | |
| return None | |
| TIME_THRESHOLD_SECONDS = 1200 # Upper bound for time difference between agent processing and bet placement | |
| # Sanity check: Let's say the upper bound for time between | |
| # `agent.process_market` being called and the bet being placed is 20 | |
| # minutes | |
| candidate_trace = traces_for_bet[closest_trace_index] | |
| if abs(candidate_trace.timestamp - bet_timestamp).total_seconds() > TIME_THRESHOLD_SECONDS: | |
| logger.info( | |
| f"Closest trace to bet has timestamp {candidate_trace.timestamp}, " | |
| f"but bet was created at {bet_timestamp}. Not matching" | |
| ) | |
| return None |
| bet_timestamp = add_utc_timezone_validator(bet.created_time) | ||
| closest_trace_index = get_closest_datetime_from_list( | ||
| add_utc_timezone_validator(bet.created_time), | ||
| bet_timestamp, |
There was a problem hiding this comment.
💡 Codebase verification
candidate_trace.timestamp is not timezone-aware
The trace.timestamp is assigned directly without applying add_utc_timezone_validator, which may lead to timezone-related comparison errors.
- File:
prediction_market_agent_tooling/tools/langfuse_client_utils.py- Line: 150-152
🔗 Analysis chain
Ensure candidate_trace.timestamp is timezone-aware when comparing datetimes
While bet_timestamp is made timezone-aware using add_utc_timezone_validator, it's important to ensure that candidate_trace.timestamp is also timezone-aware. This prevents potential errors or incorrect time difference calculations when comparing the two datetime objects.
Run the following script to verify that candidate_trace.timestamp is timezone-aware:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'trace.timestamp' used in ProcessMarketTrace is timezone-aware.
# Expectation: All instances where 'ProcessMarketTrace' is instantiated should ensure 'timestamp' is timezone-aware.
# Search for 'timestamp=trace.timestamp' assignments
rg --type py 'timestamp=trace.timestamp' -A 5 -B 5
# Check if 'add_utc_timezone_validator' is applied to 'trace.timestamp'
rg --type py 'timestamp=add_utc_timezone_validator\(trace.timestamp\)' -A 5 -B 5
Length of output: 1054
Script:
#!/bin/bash
# Description: Check if 'add_utc_timezone_validator' is imported in 'langfuse_client_utils.py'
# Search for import statements of 'add_utc_timezone_validator'
rg --type py 'from .* import add_utc_timezone_validator'
# Description: Find all usages of 'trace.timestamp' to verify if it's validated
# Search for all instances where 'trace.timestamp' is used
rg --type py 'trace\.timestamp'
# Description: Inspect the 'ProcessMarketTrace' class or function for internal timestamp handling
# Search for the definition of 'ProcessMarketTrace' and inspect its handling of 'timestamp'
rg --type py 'class ProcessMarketTrace' -A 20
Length of output: 2657
Otherwise bet timezones are one hour out, and you get a bunch of no-matches, e.g.: