compiler cutover #507
compiler cutover #507
Conversation
WalkthroughThis pull request introduces a new Changes
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. 🪧 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 (1)
api/py/ai/chronon/repo/zipline.py (1)
16-23: Fix error message inconsistency.Error message refers to "teams.json" but code checks for "team.json".
- "teams.py or teams.json file not found in current directory." + "teams.py or team.json file not found in current directory."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/py/ai/chronon/repo/zipline.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: python_tests
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
🔇 Additional comments (4)
api/py/ai/chronon/repo/zipline.py (4)
1-1: Imports look good.New imports support the compile functionality.
Also applies to: 4-5
14-15: Command definition is clean.Properly adds compile command to CLI.
30-30: Command registration is correct.Properly adds new compile command to zipline group.
25-27:✅ Verification successful
Consider simplifying compiler initialization.
Passing compile_context twice seems redundant.
compile_context = CompileContext() compiler = Compiler(compile_context) - compiler.compile(compile_context) + compiler.compile()
🏁 Script executed:
#!/bin/bash # Check Compiler class implementation to verify if compile method requires context rg -A 5 "class Compiler" --glob "*.py" rg -A 5 "def compile\(" --context 5 --glob "*.py" | grep "Compiler"Length of output: 557
Simplify Compiler Usage: Remove redundant context parameter.
The Compiler is initialized with a compile context, so calling
compiler.compile(compile_context)is unnecessary. Instead, callcompiler.compile().
- File:
api/py/ai/chronon/repo/zipline.py(Lines 25-27)compile_context = CompileContext() compiler = Compiler(compile_context) -compiler.compile(compile_context) +compiler.compile()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/py/ai/chronon/cli/logger.py (2)
56-56: Emoji replaced with textChanged error indicator from emoji to text format for better compatibility.
Consider extracting "X: " as a constant for maintainability.
61-61: Emoji replaced with textChanged success indicator from emoji to text format.
Consider extracting "DONE: " as a constant. Also,
condparameter is unused.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/py/ai/chronon/repo/zipline.py (3)
26-28: Redundant context passing.Compiler already receives compile_context in constructor, but it's passed again to compile method.
compile_context = CompileContext() compiler = Compiler(compile_context) -compiler.compile(compile_context) +compiler.compile()
15-29: Missing error handling.Add try-except block to catch and handle potential compilation errors.
@click.command(name="compile") def compile(): # check that a "teams.py" file exists in the current directory if not (os.path.exists("teams.py") or os.path.exists("teams.json")): raise click.ClickException( ( "teams.py or teams.json file not found in current directory." " Please run from the top level of conf directory." ) ) compile_context = CompileContext() compiler = Compiler(compile_context) + try: compiler.compile(compile_context) + except Exception as e: + raise click.ClickException(f"Compilation failed: {str(e)}")
15-16: Consider adding command options.The compile command might benefit from options like output directory or verbosity level.
-@click.command(name="compile") -def compile(): +@click.command(name="compile") +@click.option("--output", "-o", default=".", help="Output directory for compiled files") +@click.option("--verbose", "-v", is_flag=True, help="Enable verbose output") +def compile(output, verbose):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/py/ai/chronon/repo/zipline.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (3)
api/py/ai/chronon/repo/zipline.py (3)
1-1: Appropriate imports added.Adds necessary imports for OS operations and compilation components.
Also applies to: 4-5
15-24: File existence check is implemented correctly.Ensures either teams.py or teams.json exists before proceeding with compilation.
31-31: Command properly added to zipline group.The compile command is correctly registered with the zipline command group.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
api/py/ai/chronon/repo/zipline.py (3)
15-29: New compile command looks good but lacks error handlingMissing error handling for compiler.compile() and feedback to user.
compile_context = CompileContext() compiler = Compiler(compile_context) + try: compiler.compile(compile_context) + click.echo("Compilation completed successfully.") + except Exception as e: + raise click.ClickException(f"Compilation failed: {str(e)}")
26-28: Redundant parameter passingcompile_context is passed to both constructor and compile method.
compile_context = CompileContext() compiler = Compiler(compile_context) - compiler.compile(compile_context) + compiler.compile()
35-36: Remove commented out codeUnused main block can be removed.
-# if __name__ == "__main__": -# compile()api/py/test/sample/teams.py (2)
65-67: Commented code should be removedOld namespace parameter still present in comments.
sample_team = Team( - # namespace="test", outputNamespace="test",
65-84: Duplicate team definitionsample_team and test have identical configurations.
Consider removing one of these duplicate Team definitions or documenting why both are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/py/ai/chronon/repo/compile.py(0 hunks)api/py/ai/chronon/repo/zipline.py(2 hunks)api/py/test/sample/teams.py(3 hunks)
💤 Files with no reviewable changes (1)
- api/py/ai/chronon/repo/compile.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
🔇 Additional comments (2)
api/py/test/sample/teams.py (2)
44-45: Parameter changed from namespace to outputNamespaceConsistent with parameter renaming.
86-92: Consistent parameter renaming across all teamsAll Team instances updated to use outputNamespace.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/py/ai/chronon/cli/compile/parse_teams.py (2)
20-37: New dynamic module import function added.Function works correctly but lacks error handling.
def import_module_from_file(file_path): # Get the module name from the file path (without .py extension) module_name = file_path.split("/")[-1].replace(".py", "") # Create the module spec spec = importlib.util.spec_from_file_location(module_name, file_path) + if spec is None: + raise ImportError(f"Could not create module spec for {file_path}") # Create the module based on the spec module = importlib.util.module_from_spec(spec) # Add the module to sys.modules sys.modules[module_name] = module - # Execute the module - spec.loader.exec_module(module) + try: + # Execute the module + spec.loader.exec_module(module) + except Exception as e: + # Clean up sys.modules if execution fails + if module_name in sys.modules: + del sys.modules[module_name] + raise ImportError(f"Error executing module {module_name}: {str(e)}") return module
50-52: Outdated error message.Error message still references PYTHONPATH despite new import method.
require( team_module is not None, - f"Team config file {teams_file} is not on the PYTHONPATH. You might need to add the your config directory to the PYTHONPATH.", + f"Failed to load team config file {teams_file}. Please check that it's a valid Python file.", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (7)
api/py/ai/chronon/cli/compile/compile_context.py(1 hunks)api/py/ai/chronon/cli/compile/compiler.py(0 hunks)api/py/ai/chronon/cli/compile/parse_configs.py(2 hunks)api/py/ai/chronon/cli/compile/parse_teams.py(4 hunks)api/py/ai/chronon/repo/zipline.py(2 hunks)api/py/test/sample/group_bys/sample_team/sample_group_by_missing_input_column.py(1 hunks)api/py/test/sample/teams.py(3 hunks)
💤 Files with no reviewable changes (1)
- api/py/ai/chronon/cli/compile/compiler.py
🚧 Files skipped from review as they are similar to previous changes (1)
- api/py/test/sample/teams.py
🧰 Additional context used
🧬 Code Definitions (1)
api/py/ai/chronon/cli/compile/parse_teams.py (1)
api/py/ai/chronon/cli/logger.py (1) (1)
require(54-57)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
🔇 Additional comments (12)
api/py/test/sample/group_bys/sample_team/sample_group_by_missing_input_column.py (1)
28-28: Good clarification comment.Helpful comment explaining the intentional omission for testing.
api/py/ai/chronon/cli/compile/compile_context.py (1)
6-6: Standardized Model import location.Import now consistently comes from api.ttypes package.
api/py/ai/chronon/repo/zipline.py (3)
1-8: Added necessary imports for new compile command.Required modules for the new functionality.
15-29: Added new compile command with prerequisite checks.Function properly validates environment before compilation.
31-31: Added compile command to zipline group.Command properly registered.
api/py/ai/chronon/cli/compile/parse_configs.py (2)
75-76: Simplified module parsing logic.Improved path handling by only splitting into two parts.
86-88: Updated metadata assignment.Now uses new mod_path variable for consistent naming.
api/py/ai/chronon/cli/compile/parse_teams.py (5)
3-4: Added necessary imports for dynamic module loading.New imports support the new
import_module_from_filefunction.
47-47: Using new import function instead of PYTHONPATH-dependent approach.Good change that makes module loading more flexible.
55-55: Added helpful logging statement.Improves visibility into processing flow.
87-87: Grammar fix in error message.Added period at end of message for consistency.
17-17:❓ Verification inconclusive
Changed default team from "common" to "default".
Verify this change doesn't break existing configurations.
🏁 Script executed:
#!/bin/bash # Check for any references to "common" team in config files rg -l "common" --type pyLength of output: 453
Update Default Team Configuration and Verify Consistency
- The constant in
api/py/ai/chronon/cli/compile/parse_teams.pyhas been updated from"common"to"default".- However, references to
"common"still appear in multiple modules (e.g.,docs/source/conf.py,api/py/ai/chronon/group_by.py,api/py/ai/chronon/join.py, etc.).- Please confirm that the change is intentional and verify that all configuration-dependent logic (including tests) handles the new default correctly.
david-zlai
force-pushed
the
nikhil/compile_switch
branch
from
48bc817 to
6bdba7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/py/ai/chronon/cli/compile/compiler.py (1)
79-84: Fix unused variable per static analysisThis display update logic looks good, but rename unused variable per static analyzer:
- for name, successful_obj in objects.items(): + for name, _successful_obj in objects.items():🧰 Tools
🪛 Ruff (0.8.2)
80-80: Loop control variable
successful_objnot used within loop bodyRename unused
successful_objto_successful_obj(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/py/ai/chronon/cli/compile/compiler.py(1 hunks)api/py/ai/chronon/cli/compile/display/class_tracker.py(1 hunks)api/py/ai/chronon/cli/compile/parse_configs.py(2 hunks)api/py/ai/chronon/repo/teams.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/py/ai/chronon/repo/teams.py
🚧 Files skipped from review as they are similar to previous changes (1)
- api/py/ai/chronon/cli/compile/parse_configs.py
🧰 Additional context used
🪛 Ruff (0.8.2)
api/py/ai/chronon/cli/compile/compiler.py
80-80: Loop control variable successful_obj not used within loop body
Rename unused successful_obj to _successful_obj
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
🔇 Additional comments (1)
api/py/ai/chronon/cli/compile/display/class_tracker.py (1)
64-64: Improved feedback for added objectsGood addition for consistent output with updates.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/py/ai/chronon/utils.py (1)
329-335: Replace old team config retrieval methodChanged from file-based approach to direct team configuration loading.
Considerations:
- Debug prints should be removed before production
- TODO comment is unnecessary since change is already implemented
- print(f"chronon_root_path: {chronon_root_path}") namespace = parse_teams.load_teams(chronon_root_path).get(team_name).outputNamespace - print(f"namespace: {namespace}") - # TODO replace this with teams.py - # namespace = teams.get_team_conf( - # os.path.join(chronon_root_path, TEAMS_FILE_PATH), team_name, "namespace" - # )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
api/py/ai/chronon/utils.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
api/py/ai/chronon/utils.py (1)
api/py/ai/chronon/cli/compile/parse_teams.py (1) (1)
load_teams(38-61)
🔇 Additional comments (1)
api/py/ai/chronon/utils.py (1)
316-318: New function for dynamic team config importThis function provides a cleaner way to fetch team configurations through module imports.
david-zlai
force-pushed
the
nikhil/compile_switch
branch
2 times, most recently
from
f534199 to
000ea50
Compare
david-zlai
force-pushed
the
nikhil/compile_switch
branch
from
000ea50 to
91b240f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/py/ai/chronon/repo/compilev3.py (1)
19-21: Compiler instantiation appears redundant.compile_context is passed twice - once during Compiler initialization and again in compile().
compiler = Compiler(compile_context) -compiler.compile(compile_context) +compiler.compile()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/py/ai/chronon/repo/compilev3.py(1 hunks)api/py/ai/chronon/repo/zipline.py(2 hunks)api/py/tox.ini(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/py/ai/chronon/repo/zipline.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (4)
api/py/ai/chronon/repo/compilev3.py (3)
1-6: Imports look good.Clean imports for the new compiler implementation.
8-17: Good error handling.Properly checks for configuration files before proceeding.
24-25: Standard main block.Allows script to be run both as module and standalone.
api/py/tox.ini (1)
12-15: Updated compilation workflow.Path changed from 'production' to 'compiled' and using new compilev3.py.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/py/ai/chronon/repo/compilev3.py (1)
25-27: Redundant context passing.CompileContext passed twice unnecessarily.
- compiler = Compiler(compile_context) - compiler.compile(compile_context) + compiler = Compiler(compile_context) + compiler.compile()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/py/ai/chronon/repo/compilev3.py(1 hunks)api/py/requirements/base.txt(4 hunks)api/py/tox.ini(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/py/tox.ini
🧰 Additional context used
🪛 LanguageTool
api/py/requirements/base.txt
[duplication] ~29-~29: Possible typo: you repeated a word.
Context: ...irements/base.in face==24.0.0 # via glom glom==24.11.0 # via -r requirements/base...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (3)
api/py/requirements/base.txt (1)
8-13: New dependencies look good.Added dependencies support rich text formatting and data manipulation capabilities.
Also applies to: 28-31, 59-60, 74-75, 82-83
api/py/ai/chronon/repo/compilev3.py (2)
1-6: Command structure looks good.Command definition and imports are clean and clear.
Also applies to: 8-14
30-31: Standalone execution works correctly.Entry point properly calls the compile function.
| face==24.0.0 | ||
| # via glom | ||
| glom==24.11.0 | ||
| # via -r requirements/base. |
There was a problem hiding this comment.
Fix incomplete requirements comment.
The comment reference is incomplete.
- # via -r requirements/base.
+ # via -r requirements/base.in📝 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.
| # via -r requirements/base. | |
| # via -r requirements/base.in |
david-zlai
force-pushed
the
nikhil/compile_switch
branch
from
aa1921c to
4e59371
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/py/ai/chronon/repo/compilev3.py (1)
14-30: Potential redundancy in compiler usage.
compile_contextis passed to both the constructor and the compile method.- compiler = Compiler(compile_context) - compiler.compile(compile_context) + compiler = Compiler(compile_context) + compiler.compile()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/py/ai/chronon/repo/compilev3.py(1 hunks)api/py/ai/chronon/repo/zipline.py(2 hunks)api/py/ai/chronon/utils.py(3 hunks)api/py/tox.ini(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/py/ai/chronon/repo/zipline.py
🧰 Additional context used
🧬 Code Definitions (1)
api/py/ai/chronon/utils.py (1)
api/py/ai/chronon/cli/compile/parse_teams.py (1) (1)
load_teams(38-61)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non_spark_tests
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (7)
api/py/ai/chronon/utils.py (3)
28-28: Updated import to use new compiler moduleImport changed to use
parse_teamsfrom the compile module, replacing the oldteamsmodule approach.Also applies to: 35-36
317-320: New utility function for team configurationNew function dynamically imports team modules, enhancing flexibility for team configuration access.
331-337:✅ Verification successful
Namespace retrieval implementation changed
Updated to use
parse_teams.load_teams()instead of the previous file-based approach.Note the TODO comment indicates this may be a transitional change.
🏁 Script executed:
#!/bin/bash # Verify consistent behavior between old and new team configuration approach grep -r "get_team_conf" --include="*.py" .Length of output: 1035
Namespace retrieval update verified
- In
api/py/ai/chronon/utils.py(lines 331-337), the implementation correctly usesparse_teams.load_teams()as intended.- The commented-out old approach via
teams.get_team_conf()remains only as a transitional note.- Other parts of the codebase still use the old interface, which aligns with the current incremental migration.
api/py/ai/chronon/repo/compilev3.py (3)
1-6: Import organization looks good.Clean imports with logical grouping.
19-26: Error handling for config files looks good.Clear error message when configuration files are missing.
33-34:❓ Verification inconclusive
CLI invocation lacks chronon_root parameter.
When running directly, no path is provided to compile_v3().
🏁 Script executed:
#!/bin/bash # Check if compile works without parameters python -c "from ai.chronon.cli.compile.compiler import Compiler; from ai.chronon.cli.compile.compile_context import CompileContext; print('Compiler expects parameters:', Compiler.compile.__code__.co_varnames)"Length of output: 333
CLI Parameter Verification Needed
The initial test failed due to PYTHONPATH issues, so we need to re-run our checks with the correct environment. Please execute the following scripts to verify whether the CLI invocation of
compile_v3()is missing the requiredchronon_rootparameter and to inspect its definition:
Re-run with PYTHONPATH set:
Locate the compile_v3 definition:
Once these outputs are available, we can conclusively determine if the CLI invocation should pass a
chronon_rootparameter.
🏁 Script executed:
#!/bin/bash # Re-run ensuring PYTHONPATH is set. PYTHONPATH=. python -c "from ai.chronon.cli.compile.compiler import Compiler; print('Compiler params:', Compiler.compile.__code__.co_varnames)"Length of output: 391
🏁 Script executed:
#!/bin/bash # Search for compile_v3 definition. rg "def compile_v3(" -n .Length of output: 230
CLI Invocation Parameter Issue – Manual Verification Needed
Our automated tests couldn’t load the module (ModuleNotFoundError) or locate the definition for
compile_v3(). Please manually verify that the CLI invocation ofcompile_v3()includes the requiredchronon_rootparameter and that the PYTHONPATH is correctly configured.
- Confirm that the definition of
compile_v3expects a parameter for the chronon_root.- Ensure the CLI command passes the chronon_root path when run directly.
api/py/tox.ini (1)
11-15: Updated commands reflect new compilation approach.Directory structure changed from 'production' to 'compiled', and now using the new compile command.
|
Closing this PR in favor of #541 |
Summary
cutting over to the new compiler
Checklist
Summary by CodeRabbit
New Features
compilecommand in the CLI for processing configuration files.Refactor
Chores
tox.inifile to reflect changes in the compilation process.