Add a new validator to convert blockchain timestamps to correct datetime #438
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the modification of timezone validation methods. The function Changes
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 (
|
|
|
||
| def utc_timestamp_to_utc_datetime(ts: int | None) -> DatetimeWithTimezone | None: | ||
| return ( | ||
| convert_to_utc_datetime(datetime.fromtimestamp(ts, tz=pytz.UTC)) |
There was a problem hiding this comment.
There's a difference between adding timezone into this function and not:
>>> from datetime import datetime
>>> import pytz
>>> datetime.fromtimestamp(1727349783)
datetime.datetime(2024, 9, 26, 13, 23, 3)
>>> datetime.fromtimestamp(1727349783, tz=pytz.UTC)
datetime.datetime(2024, 9, 26, 11, 23, 3, tzinfo=<UTC>)
>>>
@coderabbitai Please create an issue for me to go over the code and check where all we need to fix.
There was a problem hiding this comment.
@kongzii, I've created an issue for you to review the code and check where timezone handling needs to be fixed: #439.
🔗 Analysis chain
@kongzii, I've created an issue for you to review the code and check where timezone handling needs to be fixed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue to review timezone handling
gh issue create --title "Review code for correct timezone handling" --body "As per PR [#438](https://github.com/gnosis/prediction-market-agent-tooling/pull/438), there is a difference in how \`datetime.fromtimestamp()\` handles timezones when a timezone is specified or not. We need to review the codebase to identify where timezone handling needs to be fixed.
Refer to the comment: https://github.com/gnosis/prediction-market-agent-tooling/pull/438#discussion_r1776869154
Requested by @kongzii
" --assignee kongzii
Length of output: 586
✏️ Learnings added
Learnt from: kongzii
PR: gnosis/prediction-market-agent-tooling#438
File: prediction_market_agent_tooling/tools/utils.py:124-124
Timestamp: 2024-09-26T11:28:10.335Z
Learning: When using `datetime.fromtimestamp()`, always specify the timezone argument to ensure correct timezone handling.
Learnt from: kongzii
PR: gnosis/prediction-market-agent-tooling#438
File: prediction_market_agent_tooling/tools/utils.py:124-124
Timestamp: 2024-09-26T11:28:10.335Z
Learning: When using datetime.fromtimestamp(), specify the timezone argument to ensure correct timezone handling.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| ) | ||
|
|
||
|
|
||
| UTCDatetimeFromUTCTimestamp = t.Annotated[ |
There was a problem hiding this comment.
Not used yet, but needs to. I will take a fresh look at it next week, because it's all over the codebase.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
.github/workflows/python_ci.yaml (1)
70-70: Approved: Increased visibility for autoflake changesRemoving the
--quietflag from thegit diffcommand will make the differences found by autoflake visible in the CI logs. This change improves transparency and can help developers understand what automated fixes are being applied.Consider adding a conditional to only show the diff if changes were made:
- git diff --exit-code || exit 1 + git diff --exit-code || (git diff && exit 1)This way, the diff is only shown when there are actual changes, reducing noise in the logs when no changes are needed.
prediction_market_agent_tooling/tools/langfuse_client_utils.py (1)
150-150: LGTM: Improved timestamp handling with UTC conversion.The use of
convert_to_utc_datetimeforbet.created_timeensures consistent UTC-based timestamp comparison, addressing the PR objective. This change improves the accuracy of finding the closest trace to a bet.Consider updating the variable name
bet_timestamptobet_utc_timestampfor clarity:- bet_timestamp = convert_to_utc_datetime(bet.created_time) + bet_utc_timestamp = convert_to_utc_datetime(bet.created_time)This small change would make it explicit that we're working with UTC timestamps throughout the function.
prediction_market_agent_tooling/tools/utils.py (3)
Line range hint
89-109: LGTM: Improved function naming and type hintingThe renaming of
add_utc_timezone_validatortoconvert_to_utc_datetimebetter reflects the function's purpose. The added type hints and overloads enhance type safety and IDE support.One minor suggestion:
Consider adding a more descriptive docstring explaining the function's behavior, especially regarding the assumption of UTC for timezone-naive datetimes. For example:
def convert_to_utc_datetime(value: datetime | None) -> DatetimeWithTimezone | None: """ Convert a datetime object to UTC. If the input datetime is timezone-naive, it is assumed to be in UTC. If the input datetime has a non-UTC timezone, it is converted to UTC. Args: value (datetime | None): The datetime to convert or None. Returns: DatetimeWithTimezone | None: The UTC datetime or None if the input is None. """ # ... (rest of the function remains the same)
112-127: LGTM: New function for timestamp to UTC datetime conversionThe new
utc_timestamp_to_utc_datetimefunction is a valuable addition for handling UTC timestamps. It correctly usesconvert_to_utc_datetimeto ensure consistency in timezone handling.One minor suggestion:
Consider adding a docstring to explain the function's purpose and behavior, especially regarding the handling of None values. For example:
def utc_timestamp_to_utc_datetime(ts: int | None) -> DatetimeWithTimezone | None: """ Convert a UTC timestamp (in seconds) to a UTC datetime. Args: ts (int | None): The UTC timestamp in seconds or None. Returns: DatetimeWithTimezone | None: The corresponding UTC datetime or None if the input is None. """ return ( convert_to_utc_datetime(datetime.fromtimestamp(ts, tz=pytz.UTC)) if ts is not None else None )
130-132: LGTM: New type annotation for automatic timestamp conversionThe
UTCDatetimeFromUTCTimestamptype annotation is a clever use of Pydantic's validation system to ensure consistent handling of UTC timestamps. It will automatically convert timestamps to UTC datetimes when used in Pydantic models.Suggestion:
Consider adding a comment or docstring to explain the purpose and usage of this type annotation. For example:
UTCDatetimeFromUTCTimestamp = t.Annotated[ datetime, BeforeValidator(utc_timestamp_to_utc_datetime) ] """ A type annotation for automatic conversion of UTC timestamps to UTC datetimes. Use this type in Pydantic models to automatically convert int timestamps to datetime objects. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- .github/workflows/python_ci.yaml (2 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (2 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (2 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (2 hunks)
- prediction_market_agent_tooling/tools/contract.py (2 hunks)
- prediction_market_agent_tooling/tools/langfuse_client_utils.py (2 hunks)
- prediction_market_agent_tooling/tools/utils.py (3 hunks)
🔇 Additional comments (19)
.github/workflows/python_ci.yaml (2)
80-80: Consistent change for isort jobThis change is identical to the one made in the
autoflakejob, maintaining consistency across the workflow. The same benefits and considerations apply.Please refer to the comment on line 70 for a suggested improvement that could also be applied here.
Line range hint
1-80: Summary: Improved visibility in CI workflowThe changes made to this workflow file are minor but beneficial. By removing the
--quietflag fromgit diffcommands in both theautoflakeandisortjobs, the workflow now provides more visibility into the automated code formatting process. This can help developers quickly identify and understand any formatting issues caught by the CI process.These changes do not alter the core functionality of the workflow but enhance its usefulness for debugging and maintaining code quality. Consider implementing the suggested conditional diff output to further optimize the balance between visibility and log clarity.
prediction_market_agent_tooling/tools/langfuse_client_utils.py (2)
17-17: LGTM: New import for UTC datetime conversion.The addition of
convert_to_utc_datetimefrom the utils module aligns with the PR objective of improving timestamp handling. This import supports the changes in theget_trace_for_betfunction.
Line range hint
1-164: Overall assessment: Improvements in timestamp handling.The changes in this file successfully address the PR objective of converting blockchain timestamps to correct datetime. The introduction of
convert_to_utc_datetimeand its usage in theget_trace_for_betfunction enhance the accuracy of timestamp comparisons. The modifications are focused and do not introduce unnecessary complexity to the existing codebase.prediction_market_agent_tooling/tools/utils.py (4)
12-12: LGTM: New import for BeforeValidatorThe addition of
BeforeValidatorfrompydantic.functional_validatorsis consistent with the changes made to improve datetime handling and validation in this file.
134-136: LGTM: Simplified utcnow functionThe change to use
convert_to_utc_datetimein theutcnowfunction ensures consistency with the new timezone handling approach and simplifies the implementation.
124-124: Addressed: Timezone handling in datetime.fromtimestamp()The previous concern about timezone handling in
datetime.fromtimestamp()has been addressed in the newutc_timestamp_to_utc_datetimefunction. This function explicitly specifies the UTC timezone when creating datetime objects from timestamps.
130-130: Resolved: Usage of timezone conversion functionsThe previous comment about unused code seems to be outdated. The new changes introduce usage of the timezone conversion functions throughout the file, improving consistency in datetime handling.
prediction_market_agent_tooling/markets/agent_market.py (4)
22-22: LGTM: Import ofconvert_to_utc_datetimefunction.The import of
convert_to_utc_datetimeis correctly placed with other imports from the same module. This function will be used in the updated validators, which is a good practice for handling timezone conversions.
Line range hint
52-52: LGTM: Corrected indentation forvolumeattribute.The indentation correction for the
volumeattribute improves code readability and consistency with other attributes in the class. This change doesn't affect functionality but enhances code quality.
Line range hint
1-338: Overall assessment: Changes improve datetime handling and code quality.The modifications in this file successfully address the PR objective of converting blockchain timestamps to correct datetime. The consistent use of
convert_to_utc_datetimein validators and the correction of indentation contribute to better code quality and maintainability.To ensure a smooth transition, consider the following:
- Update any documentation or comments that might reference the old
add_utc_timezone_validatorfunction.- If there are unit tests for the
AgentMarketclass, make sure they are updated to reflect these changes.- Verify that any code depending on the
created_timeandclose_timefields still functions correctly with the new UTC conversion.
64-67: LGTM: Updated validators forcreated_timeandclose_time.The change from
add_utc_timezone_validatortoconvert_to_utc_datetimealigns with the PR objective of converting blockchain timestamps to correct datetime. The implementation is consistent across both validators.To ensure this change doesn't have unintended consequences, let's verify its usage across the codebase:
✅ Verification successful
Action Required: Re-verify Validator Changes with Updated Shell Scripts
Let's run updated shell scripts to accurately check the usage of
add_utc_timezone_validatorandconvert_to_utc_datetime:
: Validators have been successfully updated.
All instances of
add_utc_timezone_validatorhave been removed, andconvert_to_utc_datetimeis consistently used throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of `add_utc_timezone_validator` and verify `convert_to_utc_datetime` usage. echo "Checking for any remaining usage of add_utc_timezone_validator:" rg "add_utc_timezone_validator" --type python echo "\nVerifying usage of convert_to_utc_datetime:" rg "convert_to_utc_datetime" --type pythonLength of output: 388
Script:
# Re-checking for any remaining usage of add_utc_timezone_validator and verifying convert_to_utc_datetime usage with correct file type echo "Checking for any remaining usage of add_utc_timezone_validator:" rg "add_utc_timezone_validator" --type py echo "\nVerifying usage of convert_to_utc_datetime:" rg "convert_to_utc_datetime" --type pyLength of output: 1840
prediction_market_agent_tooling/monitor/monitor.py (3)
31-31: LGTM: Import change aligns with PR objectiveThe addition of
convert_to_utc_datetimeto the imports is consistent with the changes in theDeployedAgentclass and supports the PR objective of converting blockchain timestamps to correct datetime.
52-56: LGTM: Timezone conversion update inDeployedAgentclassThe changes in
_add_timezone_validator_start_timeand_add_timezone_validator_end_timemethods to useconvert_to_utc_datetimeare consistent and align with the PR objective. The validation mechanism remains intact, and backward compatibility is maintained.
Line range hint
1-458: Overall impact: Improved timezone handling with minimal changesThe changes are localized to the
DeployedAgentclass and its timezone validation methods. The rest of the file, including the monitoring functions, remains unchanged. This update improves the handling of blockchain timestamps while maintaining the existing functionality of the monitoring system.prediction_market_agent_tooling/tools/contract.py (2)
27-27: LGTM: Import change is consistent with implementation update.The import of
utc_timestamp_to_utc_datetimeis correctly placed and aligns with the changes made in theget_nowmethod of theDebuggingContractclass.
456-456: LGTM: Implementation update simplifies timestamp conversion.The change from
add_utc_timezone_validatortoutc_timestamp_to_utc_datetimesimplifies the conversion process while maintaining the same functionality. The method signature and return type remain consistent, ensuring backward compatibility.prediction_market_agent_tooling/monitor/monitor_app.py (2)
32-32: Appropriate imports for UTC datetime conversionThe addition of
convert_to_utc_datetimeandutcnowto the imports is necessary for handling UTC datetime conversions properly in the application.
Line range hint
107-117: Correct usage ofconvert_to_utc_datetimeforstart_timeThe replacement of
add_utc_timezone_validatorwithconvert_to_utc_datetimeensures thatstart_timeis converted to a timezone-aware UTC datetime. This is essential for accurate time comparisons and scheduling within the application.
No description provided.