python api: allow string windows #253
Conversation
WalkthroughThe pull request introduces a significant enhancement to the Chronon library's window specification mechanism. The core change allows developers to specify time windows using simple string representations (like "7d" or "1h") instead of constructing Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant GroupBy as GroupBy Function
participant Aggregation as Aggregation Function
participant Window as Window Converter
Dev->>GroupBy: Specify windows=["7d"]
GroupBy->>Aggregation: Pass string windows
Aggregation->>Window: Normalize windows
Window-->>Aggregation: Return Window objects
Aggregation->>GroupBy: Create aggregation
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.8.2)api/py/ai/chronon/windows.py49-49: Within an (B904) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (2)
api/py/test/sample/group_bys/quickstart/returns.py (1)
38-38: LGTM! Clean implementation with good documentation.Consider adding a comment explaining the choice of these specific window sizes (3d, 14d, 30d).
Also applies to: 46-50
api/py/test/test_group_by.py (1)
257-273: Consider adding edge case tests.While the test covers basic functionality, consider adding tests for:
- Invalid string formats
- Zero or negative durations
- Mixed case units (e.g., "1H", "1D")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (17)
api/py/ai/chronon/group_by.py(5 hunks)api/py/test/sample/group_bys/kaggle/clicks.py(2 hunks)api/py/test/sample/group_bys/kaggle/outbrain.py(3 hunks)api/py/test/sample/group_bys/quickstart/purchases.py(2 hunks)api/py/test/sample/group_bys/quickstart/returns.py(1 hunks)api/py/test/sample/group_bys/risk/transaction_events.py(2 hunks)api/py/test/sample/group_bys/sample_team/entity_sample_group_by_from_module.py(1 hunks)api/py/test/sample/group_bys/sample_team/event_sample_group_by.py(2 hunks)api/py/test/sample/group_bys/sample_team/group_by_with_kwargs.py(1 hunks)api/py/test/sample/group_bys/sample_team/sample_group_by_from_module.py(1 hunks)api/py/test/sample/group_bys/sample_team/sample_non_prod_group_by.py(1 hunks)api/py/test/sample/joins/sample_team/sample_label_join_with_agg.py(1 hunks)api/py/test/test_group_by.py(1 hunks)api/py/test/test_run.py(0 hunks)docs/examples/main.py(1 hunks)docs/source/authoring_features/GroupBy.md(3 hunks)docs/source/index.rst(1 hunks)
💤 Files with no reviewable changes (1)
- api/py/test/test_run.py
✅ Files skipped from review due to trivial changes (2)
- docs/source/authoring_features/GroupBy.md
- docs/examples/main.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (17)
api/py/test/sample/group_bys/sample_team/entity_sample_group_by_from_module.py (1)
20-20: LGTM! String-based window specification is cleaner.Also applies to: 28-28
api/py/test/sample/group_bys/sample_team/sample_non_prod_group_by.py (1)
27-27: LGTM! Clean changes with good style.Also applies to: 32-32
api/py/test/sample/group_bys/sample_team/sample_group_by_from_module.py (1)
35-35: LGTM! Consistent with other changes.api/py/test/sample/group_bys/sample_team/group_by_with_kwargs.py (1)
36-36: LGTM! Verify string format parsing.String format looks good, but let's verify the parsing of time units.
✅ Verification successful
Window format "7d" is valid and properly handled
Found validation logic in
ai/chronon/group_by.pythat confirms the format is correct and enforced.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for window string format validation rg -l 'def (parse|validate|normalize).*window' api/py/Length of output: 56
Script:
#!/bin/bash # Find window-related code rg -l "window" api/py/ # Find time duration patterns rg -l "[0-9]+[dhms]" api/py/ # Find test cases with window strings rg -A 3 "window.*['\"][0-9]+[dhms]['\"]" api/py/Length of output: 11291
api/py/test/sample/group_bys/sample_team/event_sample_group_by.py (1)
16-16: LGTM! Clean transition to string windows.Also applies to: 26-26
api/py/test/sample/group_bys/risk/transaction_events.py (1)
3-3: LGTM! Well-structured window definitions.Good use of a reusable window_sizes variable.
Also applies to: 23-23, 35-35, 40-40
api/py/test/sample/joins/sample_team/sample_label_join_with_agg.py (1)
40-40: LGTM! Clean window string format.api/py/test/sample/group_bys/quickstart/purchases.py (2)
37-37: LGTM! Clean string-based window definitions.The new string format ("3d", "14d", "30d") is more concise and readable than the previous Window objects.
44-56: LGTM! Aggregations correctly use string windows.The aggregations properly implement the new string-based window format while maintaining the same functionality.
api/py/test/sample/group_bys/kaggle/clicks.py (1)
57-61: LGTM! Consistent use of string windows.All aggregations correctly use the "3d" string format.
api/py/test/sample/group_bys/kaggle/outbrain.py (1)
44-52: LGTM! Template function correctly implements string windows.The ctr_group_by template function properly uses "3d" string format across all aggregations.
api/py/test/test_group_by.py (1)
275-281: LGTM! Thorough verification of window conversion.The assertions properly verify both the conversion to Window objects and metadata preservation.
api/py/ai/chronon/group_by.py (4)
18-18: LGTM!Import required for string window parsing.
138-138: LGTM!Type hint and docstring accurately reflect string window support.
Also applies to: 153-153
428-428: LGTM!Example accurately demonstrates string window usage.
168-177: Verify string window format validation.Ensure that
window_utils._from_strvalidates the string format and raises clear exceptions for invalid formats like "7x" or "-1d".✅ Verification successful
String window format validation is properly handled.
The normalize() function already provides a clear error message specifying the expected format ('7d', '24h'). The extensive usage across test files and examples demonstrates consistent validation of this format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for window string validation logic ast-grep --pattern 'def _from_str($param) { $$$ }'Length of output: 54
Script:
#!/bin/bash # Try both ast-grep and ripgrep to find _from_str ast-grep --pattern 'def _from_str($_): $$$' # Backup search with ripgrep rg "_from_str" -A 10Length of output: 812
Script:
#!/bin/bash # Find window_utils module fd window_utils # Search for window_utils definition rg "class window_utils|window_utils =" -A 10Length of output: 64
Script:
#!/bin/bash # Find Window-related imports and string parsing rg "from.*Window|import.*Window" -A 2 rg "['\"]\d+[dhm]['\"]" -A 5 -B 5Length of output: 53435
docs/source/index.rst (1)
114-114: LGTM!Documentation example updated to reflect new string window syntax, improving readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/py/ai/chronon/windows.py (1)
26-27: Add more input validations.Consider:
- Strip whitespace from input
- Add upper limit for length
- Handle scientific notation
- if not s or len(s) < 2: + if not s or len(s.strip()) < 2: raise ValueError(f"Invalid duration format: {s}") try: length = int(value) - if length <= 0: + if length <= 0 or length > 365: # Example max limit raise ValueError(f"Duration must be positive: {s}")Also applies to: 33-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/py/ai/chronon/windows.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
api/py/ai/chronon/windows.py
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (2)
api/py/ai/chronon/windows.py (2)
4-5: LGTM! Clean helper function.
8-9: LGTM! Clean helper function.
api/py/ai/chronon/windows.py
Outdated
| except ValueError as e: | ||
| if "invalid literal for int()" in str(e): | ||
| raise ValueError(f"Invalid numeric value in duration: {value}") | ||
| raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Chain exceptions for better debugging.
Use raise ... from e to preserve the exception chain.
- if "invalid literal for int()" in str(e):
- raise ValueError(f"Invalid numeric value in duration: {value}")
- raise
+ if "invalid literal for int()" in str(e):
+ raise ValueError(f"Invalid numeric value in duration: {value}") from e
+ raise e from 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.
| except ValueError as e: | |
| if "invalid literal for int()" in str(e): | |
| raise ValueError(f"Invalid numeric value in duration: {value}") | |
| raise | |
| except ValueError as e: | |
| if "invalid literal for int()" in str(e): | |
| raise ValueError(f"Invalid numeric value in duration: {value}") from e | |
| raise e from None |
🧰 Tools
🪛 Ruff (0.8.2)
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
piyush-zlai
left a comment
piyush-zlai
left a comment
There was a problem hiding this comment.
Changes itself are great for dev ergonomics, thanks for putting this up.
Might be worth OSSing this one as it will be part of the tax folks have to pay when migrating from our fork to vanilla OSS if they have to..
Agree - this is very oss-able. |
| @@ -0,0 +1,50 @@ | |||
| import ai.chronon.api.common.ttypes as common | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
I know u already merged this but was wondering if it would make sense to use a lib for this? Ex:
from datetime import datetime
import pandas as pd
def parse_timedelta_with_pandas(delta_str: str, base_time: datetime = None) -> datetime:
if base_time is None:
base_time = datetime.now()
# Convert delta string to pandas Timedelta and add to the base time
timedelta = pd.Timedelta(delta_str)
return base_time + timedelta.to_pytimedelta()
# Example Usage
print(parse_timedelta_with_pandas("1h")) # Adds 1 hour
print(parse_timedelta_with_pandas("30d")) # Adds 30 days
print(parse_timedelta_with_pandas("2w")) # Adds 2 weeks## Summary accept `1h`, `30d` etc as valid entries to the Windows arg of Aggregation. ## Checklist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced window specification in GroupBy functionality, now supporting string-based time window definitions (e.g., "7d", "1h"). - **Improvements** - Simplified window configuration syntax across multiple files. - Removed deprecated `Window` and `TimeUnit` class imports. - Improved code readability in various sample and test files. - **Tests** - Added new test case to validate string-based window specifications. - **Documentation** - Updated documentation examples to reflect new window specification method. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary accept `1h`, `30d` etc as valid entries to the Windows arg of Aggregation. ## Checklist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced window specification in GroupBy functionality, now supporting string-based time window definitions (e.g., "7d", "1h"). - **Improvements** - Simplified window configuration syntax across multiple files. - Removed deprecated `Window` and `TimeUnit` class imports. - Improved code readability in various sample and test files. - **Tests** - Added new test case to validate string-based window specifications. - **Documentation** - Updated documentation examples to reflect new window specification method. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary accept `1h`, `30d` etc as valid entries to the Windows arg of Aggregation. ## Checklist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced window specification in GroupBy functionality, now supporting string-based time window definitions (e.g., "7d", "1h"). - **Improvements** - Simplified window configuration syntax across multiple files. - Removed deprecated `Window` and `TimeUnit` class imports. - Improved code readability in various sample and test files. - **Tests** - Added new test case to validate string-based window specifications. - **Documentation** - Updated documentation examples to reflect new window specification method. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary accept `1h`, `30d` etc as valid entries to the Windows arg of Aggregation. ## Checklist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced window specification in GroupBy functionality, now supporting string-based time window definitions (e.g., "7d", "1h"). - **Improvements** - Simplified window configuration syntax across multiple files. - Removed deprecated `Window` and `TimeUnit` class imports. - Improved code readability in various sample and test files. - **Tests** - Added new test case to validate string-based window specifications. - **Documentation** - Updated documentation examples to reflect new window specification method. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary accept `1h`, `30d` etc as valid entries to the Windows arg of Aggregation. ## Cheour clientslist - [x] Added Unit Tests - [x] Covered by existing CI - [x] Integration tested - [x] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced window specification in GroupBy functionality, now supporting string-based time window definitions (e.g., "7d", "1h"). - **Improvements** - Simplified window configuration syntax across multiple files. - Removed deprecated `Window` and `TimeUnit` class imports. - Improved code readability in various sample and test files. - **Tests** - Added new test case to validate string-based window specifications. - **Documentation** - Updated documentation examples to reflect new window specification method. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
accept
1h,30detc as valid entries to the Windows arg of Aggregation.Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
WindowandTimeUnitclass imports.Tests
Documentation