Add AnyExpr object #366
Add AnyExpr object #366
Conversation
CodSpeed Performance ReportMerging #366 will degrade performances by 32.01%Comparing Summary
Benchmarks breakdown
|
|
I believe these regressions are due to slowdowns in the upstream egglog. |
for more information, see https://pre-commit.ci
saulshanabrook
force-pushed
the
any-object
branch
from
b7a08c0 to
fb4a2c3
Compare
|
@codex can you review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how Python objects are stored in the egglog system, switching from storing live references to serializing values with cloudpickle. Key changes include:
- BREAKING: Drops Python 3.10 support (now requires 3.11+)
- BREAKING:
PyObjectvalues are now stored ascloudpicklebytes instead of live references, allowing duplicates to merge by value - Removes the
PyObjectSortclass in favor of directly handling pickled values - Adds
__call__andcall_extendedmethods toPyObject, deprecatingpy_eval_fn - Introduces
Identdataclass to track module information for identifiers - Adds
__replace_expr__method to support in-place expression mutations - Updates bundled Egglog dependencies and improves error handling
Reviewed Changes
Copilot reviewed 37 out of 42 changed files in this pull request and generated 125 comments.
Show a summary per file
| File | Description |
|---|---|
| src/py_object_sort.rs | Refactored to use pickled bytes instead of storing live Python references |
| python/egglog/declarations.py | Added Ident class and updated all identifier handling to track modules |
| python/egglog/runtime.py | Added __replace_expr__, improved method descriptors, and preserved method handling |
| python/egglog/builtins.py | Added __call__ method to PyObject and deprecated py_eval_fn |
| stubtest_allow | Updated to allow EGraph.__init__ instead of removed PyObjectSort.__init__ |
| pyproject.toml | Bumped minimum Python to 3.11 and added cloudpickle dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| eventTime: Y.eventTime, | ||
| lane: Y.lane, | ||
| tag: Y.tag, | ||
| payload: Y.payload, | ||
| callback: Y.callback, | ||
| next: null | ||
| }; | ||
| X === null ? L = X = N : (X.next = N, X = N), Y = Y.next; | ||
| X === null ? L = X = w : (X.next = w, X = w), Y = Y.next; | ||
| } while (Y !== null); | ||
| X === null ? L = X = b : (X.next = b, X = b); |
There was a problem hiding this comment.
Variable 'X' is of type object, but it is compared to an expression of type null.
| var N = (L.mode & pZ) !== Ql; | ||
| return yJ(c, b, d, K, o, N, X); | ||
| var w = (L.mode & pZ) !== Ql; | ||
| return yJ(c, b, d, K, o, w, X); | ||
| } | ||
| function iH(c, b, d, o) { | ||
| return Me(d, c), d.mode & pZ, SJ(c, b); |
There was a problem hiding this comment.
This expression has no effect.
| El !== null && BZ(ol, El.startTime - Il); | ||
| } | ||
| } | ||
| function Wl(Il, El) { | ||
| T = !1, f && (f = !1, Rb()), v = !0; | ||
| v = !1, f && (f = !1, Rb()), g = !0; | ||
| var sZ = F; | ||
| try { | ||
| var YZ; |
There was a problem hiding this comment.
Unused variable YZ.
|
@codex can you open a PR to modify the AGENTS.md file to account for some of the incorrect issues this copilot flagged here? The warnings in |
|
Summary
Testing
|
Adds experimental support for an
AnyExprobject which can act like an existing Python object and pass on all calls to it by forwarding to the underlying object.Useful for tracing.
TODO:
AnyExprThis PR also makes a number of other changes to get there including changin PyObject's to be stored as pickled bytes, instead of life references. This prevents issues where objects like
(1, 0)would hash the same be stored the same as(1.0, 0). By pickliing and storing that we remove any issues based on hashing. It also makes it easier to support unhashable objects but still have consistant IDs for them.