Support greedy DAG extraction in experimental - #55
Open
saulshanabrook wants to merge 19 commits into
Open
Conversation
saulshanabrook
changed the title
Member
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughAdds a greedy DAG extractor ( ChangesGreedy DAG Extractor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/logs/2026-06-24-greedy-dag-extractor-perf.md (1)
1-1123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider whether this log file belongs in the repository.
This 1123-line performance experiment log in
.agents/logs/appears to be AI session output rather than permanent project documentation. While valuable for understanding optimization decisions, consider:
- Moving to
docs/perf/greedy-dag-extractor.mdwith sanitized paths and condensed narrative- Keeping only the "Decision" summaries and final validation sections
- Removing the raw log if the key decisions are captured in code comments or a shorter ADR
The current form with extensive local paths, ephemeral temp directories, and external script references (
../egglog_repro/) is not reproducible for other contributors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/logs/2026-06-24-greedy-dag-extractor-perf.md around lines 1 - 1123, The issue is that this `.agents/logs/` file is a long-lived AI experiment log with ephemeral paths and session-specific output, so it should not live in the repository as-is. Move the durable takeaways into a concise doc or ADR such as a performance note for the greedy-DAG extractor, keeping only the relevant decision summaries and final validation results from sections like “Final validation” and “Research idea disposition.” Remove the raw experiment trace, local temp directories, and one-off command history from the tracked tree, and leave the repository with a shorter, reproducible narrative that points to the implemented greedy-DAG changes instead of the transient benchmark log.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/logs/2026-06-24-greedy-dag-extractor-perf.md:
- Line 621: The log entry still says "Status: in progress" for the
diff-reduction pass, so update the status in the relevant log entry to reflect
the actual state if the work is finished, or add a clear finalization note if
the entry is meant to stay historical. Use the existing status field in the log
entry and keep the wording consistent with the surrounding progress notes.
- Line 954: The benchmark log contains ephemeral temp directory paths with
random suffixes, which should be normalized for a permanent record. Update the
referenced path strings in the log entry to use a stable placeholder such as
<tmp-dir> or explicitly label them as example paths, and make the same change
consistently wherever the temp path appears in this document so the logs are
reproducible and not tied to one machine-specific directory.
- Line 265: The experiment log still contains local machine paths like
/private/tmp/ and ../egglog_repro/, which should be sanitized or relocated.
Update the affected entries in the markdown log so they use generic, shareable
paths, and if this content is meant to be a persistent performance note rather
than session output, move it out of the .agents/logs/ area into a more
appropriate docs/perf location. Use the existing log sections and the referenced
Config/command entries to find all remaining path leaks.
- Around line 119-120: The log contains machine-specific absolute paths that
will break for other developers; update the notes in the markdown log to use
generic placeholders or documented variables instead. In the affected log
entries, replace references like the worktree and home-directory paths with
repo-agnostic examples, or add a clear note that they are illustrative, and keep
the guidance consistent across the initial observation and all later path
mentions.
In `@Cargo.toml`:
- Around line 20-22: The three git dependencies for egglog, egglog-ast, and
egglog-reports are currently tracking a moving branch, which makes builds
non-reproducible. Update the dependency entries in Cargo.toml to use the same
fixed immutable git revision instead of branch = "codex/greedy-dag-extractor",
keeping the current repository URL and other settings unchanged.
In `@src/greedy_dag_extract.rs`:
- Around line 843-851: The non-eq root fallback in the greedy extraction branch
ignores nvariants == 0 and may still return one result. Update the logic in the
extract path around extract_best_with_sort to check nvariants first and return
an empty Vec immediately when zero variants are requested, before any fallback
to best extraction or the warning path.
In `@src/keep_best.rs`:
- Around line 124-128: The keep-best extraction path currently reinserts
constructor rows for all enodes, including ones already marked subsumed. Update
the logic in egraph.constructor_enodes so the keep-best flow matches greedy-DAG
discovery by filtering out enodes where enode.subsumed is true before pushing
rows into raw_rows. Use the existing constructor_enodes callback and the
enode.subsumed field to locate and apply the fix.
In `@src/lib.rs`:
- Around line 48-50: The greedy-DAG APIs are not exposed to external callers
because `extract_best_greedy_dag` and `extract_variants_greedy_dag` live in the
private `greedy_dag_extract` module. Update `src/lib.rs` to re-export those
functions alongside `DagCostModel`, or change their visibility to `pub(crate)`
if they are meant only for internal command wiring. Use the existing
`greedy_dag_extract` module and the two function names as the entry points to
fix.
In `@src/multi_extract.rs`:
- Around line 44-49: The UserDefinedCommand implementation for MultiExtract is
missing the extra type bounds needed by extract_variants_greedy_dag. Keep the
MultiExtract struct generic as-is, but update the impl UserDefinedCommand for
MultiExtract<C, CM> so C also requires Ord, Eq, Clone, and Debug alongside Cost
+ Send + Sync. This change should be applied in the MultiExtract impl block and
any related bounds in the UserDefinedCommand methods that call
extract_variants_greedy_dag.
In `@src/set_cost.rs`:
- Around line 220-224: The cost conversion in the set-cost extraction path can
panic on negative user-provided values, so replace the assert-based assumption
in the cost-mapping logic with a safe fallback. Update the closure in
set_cost.rs where egraph.value_to_base::<i64>(c) is converted to DefaultCost to
validate the i64 first, then either skip the invalid row or handle it as invalid
input instead of crashing. Keep the fix localized to the cost-table handling in
the extraction flow.
---
Outside diff comments:
In @.agents/logs/2026-06-24-greedy-dag-extractor-perf.md:
- Around line 1-1123: The issue is that this `.agents/logs/` file is a
long-lived AI experiment log with ephemeral paths and session-specific output,
so it should not live in the repository as-is. Move the durable takeaways into a
concise doc or ADR such as a performance note for the greedy-DAG extractor,
keeping only the relevant decision summaries and final validation results from
sections like “Final validation” and “Research idea disposition.” Remove the raw
experiment trace, local temp directories, and one-off command history from the
tracked tree, and leave the repository with a shorter, reproducible narrative
that points to the implemented greedy-DAG changes instead of the transient
benchmark log.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f837d2e6-c323-4603-ba89-ae4c438dbaaa
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.agents/logs/2026-06-24-greedy-dag-extractor-perf.mdCargo.tomlsrc/greedy_dag_extract.rssrc/keep_best.rssrc/lib.rssrc/multi_extract.rssrc/secondary_map.rssrc/set_cost.rssrc/sugar/for.rssrc/table_stats.rstests/files.rstests/integration_test.rs
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Build / 1_test.txt: Support greedy DAG extraction in experimental
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Build / test: Support greedy DAG extraction in experimental
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
🔇 Additional comments (10)
.agents/logs/2026-06-24-greedy-dag-extractor-perf.md (1)
1076-1076: 📐 Maintainability & Code Quality | ⚡ Quick winGood documentation of checked-in benchmark methodology.
Lines 1104-1108 and 1115-1119 properly document the checked-in
tests/greedy-dag-taylor.eggbenchmark and its performance characteristics. This is the right approach for reproducible benchmarks—prefer checked-in files over ephemeral local copies. The note about CodSpeed substring filtering (Line 1108) is valuable context for future maintainers.Also applies to: 1104-1108, 1115-1119
src/table_stats.rs (2)
264-286: 🎯 Functional CorrectnessVerify the table-kind probe does not swallow real backend errors.
Line 264 treats any
function_entriesfailure as “try constructor rows instead.” IfEGraph::function_entriescan also fail for ordinary backend reasons, this path will mask the real error and retry down the wrong branch. Please confirm the egglog API only returnsErrhere for non-function tables, or branch on a specific error kind instead.
13-15: 🎯 Functional CorrectnessConfirm the hidden-table guarantee matches the implementation.
The updated docs say the no-argument form skips hidden tables, but Lines 345-352 only filter
!f.is_let_binding(). Unlessget_function_names()already omits hidden symbols,print-table-statswill still report them.Also applies to: 328-329
src/sugar/for.rs (1)
2-2: LGTM!Also applies to: 46-48
src/secondary_map.rs (1)
438-438: 🎯 Functional CorrectnessNo change needed:
Option::is_none_oris supported by the declared Rust 1.91.0 toolchain.> Likely an incorrect or invalid review comment.tests/files.rs (1)
3-8: LGTM!Also applies to: 50-59, 126-239
tests/integration_test.rs (1)
56-75: LGTM!Also applies to: 120-225, 487-502, 545-577, 767-767, 805-814
src/set_cost.rs (1)
1-11: LGTM!Also applies to: 240-337
src/multi_extract.rs (1)
6-22: LGTM!Also applies to: 95-124
src/keep_best.rs (1)
3-23: LGTM!Also applies to: 35-96, 132-167
Comment on lines
+20
to
+22
| egglog = { git = "https://github.com/saulshanabrook/egg-smol.git", branch = "codex/greedy-dag-extractor", default-features = false } | ||
| egglog-ast = { git = "https://github.com/saulshanabrook/egg-smol.git", branch = "codex/greedy-dag-extractor", default-features = false } | ||
| egglog-reports = { git = "https://github.com/saulshanabrook/egg-smol.git", branch = "codex/greedy-dag-extractor", default-features = false } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-remote https://github.com/saulshanabrook/egg-smol.git refs/heads/codex/greedy-dag-extractorRepository: egraphs-good/egglog-experimental
Length of output: 249
Pin the egglog git dependencies to a fixed commit.
Cargo.toml:20-22 currently tracks branch = "codex/greedy-dag-extractor" for all three crates; switch them to the same immutable rev to keep builds reproducible and avoid unexpected API drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` around lines 20 - 22, The three git dependencies for egglog,
egglog-ast, and egglog-reports are currently tracking a moving branch, which
makes builds non-reproducible. Update the dependency entries in Cargo.toml to
use the same fixed immutable git revision instead of branch =
"codex/greedy-dag-extractor", keeping the current repository URL and other
settings unchanged.
…g-extractor # Conflicts: # Cargo.lock # Cargo.toml # src/keep_best.rs # src/set_cost.rs # src/table_stats.rs
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
1 participant
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.
Greedy DAG extraction in experimental
Depends on the core support-API PR:
This PR is the experimental half of the greedy-DAG extraction split. Core exposes the extraction support APIs; this PR owns the actual greedy-DAG extractor and the user-facing
:extractor greedy-dagcommand option.Related context:
greedy_dag_cost_modelegglog-python#387faster_greedy_dagreference implementation: https://github.com/egraphs-good/extraction-gym/blob/903ba0f818b50608fe20ae9e0f03c35cb27bc50a/src/extract/faster_greedy_dag.rs_rootargument in DAG cost extractors? extraction-gym#49, Why is fast-greedy-dag worse in terms of dag size? extraction-gym#19, global-greedy-dag sometimes extracts nodes from the wrong class extraction-gym#28Changes
src/greedy_dag_extract.rs.:extractor greedy-dagsupport forextract,multi-extract, andkeep-best; omitting the option still uses tree extraction.DagCostModeltrait and wires the dynamic set-cost model through both tree and greedy-DAG extraction.DagCostModel,TreeCostModelFromDag,extract_best_greedy_dag, andextract_variants_greedy_dag.Read::enodes_for_eclassindexed lookup to discover normal constructor rows for each reachable(sort, value)root.Nonefor ordinary per-root best-extraction misses, matching the new coreExtractedTermsshape; strict commands such askeep-bestand dynamicextractconvert that back intoExtractError.(sort, value)as the requested root. For ordinaryextractcommands, the harness also runs a paired greedy-DAG extraction at the same program point.multi-extract, andkeep-best.keep-best, negative dynamic cost rows, hidden table stats, and repository-local performance log hygiene.Algorithm Notes
The extractor adapts extraction-gym's worklist-based greedy DAG heuristic to egglog values. It discovers producer rows reachable from the requested roots, keeps a once-paid dependency set per candidate DAG, rejects self-reachable choices, and propagates improvements only to producer rows whose dependencies changed.
This experimental implementation is normal-mode only: it uses exact
(sort, value)roots and constructor row outputs, and it does not depend on internal union-find/proof-term canonicalization or view constructors.The result is still a greedy heuristic, not an optimal DAG extractor. Combined-root extraction intentionally selects one producer per reachable
(sort, value)for the requested root set, which matches the practical behavior tested here but does not claim globally optimal per-root choices.Performance
Measured locally with release builds and stdout redirected away:
tests/taylor51.egg: tree985.3 ms +/- 2.5 ms; greedy-DAG210.2 ms +/- 0.9 ms; greedy-DAG was4.69xfaster.taylor7.eggadapted to current syntax in a temporary copy: tree35.355 s +/- 0.124 s; greedy-DAG2.079 s +/- 0.002 s; greedy-DAG was17.01xfaster.CodSpeed
execwas attempted, but the local macOS arm64 harness was unavailable, so these numbers come fromhyperfine.Validation
cargo checkcargo test --test integration_test greedy_dagcargo test --test integration_test multi_extractcargo test --test integration_test keep_bestcargo test --release --test filesmake nitsgit diff --check