Open
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request introduces substantial refactoring and feature enhancements across the egglog-python codebase. Primary changes include migrating egglog dependencies from the Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches
🧪 Generate unit tests (beta)
|
saulshanabrook
force-pushed
the
polynomials
branch
from
5a3d3c2 to
6b3aabc
Compare
saulshanabrook
force-pushed
the
polynomials
branch
from
4aa6590 to
afe9c82
Compare
saulshanabrook
force-pushed
the
polynomials
branch
from
9f2fe6d to
83e73c3
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
saulshanabrook
force-pushed
the
polynomials
branch
from
bf94c2e to
f5d993b
Compare
saulshanabrook
force-pushed
the
polynomials
branch
from
d27f2cd to
b0daea4
Compare
saulshanabrook
commented
Mar 16, 2026
saulshanabrook
force-pushed
the
polynomials
branch
from
bc77f89 to
d1a9f04
Compare
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a polynomial factoring example (via multiset-based normalization) and expands the egglog Python/Rust bindings to support new debugging (“freeze”) and updated container/array-api/program-generation workflows.
Changes:
- Add
EGraph.freeze()and supporting Rust/Python plumbing to snapshot an e-graph into replayable high-level actions. - Introduce a polynomial container example + tests/docs demonstrating factoring without AC blowup.
- Refactor/extend runtime/type-inference, container helpers, and array-api program generation; update tests/snapshots accordingly.
Reviewed changes
Copilot reviewed 53 out of 57 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Add sympy (and mpmath) to dev dependencies |
| src/termdag.rs | Switch TermDag wrapper APIs to use TermId instead of Term |
| src/serialize.rs | Fix spelling in doc comment (“separate”) |
| src/py_object_sort.rs | Update to new TermId-based TermDag APIs |
| src/lib.rs | Export new freeze pyclasses into module bindings |
| src/freeze.rs | Add Rust-side frozen e-graph structures (FrozenEGraph, etc.) |
| src/extract.rs | Update extractor return types to TermId |
| src/egraph.rs | Record command strings earlier; add freeze; adjust Value pyclass flags |
| src/conversions.rs | Update bindings conversions for newer egglog AST/outputs + prove exists output |
| python/tests/test_unstable_fn.py | Add regression test for distinct lambda naming |
| python/tests/test_type_constraint_solver.py | Update tests for new TypeConstraintSolver API |
| python/tests/test_runtime.py | Add doctest/runtime stability tests; adjust typevar usage |
| python/tests/test_program_gen.py | Fix typo eval_program_ruleset reference |
| python/tests/test_pretty.py | Add freeze pretty-print tests; adjust long-line wrapping expectations |
| python/tests/test_high_level.py | Add tests for let-name prefixing, constructor registration, scheduler updates, etc. |
| python/tests/test_array_api.py | Broad refactors for tuple/value constructors, reshape/lda/jit behavior, add polynomial factoring test |
| python/tests/snapshots/test_array_api/test_program_compile[tuple][expr].py | Update snapshot for new tuple/value constructors |
| python/tests/snapshots/test_array_api/test_jit[tuple][initial_expr].py | Update snapshot for new tuple constructor form |
| python/tests/snapshots/test_array_api/test_jit[tuple][expr].py | Update snapshot for new program-gen output structure |
| python/tests/snapshots/test_array_api/test_jit[tuple][code].py | Remove outdated generated code snapshot |
| python/tests/snapshots/test_array_api/test_jit[lda][initial_expr].py | Update snapshot for numerous array-api lowering changes |
| python/tests/snapshots/test_array_api/test_jit[lda][expr].py | Update snapshot for numerous array-api lowering changes |
| python/tests/snapshots/test_array_api/test_jit[lda][code].py | Update snapshot for new generated numpy code |
| python/tests/snapshots/test_array_api/TestLoopNest.test_index_codegen[expr].py | Update snapshot formatting / tuple constructor changes |
| python/egglog/visualizer_widget.py | Change non-notebook visualization to write standalone HTML |
| python/egglog/type_constraint_solver.py | Redesign solver around TypeVarRef; add callable probing path for UnstableFn |
| python/egglog/thunk.py | Preserve exception chaining/traceback when thunk evaluation fails |
| python/egglog/runtime.py | Runtime typevar ref updates; add attribute caching; improve partial application + type inference plumbing |
| python/egglog/pretty.py | Improve formatting pipeline (AST normalize + black); add EGraphDecl/DummyDecl support |
| python/egglog/exp/program_gen.py | Fix typos and rename is_identifier / eval_program_ruleset |
| python/egglog/exp/polynomials.py | New end-to-end polynomial/multiset example utilities + reports |
| python/egglog/exp/array_api_program_gen.py | Refactor program generation rules; add recursive value printing; update optionals handling |
| python/egglog/exp/array_api_numba.py | Update numba rewrite rules for updated array-api signatures |
| python/egglog/exp/array_api_loopnest.py | Update loopnest rules for new tuple/vec encodings |
| python/egglog/exp/array_api_jit.py | Rewrite JIT to use EGraph.run/extract and improve error notes |
| python/egglog/egraph_state.py | Deterministic naming counters; $-prefix normalization; TermId extraction changes; unnamed function naming |
| python/egglog/egraph.py | Variadic constructor for initial actions; add high-level freeze(); schedule decl thunk refactors |
| python/egglog/deconstruct.py | Add constant name helper; adjust bound type param extraction |
| python/egglog/declarations.py | Introduce EGraphDecl, DummyDecl, TypeVarRef; adjust declaration plumbing and typevar utilities |
| python/egglog/conversion.py | Rework literal resolution (dummy sentinel support); rename debug helper |
| python/egglog/builtins.py | Add/extend multiset/vec/numeric/string builtins; move PyObject definition; update conversions |
| python/egglog/bindings.pyi | Update stubs for TermId-based extraction + freeze + prove commands |
| python/egglog/init.py | Re-export ActionLike for typing |
| pyproject.toml | Add sympy dev dep; adjust ruff ignores |
| docs/reference/python-integration.md | Expand debugging/inspection docs (run/stats/function_values/freeze/display/saturate) |
| docs/reference/egglog-translation.md | Document passing actions to EGraph(...); note proof-mode limitations |
| docs/how-to-guides.md | Clarify parsing/running program strings section |
| docs/explanation/2026_02_containers.md | Add large containers/polynomials writeup and reproducible examples |
| docs/conf.py | Remove INP001 noqa after config update |
| docs/changelog.md | Add unreleased entry summarizing new features and fixes |
| Cargo.toml | Update egglog git deps; add indexmap, tempfile; enable pyo3 indexmap feature |
| Cargo.lock | Update dependency graph for new egglog branch + added crates |
| AGENTS.md | Add repo guidance document for contributors/agents |
| .github/workflows/CI.yml | Narrow CodSpeed snapshot update run to targeted array-api tests |
| .github/AGENTS.md | Remove older agent instructions (replaced by top-level AGENTS.md) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/egglog/runtime.py (1)
301-312:⚠️ Potential issue | 🟠 MajorKeep the bound receiver in partial method calls.
For bound instance methods,
RuntimeFunction.__call__prependsself.__egg_bound__before buildingcall.args, but Line 309 still slices withlen(partial_args)only.UnstableFn(obj.method, x)therefore turns intoPartialCallDecl(obj)instead ofPartialCallDecl(obj, x)and exposesxagain as a remaining parameter.Suggested fix
call = (res_typed_expr := res.__egg_typed_expr__).expr return_tp = res_typed_expr.tp assert isinstance(call, CallDecl), "partial function must be a call" # Clip off the remaining arguments - n_args = len(partial_args) - value = PartialCallDecl(replace(call, args=call.args[:n_args])) - remaining_arg_types = [a.tp for a in call.args[n_args:]] + bound_arg_count = int( + isinstance(fn_arg, RuntimeFunction) and isinstance(fn_arg.__egg_bound__, RuntimeExpr) + ) + kept_arg_count = bound_arg_count + len(partial_args) + value = PartialCallDecl(replace(call, args=call.args[:kept_arg_count])) + remaining_arg_types = [a.tp for a in call.args[kept_arg_count:]] type_ref = JustTypeRef(Ident.builtin("UnstableFn"), (return_tp, *remaining_arg_types)) return RuntimeExpr.__from_values__(Declarations.create(self, res), TypedExprDecl(type_ref, value))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/egglog/runtime.py` around lines 301 - 312, RuntimeFunction.__call__ is failing to account for a bound receiver when creating PartialCallDecl: it prepends self.__egg_bound__ to call.args but still uses n_args = len(partial_args) to slice the original call.args, causing the bound self to be dropped and a real partial argument to be misclassified as remaining; fix by computing n_args as len(partial_args) + (1 if getattr(self, "__egg_bound__", None) is not None else 0) (or otherwise detecting whether a bound receiver was prepended) and use that adjusted n_args when slicing call.args and when building remaining_arg_types and PartialCallDecl so the bound receiver remains included in the partial call.
🧹 Nitpick comments (9)
AGENTS.md (2)
39-39: Consider whether this specific skill name might become stale.The reference to
$github-actions-rest-logsis environment-specific and could change over time. Per the earlier review comment about removing content that "might change too quickly," consider either:
- Making this more generic (e.g., "use available GitHub Actions debugging tools")
- Adding a note that the skill name may vary by environment
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 39, The line referencing the environment-specific skill `$github-actions-rest-logs` may become stale; update the sentence so it uses a generic phrase (e.g., "use available GitHub Actions debugging tools or the REST API with GITHUB_PAT_TOKEN") or append a short parenthetical note that the exact skill name may vary by environment. Locate the literal `$github-actions-rest-logs` in AGENTS.md and replace it with the chosen generic wording or add the clarification immediately after that token to prevent future breakage.
43-45: Minor style: Vary sentence structure in the verification list.Three consecutive sentences start with "Run," which reduces readability slightly. Consider rewording for variety, e.g., "After typing changes, run
make mypy" or using a bulleted list format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 43 - 45, In AGENTS.md, the three verification lines all start with "Run" which is repetitive; reword them to vary sentence structure and improve readability by making one or two sentences lead with the context instead (for example: "After typing changes, run `make mypy`", "For touched modules, run targeted `pytest`", "When docs or public API change, run `make docs`"), or convert them into a bulleted list—update the lines mentioning `make mypy`, `pytest`, and `make docs` accordingly.pyproject.toml (1)
206-209: KeepINP001scoped todocs/**.The comment says this exception is only needed for docs, but adding it to the global ignore list disables Ruff's namespace-package check across the whole repo. Move
INP001into the existingdocs/**per-file ignore instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 206 - 209, Remove "INP001" from the global ignore list in pyproject.toml and instead add it to the existing per-file-ignore entry for "docs/**" so the INP001 exception only applies to documentation files; locate the global ignore array that currently contains "INP001" and the per-file-ignore mapping for "docs/**" and move the "INP001" token there (leave other ignores like "PLW0108" untouched).python/tests/test_array_api.py (1)
458-479: New polynomial factoring test validates key PR functionality.The
test_polynomial_factoringtest covers the polynomial factoring feature mentioned in the PR objectives. The test approach of normalizing both extracted and expected expressions before comparison is sound.Note: The
inputparameter on line 469 shadows the Python builtin. Consider renaming toinput_expror similar to avoid potential confusion.♻️ Optional: Rename parameter to avoid shadowing builtin
`@pytest.mark.parametrize`( - ("input", "expected"), + ("input_expr", "expected"), [ pytest.param(x * x, x**2, id="exp"), ... ], ) -def test_polynomial_factoring(input: Value, expected: Value): +def test_polynomial_factoring(input_expr: Value, expected: Value): egraph = EGraph() - x = egraph.let("x", input) + x = egraph.let("x", input_expr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/tests/test_array_api.py` around lines 458 - 479, Rename the test parameter that shadows the builtin: change the parametrize tuple and the test function signature from ("input", "expected") / def test_polynomial_factoring(input: Value, expected: Value) to ("input_expr", "expected") / def test_polynomial_factoring(input_expr: Value, expected: Value), and update all intra-function references (e.g., egraph.let("x", input) -> egraph.let("x", input_expr)) so the parameter name matches everywhere (keeping the pytest.param entries intact except for the renamed parameter).python/egglog/exp/array_api_program_gen.py (1)
560-579: New recursive value program generation support.The
recursive_value_programandvec_recursive_value_programfunctions enable code generation for nested/recursive value structures, supporting the newNDArray(rv)literal syntax on line 428.Note: The
v: Valueparameter in_vec_recursive_value_program(line 575) appears unused. If it's required for the ruleset registration pattern, consider adding a comment; otherwise, it could be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/egglog/exp/array_api_program_gen.py` around lines 560 - 579, The parameter v in the ruleset function _vec_recursive_value_program is unused; either remove it from the signature or mark it explicitly as unused (e.g., rename to _v) or add a comment explaining it's required by array_api_program_gen_ruleset.register's signature pattern; update the function signature for _vec_recursive_value_program (and any registration call) accordingly and add a brief inline comment referencing vec_recursive_value_program and recursive_value_program so future readers know why the parameter was kept if you choose not to remove it.python/egglog/egraph_state.py (1)
482-496: Complex conditional logic for UnstableFn type arguments.The conditional handling for
UnstableFnwithlen(ref.args) > 1vs the fallbackUnit()literal is intricate. The logic constructs type arguments differently when there are more than one argument versus zero/one.Consider adding a brief inline comment explaining the expected structure of
UnstableFntype arguments (return type position, arg types position) to aid future maintainability.🤖 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 482 - 496, Add an inline comment above the UnstableFn branch (the if ref.ident == Ident.builtin("UnstableFn") block) explaining the expected structure of ref.args and how type_args are constructed: document that ref.args[0] is the return type, ref.args[1:] are parameter types (and when len(ref.args) <= 1 we use a Unit() literal for the param-list), and that the code builds a bindings.Call with the return type as the head and parameter types as Var children (referencing bindings.Call, bindings.Lit, and bindings.Var in this block); keep it short and local to the type_args construction to improve future maintainability.python/egglog/declarations.py (1)
388-519: Substantialto_actionsimplementation for EGraph serialization.The
to_actionscached property converts anEGraphDeclto a list of actions for reconstructing the egraph. The implementation:
- Iteratively finds grounded terms for e-classes
- Converts all expressions to grounded form
- Emits unions, let bindings, sets, and expr actions
The algorithm handles the grounding fixpoint correctly. Note the comment on line 512 has a typo: "remaining call s" (extra spaces).
📝 Fix typo in comment
- # Now add any remaining call s that weren't part of any other actions + # Now add any remaining calls that weren't part of any other actions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/egglog/declarations.py` around lines 388 - 519, Fix the typo in the comment above the final actions.extend block in the to_actions cached_property: change the comment text "Now add any remaining call s that weren't part of any other actions" to "Now add any remaining calls that weren't part of any other actions" so it reads correctly; the change should be made near the end of the to_actions method just before the actions.extend(...) that handles single_e_class_calls and emitted_call_decls.python/egglog/conversion.py (1)
230-233: Move DUMMY_VALUE guard beforeresolve_typefor clearer control flow.At Line 231,
arg_typeis computed even though it’s unused whenarg is DUMMY_VALUE(Line 232). Reordering makes this path cheaper and easier to read.Proposed cleanup
- tp_just = tp.to_just() - arg_type = resolve_type(arg) - if arg is DUMMY_VALUE: + tp_just = tp.to_just() + if arg is DUMMY_VALUE: return RuntimeExpr.__from_values__(decls(), TypedExprDecl(tp_just, DummyDecl())) + arg_type = resolve_type(arg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/egglog/conversion.py` around lines 230 - 233, Move the DUMMY_VALUE early-return guard to before the call to resolve_type to avoid computing arg_type when it's unused: check "if arg is DUMMY_VALUE" first and return the RuntimeExpr.__from_values__(decls(), TypedExprDecl(tp.to_just(), DummyDecl())) there, then compute arg_type = resolve_type(arg) afterwards; reference symbols: DUMMY_VALUE, resolve_type, tp.to_just()/tp_just, TypedExprDecl, RuntimeExpr.__from_values__, decls(), DummyDecl().python/egglog/deconstruct.py (1)
191-193: Remove unreachableInitRefbranch fromegg_boundguard.Because Line 185–189 already returns early for
InitRef, includingInitRefagain in Line 192 is dead logic and can be simplified.Suggested simplification
- JustTypeRef(call.callable.ident, call.bound_tp_params) - if isinstance(call.callable, (ClassMethodRef, MethodRef, InitRef)) and call.bound_tp_params + JustTypeRef(call.callable.ident, call.bound_tp_params) + if isinstance(call.callable, (ClassMethodRef, MethodRef)) and call.bound_tp_params else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/egglog/deconstruct.py` around lines 191 - 193, The isinstance check in the egg_bound construction includes InitRef redundantly; since InitRef is already returned early in the prior branch, remove InitRef from the tuple in the conditional expression so it reads isinstance(call.callable, (ClassMethodRef, MethodRef)) (leaving the rest of the expression unchanged), ensuring JustTypeRef and the None branch behavior remain the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 39: Remove the unused Rust crate dependency declaration for tempfile from
Cargo.toml by deleting the line `tempfile = "3.24.0"` in the [dependencies]
section; then run `cargo check` (or `cargo build`) to ensure no Rust code
references remain and update Cargo.lock if necessary.
- Around line 22-26: Update the git dependencies for the egg-smol fork to pin to
the fixed commit instead of a moving branch: replace branch = "multiset-changes"
with rev = "66b277805587423e92a96b05fc4b2643f2cf4aec" for the egglog,
egglog-ast, egglog-core-relations, egglog-reports, and egglog-bridge entries in
Cargo.toml, and make the identical change in the
[patch.'https://github.com/egraphs-good/egglog'] block so the fork is locked to
the exact commit shown in Cargo.lock.
In `@docs/conf.py`:
- Line 1: The import line for pathlib lost its lint suppression for INP001;
either restore the inline suppression by adding back "# noqa: INP001" on the
"import pathlib" line, or update the project's ruff per-file-ignores by adding
"INP001" to the "docs/**" entry in [tool.ruff.lint.per-file-ignores] so that the
implicit namespace package rule is ignored for docs; pick one approach and apply
it consistently to avoid INP001 lint failures.
In `@docs/explanation/2026_02_containers.md`:
- Line 438: Fix the typo in the sentence "We now have an expression that is
mainly a sum of products, a multivariate polyonimal." by replacing the
misspelled word "polyonimal" with the correct word "polynomial" so the line
reads "We now have an expression that is mainly a sum of products, a
multivariate polynomial."
In `@pyproject.toml`:
- Around line 283-286: Add sympy to the project's optional dev dependencies so
pip installs include it: update the pyproject.toml by adding "sympy>=1.14.0"
under the [project.optional-dependencies].dev table to mirror the existing entry
in [dependency-groups].dev; ensure the version string matches "sympy>=1.14.0"
exactly so pip install .[dev] and .[test] will provide SymPy as expected.
In `@python/egglog/builtins.py`:
- Around line 1209-1233: The py_eval and py_exec stubs declare parameters named
globals and locals which shadow Python builtins; rename these parameters to
globals_ and locals_ in the py_eval and py_exec function signatures and update
their default expressions and docstrings to use globals_/locals_ (and update any
internal references or callers in this module to the new names); if API
compatibility must be preserved, add backwards-compatible aliases or wrapper
stubs that accept globals and locals and forward them to the new signatures;
ensure you update any references in related functions (e.g., documentation or
tests) so the code compiles and behavior is unchanged.
In `@python/egglog/egraph.py`:
- Around line 1402-1407: The current code assumes a single callable ref when
reading self._state.egg_fn_to_callable_refs[name] and when scanning
self._state.cost_callables, which raises ValueError if multiple callables share
the same egg name; change both sites to select the correct callable_ref by
matching the function signature (arity and sorts) instead of unpacking a
singleton: iterate candidates from self._state.egg_fn_to_callable_refs[name] or
the generator over cost_callables and pick the ref whose callable metadata
matches fn.input_sorts and fn.output_sort (or equivalent stored sorts), then use
that matched callable_ref; update the logic in the functions related to freeze()
and _values_to_expr() so they handle multiple refs safely by signature-matching
rather than tuple-unpacking.
In `@python/egglog/exp/array_api_jit.py`:
- Around line 37-41: The exception handler for bindings.EggSmolError currently
calls egraph.extract(fn_program) which may itself raise another EggSmolError;
instead perform the extraction for the diagnostic note in a best-effort way: try
to call egraph.extract(fn_program) inside its own small try/except and capture a
safe string (e.g., the extracted value or a fallback like "<unavailable>") and
then call e.add_note(...) with that safe value; do not let a failing extract
replace the original exception. Reference the symbols EggSmolError, e.add_note,
egraph.extract, and fn_program.as_py_object when updating the handler.
In `@python/egglog/exp/polynomials.py`:
- Around line 197-202: The factoring loop currently compares only the
single-iteration res.run_sec to max_factoring_sec and can both exceed the total
phase cap and fail to append a slow-but-updated result; introduce a cumulative
timer (e.g., cumulative_run_sec = 0.0) before the loop, after each run_example
call add res.run_sec to cumulative_run_sec, append res to factored_reports when
res.updated is true (even if this iteration pushed cumulative_run_sec over the
cap), and break the loop when cumulative_run_sec > max_factoring_sec or when
res.updated is false; adjust checks around
run_example/factoring/distributed_report.extracted/egraph and keep printing the
iteration and res.cost as before.
- Around line 123-148: Both combined_factored and combined_polynomial are only
taking a single register_sec and extract_sec from one element, undercounting
total time; update combined_factored (method combined_factored) to sum
register_sec across all entries in self.factored (e.g., sum(r.register_sec for r
in self.factored)) and sum extract_sec across all entries (e.g.,
sum(r.extract_sec for r in self.factored)), and update combined_polynomial
(property combined_polynomial) to sum register_sec across the contributing
reports (self.polynomial_multisets, self.polynomial_multisets_factored,
self.polynomial) and likewise sum extract_sec across those reports instead of
using only polynomial_multisets.register_sec and polynomial.extract_sec so
Report.total_sec reflects the full aggregated register/extract time.
In `@python/egglog/visualizer_widget.py`:
- Around line 35-52: The code writes raw str(self.egraphs) into HTML/JS and uses
a fixed tmp.html which can lead to XSS and clobbering; change VisualizerWidget
to JSON-serialize self.egraphs (use json.dumps) and embed that JSON string into
the HTML template instead of str(self.egraphs), and write to a uniquely named
temp file (use tempfile.NamedTemporaryFile or tempfile.mkstemp) rather than a
fixed "tmp.html"; update the place where HTML (the HTML constant / MAGIC_STRING
replacement) is created and where file.write_text and webbrowser.open are called
so the file contains safe JSON and is unique per invocation.
In `@src/egraph.rs`:
- Around line 69-73: The code appends cmds_str into self.cmds before executing
the batch, causing unexecuted commands to be recorded if egraph.run_program
(invoked via py.detach(|| self.egraph.run_program(commands))) fails; change the
flow so you call py.detach(...) and only on successful completion append
cmds_str to self.cmds (i.e., move the cmds.push_str(&cmds_str) into the success
path after egraph.run_program returns without error), keeping the info!("Running
commands:\n{}", cmds_str) log where appropriate and referencing self.cmds,
cmds_str, and the py.detach(|| self.egraph.run_program(commands)) call to locate
the change.
In `@src/freeze.rs`:
- Around line 33-35: Update the doc comment for the function from_egraph to
accurately describe its behavior: it constructs a FrozenEGraph from a live
EGraph (taking an &EGraph and producing a FrozenEGraph), rather than converting
a frozen e-graph into reconstruction commands; reference the function name
from_egraph and the types EGraph and FrozenEGraph in the comment so the purpose
is clear.
---
Outside diff comments:
In `@python/egglog/runtime.py`:
- Around line 301-312: RuntimeFunction.__call__ is failing to account for a
bound receiver when creating PartialCallDecl: it prepends self.__egg_bound__ to
call.args but still uses n_args = len(partial_args) to slice the original
call.args, causing the bound self to be dropped and a real partial argument to
be misclassified as remaining; fix by computing n_args as len(partial_args) + (1
if getattr(self, "__egg_bound__", None) is not None else 0) (or otherwise
detecting whether a bound receiver was prepended) and use that adjusted n_args
when slicing call.args and when building remaining_arg_types and PartialCallDecl
so the bound receiver remains included in the partial call.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 39: The line referencing the environment-specific skill
`$github-actions-rest-logs` may become stale; update the sentence so it uses a
generic phrase (e.g., "use available GitHub Actions debugging tools or the REST
API with GITHUB_PAT_TOKEN") or append a short parenthetical note that the exact
skill name may vary by environment. Locate the literal
`$github-actions-rest-logs` in AGENTS.md and replace it with the chosen generic
wording or add the clarification immediately after that token to prevent future
breakage.
- Around line 43-45: In AGENTS.md, the three verification lines all start with
"Run" which is repetitive; reword them to vary sentence structure and improve
readability by making one or two sentences lead with the context instead (for
example: "After typing changes, run `make mypy`", "For touched modules, run
targeted `pytest`", "When docs or public API change, run `make docs`"), or
convert them into a bulleted list—update the lines mentioning `make mypy`,
`pytest`, and `make docs` accordingly.
In `@pyproject.toml`:
- Around line 206-209: Remove "INP001" from the global ignore list in
pyproject.toml and instead add it to the existing per-file-ignore entry for
"docs/**" so the INP001 exception only applies to documentation files; locate
the global ignore array that currently contains "INP001" and the per-file-ignore
mapping for "docs/**" and move the "INP001" token there (leave other ignores
like "PLW0108" untouched).
In `@python/egglog/conversion.py`:
- Around line 230-233: Move the DUMMY_VALUE early-return guard to before the
call to resolve_type to avoid computing arg_type when it's unused: check "if arg
is DUMMY_VALUE" first and return the RuntimeExpr.__from_values__(decls(),
TypedExprDecl(tp.to_just(), DummyDecl())) there, then compute arg_type =
resolve_type(arg) afterwards; reference symbols: DUMMY_VALUE, resolve_type,
tp.to_just()/tp_just, TypedExprDecl, RuntimeExpr.__from_values__, decls(),
DummyDecl().
In `@python/egglog/declarations.py`:
- Around line 388-519: Fix the typo in the comment above the final
actions.extend block in the to_actions cached_property: change the comment text
"Now add any remaining call s that weren't part of any other actions" to "Now
add any remaining calls that weren't part of any other actions" so it reads
correctly; the change should be made near the end of the to_actions method just
before the actions.extend(...) that handles single_e_class_calls and
emitted_call_decls.
In `@python/egglog/deconstruct.py`:
- Around line 191-193: The isinstance check in the egg_bound construction
includes InitRef redundantly; since InitRef is already returned early in the
prior branch, remove InitRef from the tuple in the conditional expression so it
reads isinstance(call.callable, (ClassMethodRef, MethodRef)) (leaving the rest
of the expression unchanged), ensuring JustTypeRef and the None branch behavior
remain the same.
In `@python/egglog/egraph_state.py`:
- Around line 482-496: Add an inline comment above the UnstableFn branch (the if
ref.ident == Ident.builtin("UnstableFn") block) explaining the expected
structure of ref.args and how type_args are constructed: document that
ref.args[0] is the return type, ref.args[1:] are parameter types (and when
len(ref.args) <= 1 we use a Unit() literal for the param-list), and that the
code builds a bindings.Call with the return type as the head and parameter types
as Var children (referencing bindings.Call, bindings.Lit, and bindings.Var in
this block); keep it short and local to the type_args construction to improve
future maintainability.
In `@python/egglog/exp/array_api_program_gen.py`:
- Around line 560-579: The parameter v in the ruleset function
_vec_recursive_value_program is unused; either remove it from the signature or
mark it explicitly as unused (e.g., rename to _v) or add a comment explaining
it's required by array_api_program_gen_ruleset.register's signature pattern;
update the function signature for _vec_recursive_value_program (and any
registration call) accordingly and add a brief inline comment referencing
vec_recursive_value_program and recursive_value_program so future readers know
why the parameter was kept if you choose not to remove it.
In `@python/tests/test_array_api.py`:
- Around line 458-479: Rename the test parameter that shadows the builtin:
change the parametrize tuple and the test function signature from ("input",
"expected") / def test_polynomial_factoring(input: Value, expected: Value) to
("input_expr", "expected") / def test_polynomial_factoring(input_expr: Value,
expected: Value), and update all intra-function references (e.g.,
egraph.let("x", input) -> egraph.let("x", input_expr)) so the parameter name
matches everywhere (keeping the pytest.param entries intact except for the
renamed parameter).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c1c44b9-f994-49c2-8186-da02b873a6cb
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockdocs/explanation/2026_02_yarn-polynomials.gifis excluded by!**/*.gifuv.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
.github/AGENTS.md.github/workflows/CI.ymlAGENTS.mdCargo.tomldocs/changelog.mddocs/conf.pydocs/explanation/2026_02_containers.mddocs/how-to-guides.mddocs/reference/egglog-translation.mddocs/reference/python-integration.mdpyproject.tomlpython/egglog/__init__.pypython/egglog/bindings.pyipython/egglog/builtins.pypython/egglog/conversion.pypython/egglog/declarations.pypython/egglog/deconstruct.pypython/egglog/egraph.pypython/egglog/egraph_state.pypython/egglog/exp/array_api.pypython/egglog/exp/array_api_jit.pypython/egglog/exp/array_api_loopnest.pypython/egglog/exp/array_api_numba.pypython/egglog/exp/array_api_program_gen.pypython/egglog/exp/polynomials.pypython/egglog/exp/program_gen.pypython/egglog/pretty.pypython/egglog/runtime.pypython/egglog/thunk.pypython/egglog/type_constraint_solver.pypython/egglog/visualizer_widget.pypython/tests/__snapshots__/test_array_api/TestLoopNest.test_index_codegen[expr].pypython/tests/__snapshots__/test_array_api/test_jit[lda][code].pypython/tests/__snapshots__/test_array_api/test_jit[lda][expr].pypython/tests/__snapshots__/test_array_api/test_jit[lda][initial_expr].pypython/tests/__snapshots__/test_array_api/test_jit[tuple][code].pypython/tests/__snapshots__/test_array_api/test_jit[tuple][expr].pypython/tests/__snapshots__/test_array_api/test_jit[tuple][initial_expr].pypython/tests/__snapshots__/test_array_api/test_program_compile[tuple][expr].pypython/tests/test_array_api.pypython/tests/test_high_level.pypython/tests/test_pretty.pypython/tests/test_program_gen.pypython/tests/test_runtime.pypython/tests/test_type_constraint_solver.pypython/tests/test_unstable_fn.pysrc/conversions.rssrc/egraph.rssrc/extract.rssrc/freeze.rssrc/lib.rssrc/py_object_sort.rssrc/serialize.rssrc/termdag.rs
💤 Files with no reviewable changes (2)
- .github/AGENTS.md
- python/tests/snapshots/test_array_api/test_jit[tuple][code].py
Merging this PR will degrade performance by 52.45%
Performance Changes
Comparing 
Footnotes
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP PR to add example for factoring polynomials efficiently using multisets. Eventually the goal here is to explore how to avoid AC blowup by using containers