feat: add pretty run report#416
Conversation
saulshanabrook
left a comment
There was a problem hiding this comment.
Thank you for this! Added a few comments. Could you also add this to the changelog file with a link to this PR?
Merging this PR will improve performance by 77.41%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_jit[lda] |
15.8 s | 7.9 s | +99.89% |
| ⚡ | Simulation | test_jit[lda] |
11.6 s | 7.3 s | +57.46% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing kaeun97:kaeun97/pretty-report (568d6bf) with main (8812ec9)2
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(568d6bf) during the generation of this report, so 8812ec9 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
There was a problem hiding this comment.
Thanks for the fixes, I left a few small comments. There are also some mypy and formatting issues I think.
There is a bigger question about performance, if the codspeed is correct it looks like this slows things down by a ton!
Taking almost 40% of the time in a bigger benchmark just to translate bindings.
It makes me wonder about a different approach, where we set each rewrite and rule with a manual name like 1, 2, 3, ... and then we don't have to do the name searching and mangling and can just parse the name as an int then look it up? And if it's a birewrite just take off the <= or >=?
It would make the egglog file a bit more verbose, but makes parsing the reports more straightforward and more performant which seems like a good tradeoff?
I was also going back and forth on whether the RunReport should store a RewriteOrRule or the decl? If we just store the RewriteOrRule it's easier to pretty print, can just use the builtin one, and it's easier for users to grab that off and compare it or use it... But most of the other exposed objects just store the decls, so I will leave it up to you!
EDIT: It looks like the docs failures also highlight some other exceptions from this. I imagine also if we name the rules here that might also help since it seems like it's hitting on looking up the string?
|
@saulshanabrook Thanks for the thorough review! I do agree that the performance looks concerning. The numeric name approach you mentioned would work for bindings with a "name" field - so not for, |
Ah yeah I kept forgetting about this! I just talked to some other folks on the egglog team and they said that sounds like a great feature to add, just something we hadn't gotten around to yet. It should also I think be relatively straightforward so a good first PR to egglog core if you don't mind doing that... Then once that is merged hopefully should just be able to update the pin here and can use that feature. I believe the version of egglog we depend on here is pretty recent, so hopefully won't be other changes we have to adapt to. |
|
Hey @saulshanabrook , thank you again for your feedback. The benchmark seems much better now. Let me know how it looks! |
saulshanabrook
left a comment
There was a problem hiding this comment.
Thanks again for your continued updates on this!
I have some additional cleanup feedback, to try and keep the data structures a bit more minimal and specific, raise any errors earlier, and make sure bi-rewrite preserves both times.
|
@saulshanabrook , thank you for the clean up suggestion. It does indeed made it much nicer to read and use! Let me know how this looks! |
saulshanabrook
left a comment
There was a problem hiding this comment.
This looks good! I just need to figure out why the checks are failing, they seem to all get cancelled and I am not sure why... I dont think its due to this PR?
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📜 Recent 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). (8)
🧰 Additional context used📓 Path-based instructions (1)python/egglog/**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🪛 Ruff (0.15.14)python/tests/test_run_report.py[error] 6-6: (F403) [warning] 11-11: Missing return type annotation for private function (ANN202) 🔇 Additional comments (20)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR implements a Python-friendly wrapper layer for e-graph execution reports. It adds deterministic rule naming to the state, introduces dataclass models that translate low-level rule identifiers to their originating command declarations, and exposes these structured reports through the public EGraph API instead of raw bindings payloads. ChangesRun Report Wrapper and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@kaeun97 Could you add eggcc-2mm to the SKIP_TESTS: I think the CI is getting stuck on it somehow. You could also just add a pytest skip mark to the whole thing here: egglog-python/python/tests/test_bindings.py Lines 48 to 56 in 8812ec9 There were new examples that got added in the egglog commits that I think are breaking CI for some reason... They do pass locally. |
|
@saulshanabrook Thanks Saul, finally got the CI to pass. Excited to land this! |
|
@kaeun97 Amazing! Thank you so much for your diligent work on this :) Was looking to release this after and realized we just wanna move the changelog entry up so that the automatic release pipeline puts it in the right place! |
Resolves #398.
Here is an example code:
Output before:
Output after: