Add eggcc container benchmark and proof coverage#10
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Merging this PR will not alter performance
Performance Changes
Comparing Footnotes |
87185c7 to
d1a80c5
Compare
e33dd55 to
919030c
Compare
oflatt
left a comment
There was a problem hiding this comment.
Had roughly two concerns, one with proof checker (hiding a bug?) and another with files.rs getting more complex (and also hiding another bug perhaps?)
| subst: &mut HashMap<String, TermId>, | ||
| rule_name: &str, | ||
| ) -> Result<(), ProofCheckError> { | ||
| let ResolvedFact::Eq(_, lhs, rhs) = fact else { |
There was a problem hiding this comment.
Why this change? The old comment says is_container_side_condition only flags eq. This seems bad if you ran into otherwise. Actually I wonder if we should have thrown a panic here before
There was a problem hiding this comment.
Removed the changes, they weren't necessary it seems like
| self.term_encoding || self.proofs || self.proof_testing | ||
| } | ||
|
|
||
| fn is_curated_proof_integration(&self) -> bool { |
There was a problem hiding this comment.
what is the point of is_curated_proof_integration?
There was a problem hiding this comment.
Removed this. Now all proof compatible files will be run with proof testing and we create snapshots of their proofs
| if !should_fail && supports_proofs { | ||
| if !should_fail | ||
| && !proof_manually_disabled | ||
| && (supports_proofs || run.is_curated_proof_integration()) |
There was a problem hiding this comment.
Why would something be supports_proofs but not is_curated_proof_integration? I think I could imagine this case due to a known bug in the proof support detection and globals
There was a problem hiding this comment.
Fixed it to be this
| @@ -1,26 +1,57 @@ | |||
| use std::path::PathBuf; | |||
There was a problem hiding this comment.
Yikes, lots of code duplication with the other files.rs... worth fixing in our fork? I'd love to fix in upstream as well
There was a problem hiding this comment.
Yeah I think we could at some point, I think the stuff with testing de-sugarring is also a bit different, was disabled on experimental at some point? Def worth looking into
There was a problem hiding this comment.
yeah we did have to disable desugar round trip on experimental at some opint
919030c to
bc02144
Compare
f9289da to
7d8e104
Compare
Stacked on the subtree-sync branch. This preserves the final reviewed tree from codex/eggcc-container-benchmark while separating local benchmark, proof, CI, and review-fix changes from the subtree merge commits. The hash index ordering fix from egraphs-good/egglog#914 lives in the subtree-sync branch.
7d8e104 to
64ddf2f
Compare
oflatt
left a comment
There was a problem hiding this comment.
just some details on files.rs
| @@ -1,26 +1,57 @@ | |||
| use std::path::PathBuf; | |||
There was a problem hiding this comment.
yeah we did have to disable desugar round trip on experimental at some opint
oflatt
left a comment
There was a problem hiding this comment.
yay files.rs looks like a cleanup now. had one nit, but approving now
e71edaf
into
codex/eggcc-container-subtree-sync
6f0d26e3 Merge pull request #927 from oflatt/oflatt-container-proofs 85ba2626 small feedback fixes 4141a970 Merge pull request #935 from egraphs-good/nightly 5009aaee simplify map.rs e45cd547 address feedback from Saul 3960015c Merge pull request #885 from saulshanabrook/codex/split-scheduler-stale-match-cleanup bd1697a3 Tighten the Containers section of proof_encoding.md (saulshanabrook#10) 5fca83b7 Add container support to the term/proof encoding 7f111370 nightly: prefer the rustup cargo shim over the system cargo (#942) 15888b87 nightly: install deps into a venv to satisfy PEP 668 (#939) f295052e scheduler: skip subsumed fresh matches d1418d20 Merge remote-tracking branch 'upstream/main' into codex/split-scheduler-stale-match-cleanup f99482a2 Merge pull request #936 from oflatt-claude/nightly-eval-live fe240e6b Render nightly benchmark report with eval-live 5bb42afc nightly 6c6f01d5 scheduler: simplify changelog entry c1420100 scheduler: reduce held validation churn ce4ef0b5 scheduler: validate only held matches f0d4822b scheduler: keep stale validation focused 1dedb505 scheduler: condense changelog entry 939339b1 scheduler: simplify stale match validation ed6aa6e9 scheduler: preserve existing match handling 8602f56e scheduler: split action-key dedup cleanup 89779541 scheduler: validate only held matches e24d1468 scheduler: clarify stale match validation eaf4ab4f Merge remote-tracking branch 'upstream/main' into codex/split-scheduler-stale-match-cleanup 0804bee1 scheduler: deduplicate residual match keys 2c55caff Merge remote-tracking branch 'upstream/main' into codex/split-scheduler-stale-match-cleanup 09b8c002 scheduler: document stale match validation 2c749b89 scheduler: clean stale matches git-subtree-dir: egglog git-subtree-split: 6f0d26e37241055a1230bdcdb33f0c5d3f01e935
Summary
Stacked on #9.
This PR contains the local work on top of the subtree sync:
Review Notes
This branch has one commit on top of
codex/eggcc-container-subtree-sync, so the diff should show only the local changes rather than the imported subtree updates.The
egglog/core-relations/src/hash_indexfix from egraphs-good/egglog#914 lives in #9, not this PR.The final tree is identical to the previously reviewed
codex/eggcc-container-benchmarktip5adee2de.Validation
cargo test --workspace --test files proof_integration