fix: compiler fixes #619
fix: compiler fixes #619
Conversation
WalkthroughThis pull request enhances code readability and output formatting across multiple modules. It introduces a Rich-based console for improved output, replaces direct print calls with Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as compile_v3
participant Sys as sys.path
participant Con as console
U->>C: call compile_v3(chronon_root)
C->>Con: print blank line
C->>Sys: check if chronon_root in sys.path
alt Not in path
C->>Con: print "adding chronon_root to sys.path"
C->>Sys: append chronon_root
else Already in path
C->>Con: print "chronon_root found in sys.path"
end
C->>U: return compilation result
sequenceDiagram
participant Caller as load_teams Caller
participant LT as load_teams
participant Con as console
Caller->>LT: call load_teams(conf_root, print)
alt print is True
LT->>Con: console.print "pulling team configuration"
end
LT->>Caller: return team dict
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: 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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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 compile_v3(chronon_root): | ||
|
|
||
| print() |
There was a problem hiding this comment.
wanted a newline actually
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
api/python/ai/chronon/cli/compile/compiler.py(4 hunks)api/python/ai/chronon/cli/compile/display/class_tracker.py(0 hunks)api/python/ai/chronon/cli/compile/display/compile_status.py(4 hunks)api/python/ai/chronon/cli/compile/display/console.py(1 hunks)api/python/ai/chronon/cli/compile/parse_configs.py(3 hunks)api/python/ai/chronon/cli/compile/parse_teams.py(3 hunks)api/python/ai/chronon/cli/entrypoint.py(0 hunks)api/python/ai/chronon/cli/plan/controller_iface.py(0 hunks)api/python/ai/chronon/cli/plan/physical_index.py(0 hunks)api/python/ai/chronon/cli/plan/physical_node.py(0 hunks)api/python/ai/chronon/join.py(1 hunks)api/python/ai/chronon/repo/compilev3.py(1 hunks)api/python/ai/chronon/repo/validator.py(1 hunks)api/python/ai/chronon/utils.py(1 hunks)
💤 Files with no reviewable changes (5)
- api/python/ai/chronon/cli/compile/display/class_tracker.py
- api/python/ai/chronon/cli/plan/controller_iface.py
- api/python/ai/chronon/cli/plan/physical_index.py
- api/python/ai/chronon/cli/plan/physical_node.py
- api/python/ai/chronon/cli/entrypoint.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
api/python/ai/chronon/cli/compile/parse_teams.py (1)
api/python/ai/chronon/cli/compile/compiler.py (1)
compile(30-59)
api/python/ai/chronon/cli/compile/display/compile_status.py (2)
api/python/ai/chronon/cli/compile/compiler.py (1)
compile(30-59)api/python/ai/chronon/cli/compile/display/diff_result.py (1)
render(12-46)
api/python/ai/chronon/utils.py (1)
api/python/ai/chronon/cli/compile/parse_teams.py (1)
load_teams(42-69)
api/python/ai/chronon/repo/compilev3.py (2)
api/python/ai/chronon/cli/compile/compiler.py (1)
compile(30-59)api/python/ai/chronon/cli/compile/compile_context.py (1)
CompileContext(25-166)
api/python/ai/chronon/cli/compile/compiler.py (2)
api/python/ai/chronon/cli/compile/display/compile_status.py (2)
render(66-85)print_live_console(25-27)api/python/ai/chronon/cli/compile/display/diff_result.py (1)
render(12-46)
🪛 Ruff (0.8.2)
api/python/ai/chronon/cli/compile/display/compile_status.py
7-7: ai.chronon.cli.compile.display.console imported but unused
Remove unused import: ai.chronon.cli.compile.display.console
(F401)
🔇 Additional comments (24)
api/python/ai/chronon/cli/compile/display/console.py (1)
1-3: Good addition of Rich's Console for better formattingCreates a centralized console instance for formatted output, replacing logger.
api/python/ai/chronon/repo/validator.py (1)
186-188: Nice fix for deterministic column orderingSorting the output columns addresses non-determinism in join outputs.
api/python/ai/chronon/utils.py (1)
327-327: Good update to suppress console outputPasses print=False to avoid unnecessary output when loading teams from utils.
api/python/ai/chronon/cli/compile/parse_teams.py (3)
15-15: Appropriate import for consoleImports the centralized console object for formatted output.
42-42: Good API enhancementAdding print parameter with default value maintains backward compatibility.
59-63: Clean implementation of conditional outputUses Rich formatting for better visibility when output is desired.
api/python/ai/chronon/cli/compile/parse_configs.py (3)
34-36: Good use of relative paths.Using relative paths to the chronon root makes the source file references more portable.
59-65: Better formatting.Improved readability by breaking the CompiledObj instantiation across multiple lines.
69-71: Consistent formatting.The multi-line format matches the style used for CompiledObj above.
api/python/ai/chronon/repo/compilev3.py (3)
4-4: Required import added.The sys module is needed for path manipulation.
16-16: Sensible default added.Using current working directory as default for chronon_root improves usability.
20-30: Path management improved.Added Python path handling helps ensure imports work correctly during compilation. Good user feedback included.
api/python/ai/chronon/join.py (1)
252-305:✅ Verification successful
API simplification: removed offset parameters.
Removed
left_start_offsetandleft_end_offsetparameters fromLabelPartsfunction, relying instead on training windows specified in label GroupBy-s.
🏁 Script executed:
#!/bin/bash # Check if there are any existing calls to LabelParts with the removed parameters rg "LabelParts\s*\(" --type py | grep -v "join\.py" | xargs grep -l "left_start_offset" || echo "No usage of left_start_offset found" rg "LabelParts\s*\(" --type py | grep -v "join\.py" | xargs grep -l "left_end_offset" || echo "No usage of left_end_offset found"Length of output: 653
Action: API simplification verified – offset parameters removed.
- In
api/python/ai/chronon/join.py, theleft_start_offsetandleft_end_offsetparameters have been removed fromLabelParts, relying on the training windows specified in labelGroupBy-s.- Shell script verification confirms there are no existing calls referencing these parameters.
api/python/ai/chronon/cli/compile/display/compile_status.py (3)
22-22: Quote style change.Maintaining consistent quote style.
66-66:✅ Verification successful
Method renamed to "render".
Renamed from
generate_update_display_textto the simplerrenderfor consistency with other UI components.
🏁 Script executed:
#!/bin/bash # Check if any code still calls the old method name rg "generate_update_display_text" --type py || echo "No references to old method name found"Length of output: 130
Rename Verified: Method now consistently uses "render".
- File:
api/python/ai/chronon/cli/compile/display/compile_status.py, Line 66- The shell script confirmed no remaining references to
generate_update_display_text.
90-95:✅ Verification successful
Temporary code removal.
Commented out display update code with a TODO for implementing live_crop. Ensure this doesn't break functionality.
🏁 Script executed:
#!/bin/bash # Look for code that might depend on _update_display functionality rg "\._update_display" --type py | grep -v "compile_status\.py" || echo "No external calls to _update_display found"Length of output: 160
Verified: Temporary Code Removal Appears Safe
The temporary removal of the live update is fine. The grep search found no external calls to
_update_display, so there’s no immediate risk. Please ensure regression tests cover this behavior until live_crop is implemented.api/python/ai/chronon/cli/compile/compiler.py (8)
7-7: Added Rich console importNew import adds the Rich-based console for formatted output, replacing logger in some places.
30-30: Simplified method signature formattingMethod signature formatting change improves readability.
37-37: Method call simplifiedMethod call now on single line. Cleaner code.
57-57: Replaced print with console.printChanged from direct printing to Rich console rendering, aligning with PR objective to replace logger with console.print.
101-103: Improved error message formattingError message formatting now more readable with consistent style.
105-106: Simplified traceback printingTraceback printing formatted consistently with error messages.
114-116: Consistent error formattingFile error message formatting matches the style used elsewhere.
118-118: Simplified traceback printingTraceback formatting consistent with other error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
api/python/ai/chronon/join.py (2)
252-257:⚠️ Potential issueParameter inconsistency in LabelParts function.
The function signature still includes
left_start_offsetandleft_end_offsetparameters, but they aren't included in the return statement. This mismatch could cause compiler errors.-def LabelParts( - labels: List[api.JoinPart], - left_start_offset: int, - left_end_offset: int, - label_offline_schedule: str = "@daily", -) -> api.LabelParts: +def LabelParts( + labels: List[api.JoinPart], + label_offline_schedule: str = "@daily", +) -> api.LabelParts:Also applies to: 302-305
500-509:⚠️ Potential issueRemove leftStartOffset and leftEndOffset from LabelParts initialization.
These parameters are still being used in the creation of the LabelParts object but aren't included in the function's return value.
- label_part = api.LabelParts( - labels=label_part.labels, - leftStartOffset=label_part.leftStartOffset, - leftEndOffset=label_part.leftEndOffset, - metaData=label_metadata, - ) + label_part = api.LabelParts( + labels=label_part.labels, + metaData=label_metadata, + )
♻️ Duplicate comments (1)
api/python/ai/chronon/repo/compilev3.py (1)
20-20: Remove empty print statementEmpty print statement serves no purpose.
- print()
🧹 Nitpick comments (1)
api/python/ai/chronon/cli/compile/display/compile_status.py (1)
7-7: Remove unused importStatic analysis shows this import isn't used in the file.
-from ai.chronon.cli.compile.display import console🧰 Tools
🪛 Ruff (0.8.2)
7-7:
ai.chronon.cli.compile.display.consoleimported but unusedRemove unused import:
ai.chronon.cli.compile.display.console(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (14)
api/python/ai/chronon/cli/compile/compiler.py(4 hunks)api/python/ai/chronon/cli/compile/display/class_tracker.py(0 hunks)api/python/ai/chronon/cli/compile/display/compile_status.py(4 hunks)api/python/ai/chronon/cli/compile/display/console.py(1 hunks)api/python/ai/chronon/cli/compile/parse_configs.py(3 hunks)api/python/ai/chronon/cli/compile/parse_teams.py(3 hunks)api/python/ai/chronon/cli/entrypoint.py(0 hunks)api/python/ai/chronon/cli/plan/controller_iface.py(0 hunks)api/python/ai/chronon/cli/plan/physical_index.py(0 hunks)api/python/ai/chronon/cli/plan/physical_node.py(0 hunks)api/python/ai/chronon/join.py(1 hunks)api/python/ai/chronon/repo/compilev3.py(1 hunks)api/python/ai/chronon/repo/validator.py(1 hunks)api/python/ai/chronon/utils.py(1 hunks)
💤 Files with no reviewable changes (5)
- api/python/ai/chronon/cli/plan/controller_iface.py
- api/python/ai/chronon/cli/plan/physical_node.py
- api/python/ai/chronon/cli/plan/physical_index.py
- api/python/ai/chronon/cli/compile/display/class_tracker.py
- api/python/ai/chronon/cli/entrypoint.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/python/ai/chronon/utils.py (1)
api/python/ai/chronon/cli/compile/parse_teams.py (1)
load_teams(43-70)
api/python/ai/chronon/repo/compilev3.py (2)
api/python/ai/chronon/cli/compile/compiler.py (2)
compile(30-59)Compiler(25-134)api/python/ai/chronon/cli/compile/compile_context.py (1)
CompileContext(25-166)
api/python/ai/chronon/cli/compile/display/compile_status.py (2)
api/python/ai/chronon/cli/compile/compiler.py (1)
compile(30-59)api/python/ai/chronon/cli/compile/display/diff_result.py (1)
render(12-46)
🪛 Ruff (0.8.2)
api/python/ai/chronon/cli/compile/display/compile_status.py
7-7: ai.chronon.cli.compile.display.console imported but unused
Remove unused import: ai.chronon.cli.compile.display.console
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
🔇 Additional comments (14)
api/python/ai/chronon/cli/compile/display/console.py (1)
1-3: Rich console implementation.Creates a reusable console object using Rich library for improved output formatting.
api/python/ai/chronon/repo/validator.py (1)
186-188: Column sorting adds determinism.Sorting output columns ensures consistent order in join outputs, eliminating non-deterministic behavior.
api/python/ai/chronon/utils.py (1)
327-330: Silent team loading and improved readability.Added print=False parameter to avoid unnecessary console output and improved code readability with line breaks in method chaining.
api/python/ai/chronon/cli/compile/parse_teams.py (3)
15-15: Console import added.Imports the rich console object for enhanced output formatting.
43-43: Added print parameter.Function signature updated with optional boolean parameter for controlling output.
59-64: Logger replaced with rich console.Conditional printing with rich console formatting instead of logging, improving output consistency.
api/python/ai/chronon/cli/compile/display/compile_status.py (1)
66-66:✅ Verification successful
Method renamed from generate_update_display_text to render
API change that aligns with the render method in other modules.
🏁 Script executed:
#!/bin/bash # Find any remaining calls to the old method name rg "generate_update_display_text" -t pyLength of output: 39
Verified Method Rename:
The method was successfully renamed torenderwith no remaining calls togenerate_update_display_textin the codebase.api/python/ai/chronon/repo/compilev3.py (2)
16-16: Good default parameter additionSetting default chronon_root to current directory improves usability.
22-30: Good addition of python path managementEnsures chronon_root is in sys.path for proper module imports.
api/python/ai/chronon/cli/compile/parse_configs.py (2)
33-35: Improved path handling with relative pathsUsing relative paths instead of absolute makes file references more portable.
58-64: Better code formattingMulti-line parameter formatting improves readability.
Also applies to: 68-70
api/python/ai/chronon/cli/compile/compiler.py (2)
7-7: Replaced print with console.printSwitched to Rich console for enhanced output formatting.
Also applies to: 57-57
101-103: Improved error message formattingBetter multi-line formatting for error messages and traceback printing.
Also applies to: 105-106, 114-116, 118-118
api/python/ai/chronon/join.py (1)
261-261: Documentation update matches implementation change.The documentation now correctly reflects that labels are joined based on user-specified settings within label GroupBy-s rather than using explicit offsets.
| # TODO: add this after live_crop is implemented | ||
| # text = self.display_text() | ||
| # if self.use_live: | ||
| # self.live.update(text, refresh=True) | ||
| # return text | ||
| pass |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commented-out implementation replaced with pass
The _update_display method now does nothing but is still called in multiple places.
🏁 Script executed:
#!/bin/bash
# Find all calls to _update_display to assess impact
rg -A 3 "_update_display" api/python/ai/chronon/cli/compile/display/compile_status.pyLength of output: 758
Issue: _update_display is a no-op and is called in multiple methods
- Called in: add_object_update_display, add_existing_object_update_display, close_cls, and close.
- Currently, it only executes a "pass" with a TODO note.
- Please add a minimal stub (e.g., logging) or remove redundant calls until live_crop is implemented.
Summary
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chores