Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undo scheduler attempt #1004

Closed
wants to merge 2 commits into from
Closed

Undo scheduler attempt #1004

wants to merge 2 commits into from

Conversation

pavpanchekha
Copy link
Contributor

Test https://github.com/paulzzy/egg in Herbie. This is the "Undo Scheduler" described on Zulip which should make egg faster and hopefully also better.

@pavpanchekha
Copy link
Contributor Author

The unit tests are failing because I didn't update Cargo.lock, no worries there.

@pavpanchekha
Copy link
Contributor Author

@paulzzy

@pavpanchekha
Copy link
Contributor Author

Ok, here's the branch results vs baseline. @paulzzy you can use the diff feature to compare them if you'd like. Basically, there's almost no change in either results or runtime, which is a bummer.

This might be because Herbie egraphs are quite small (8k nodes) so perhaps the undo scheduler just never gets a chance to fire, I don't know.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 8, 2024

Thanks for trying it out! I'll spend some time looking into what's going on

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 8, 2024

I'm surprised the tests passed because I haven't yet implemented support for explanations (which suggests it wasn't actually running, so I'll figure out how to fix that). Are explanations required by Herbie?

@pavpanchekha
Copy link
Contributor Author

Herbie uses explanations but with them unimplemented I imagine you just return empty explanations? Which Herbie knows to ignore.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 9, 2024

Currently it just panics with todo! (hence why I know for sure it's not running, because otherwise we'd get a bunch of errors), but if Herbie can accept empty explanations that'll make my life a lot easier :)

@pavpanchekha
Copy link
Contributor Author

Oh. Huh, then I wonder what this PR even did. Do you want to take a look at teh code? The egg side is tiny / trivial.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 10, 2024

Currently taking a look! Looks like it panics without explanations being enabled, so I'm figuring out how to bypass that

@pavpanchekha
Copy link
Contributor Author

@paulzzy I'm going to close this PR, since it's not going to be merged and to keep our PRs clean, but please feel free to fork & make PR when you're ready on your end.

@paulzzy
Copy link
Collaborator

paulzzy commented Oct 15, 2024

Thanks Pavel! I'll submit a PR once I get it working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants