Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where custom extraction cost models (and related APIs like lookup_function_value) could fail when operating on subexpressions represented as already-interned egglog values.
Changes:
- Fix
typed_expr_to_valueto directly return the underlyingbindings.ValueforValueDeclinstead of attempting to translate/evaluate it. - Add a regression test covering cost-model lookup of function values on subtree values.
- Document the fix in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
python/tests/test_high_level.py |
Adds regression test exercising lookup_function_value inside a custom cost model when subtree args are ValueDecls. |
python/egglog/egraph_state.py |
Fixes conversion of TypedExprDecl to bindings.Value by special-casing ValueDecl. |
docs/changelog.md |
Adds an UNRELEASED changelog entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThese changes enhance the cost model evaluation system with a performance optimization and test coverage. The egraph_state module now short-circuits ValueDecl expression evaluation, a changelog entry documents a cost model lookup fix, and a new test validates custom cost model functionality with expression subtree inspection. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/egglog/egraph_state.py`:
- Around line 686-687: The isinstance check uses ValueDecl but ValueDecl is only
coming from a star import, causing Ruff F405; update the module imports to
explicitly import ValueDecl (add ValueDecl to the from ... import (...) list
where other AST/decl types are imported) so the isinstance(typed_expr.expr,
ValueDecl) line in egraph_state (around the check on typed_expr.expr) resolves
without a star import warning.
In `@python/tests/test_high_level.py`:
- Around line 1597-1600: The conv_cost function must guard against
get_callable_args returning None and avoid the unused-argument warning for
child_costs: update conv_cost (handling KAT) to call get_callable_args(expr),
treat a None result as an empty sequence before iterating (e.g., if args is
None: args = ()), and explicitly mark child_costs as intentionally unused (for
example by assigning it to _ or using del child_costs) so linters stop warning;
keep the existing sum over cast("KAT", a).size() for each arg.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/changelog.mdpython/egglog/egraph_state.pypython/tests/test_high_level.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Agent
- GitHub Check: benchmark (ubuntu-latest)
- GitHub Check: benchmark (codspeed-macro)
- GitHub Check: docs
- GitHub Check: build linux (aarch64)
- GitHub Check: build linux (ppc64)
- GitHub Check: build linux
- GitHub Check: build macos
- GitHub Check: build windows
🧰 Additional context used
🧬 Code graph analysis (2)
python/tests/test_high_level.py (4)
python/egglog/bindings.pyi (1)
size(744-744)src/termdag.rs (1)
size(18-20)python/egglog/examples/jointree.py (1)
size(24-24)python/egglog/deconstruct.py (3)
get_callable_args(117-117)get_callable_args(121-121)get_callable_args(124-153)
python/egglog/egraph_state.py (1)
python/egglog/declarations.py (1)
ValueDecl(727-728)
🪛 Ruff (0.15.2)
python/tests/test_high_level.py
[error] 1578-1578: Expr may be undefined, or defined from star imports
(F405)
[error] 1587-1587: i64 may be undefined, or defined from star imports
(F405)
[error] 1591-1591: EGraph may be undefined, or defined from star imports
(F405)
[error] 1594-1594: set_ may be undefined, or defined from star imports
(F405)
[error] 1595-1595: set_ may be undefined, or defined from star imports
(F405)
[warning] 1597-1597: Unused function argument: child_costs
(ARG001)
[error] 1599-1599: get_callable_args may be undefined, or defined from star imports
(F405)
python/egglog/egraph_state.py
[error] 686-686: ValueDecl may be undefined, or defined from star imports
(F405)
🔇 Additional comments (1)
docs/changelog.md (1)
16-16: Changelog entry is clear and appropriately scoped.The new UNRELEASED note accurately captures the behavior fix and links to discussion context.
| if isinstance(typed_expr.expr, ValueDecl): | ||
| return typed_expr.expr.value |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ruff check python/egglog/egraph_state.py --select F405
rg -n "from \.declarations import" python/egglog/egraph_state.pyRepository: egraphs-good/egglog-python
Length of output: 50383
🏁 Script executed:
#!/bin/bash
# Check lines 686-687 and surrounding context
sed -n '680,695p' python/egglog/egraph_state.py | cat -n
# Check the declarations import specifically
rg -n "^from \.declarations import|^from \.declarations import.*ValueDecl|^from \*" python/egglog/egraph_state.py | head -20Repository: egraphs-good/egglog-python
Length of output: 888
🏁 Script executed:
#!/bin/bash
# Check exact import line and context
sed -n '15,20p' python/egglog/egraph_state.py | cat -n
# Run Ruff check focused on the region around lines 686-687
ruff check python/egglog/egraph_state.py --select F405 2>&1 | grep -A2 -B2 "686\|687"Repository: egraphs-good/egglog-python
Length of output: 1176
Add explicit import of ValueDecl to fix Ruff F405.
The new isinstance(typed_expr.expr, ValueDecl) check at line 686 triggers Ruff F405 since ValueDecl is only available through the star import. Add it to the explicit imports to keep the codebase lint-clean.
Suggested patch
-from .declarations import ConstructorDecl
+from .declarations import ConstructorDecl, ValueDecl🧰 Tools
🪛 Ruff (0.15.2)
[error] 686-686: ValueDecl may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/egglog/egraph_state.py` around lines 686 - 687, The isinstance check
uses ValueDecl but ValueDecl is only coming from a star import, causing Ruff
F405; update the module imports to explicitly import ValueDecl (add ValueDecl to
the from ... import (...) list where other AST/decl types are imported) so the
isinstance(typed_expr.expr, ValueDecl) line in egraph_state (around the check on
typed_expr.expr) resolves without a star import warning.
| def conv_cost(eg, expr, child_costs): | ||
| if isinstance(expr, KAT): | ||
| args = get_callable_args(expr) | ||
| return sum(int(eg.lookup_function_value(cast("KAT", a).size())) for a in args) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for linting issues and find get_callable_args definition
ruff check python/tests/test_high_level.py --select ARG001,F405
echo "---"
rg -n "def get_callable_args|from \.deconstruct import get_callable_args|__all__" python/egglog/deconstruct.py python/egglog/__init__.pyRepository: egraphs-good/egglog-python
Length of output: 50383
🏁 Script executed:
#!/bin/bash
# Read the full function and surrounding context
sed -n '1590,1610p' python/tests/test_high_level.pyRepository: egraphs-good/egglog-python
Length of output: 542
🏁 Script executed:
#!/bin/bash
# Find the definition of get_callable_args to check return type
rg -A 10 "^def get_callable_args" python/egglog/Repository: egraphs-good/egglog-python
Length of output: 1276
Harden conv_cost against None args and silence unused-arg warning.
get_callable_args can return None; iterating that would raise TypeError. Also, child_costs is intentionally unused here.
Suggested patch
-def conv_cost(eg, expr, child_costs):
+def conv_cost(eg, expr, _child_costs):
if isinstance(expr, KAT):
- args = get_callable_args(expr)
+ args = get_callable_args(expr) or ()
return sum(int(eg.lookup_function_value(cast("KAT", a).size())) for a in args)🧰 Tools
🪛 Ruff (0.15.2)
[warning] 1597-1597: Unused function argument: child_costs
(ARG001)
[error] 1599-1599: get_callable_args may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/tests/test_high_level.py` around lines 1597 - 1600, The conv_cost
function must guard against get_callable_args returning None and avoid the
unused-argument warning for child_costs: update conv_cost (handling KAT) to call
get_callable_args(expr), treat a None result as an empty sequence before
iterating (e.g., if args is None: args = ()), and explicitly mark child_costs as
intentionally unused (for example by assigning it to _ or using del child_costs)
so linters stop warning; keep the existing sum over cast("KAT", a).size() for
each arg.
No description provided.