-
Notifications
You must be signed in to change notification settings - Fork 399
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
Add CFG-guided generation #391
Conversation
have extended testing and found bug, defer review until next commit (in progress). |
thanks, should be good now! |
Could you run |
Thanks, I agree that merging before #366 makes sense. Taking a peak at that interface refactor, it looks like it should ultimately simplify this implementation as well, which is great. I should note that I am in a bit of a deadline heavy period now, so would not expect to be able to contribute in the next ~2 weeks, but after that would be happy to refactor this if no one else has gotten to it by then. |
d9ddc72
to
6013abf
Compare
FYI: I've rebased and squashed. Still reviewing, though... |
6013abf
to
551517b
Compare
@benlipkin I ended up merging #366 because it was blocking many other improvements. It should be relatively easy to adapt this PR to make it work with the new interface. You will need to implement a |
Thanks @rlouf and apologies for the delays here. My other deadline is now in, and I will begin this refactor today. |
No problem, thank you for taking this on! |
While working on this, I noticed a bug (based on my understanding of how tokens are expected to be tracked by a user) in how It is my understanding that Instead I propose the following: In This |
I'll take a look at that bug tomorrow. We were thinking about moving this logic to |
Have outlined the rest of the PR. This implementation should be sound, in that it will always generate strings within the language, but it is not complete, in that there are strings in the language that cannot be generated. This surrounds a particular branching point of determining when an underlying regex is done, thereby allowing us to transition to the next CFG parser state, which I point out here. If a particular regex's only next state is generating the EOS token, then we can easily transition to the next regex. But, if the regex is in a final state, yet could generate more, e.g., the string Relatedly, the |
Also, placing this in a separate comment, but @brandonwillard , I tried out plugging in the |
No problem, we can do all that in a follow-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: We'll need to rebase to remove the merge commit and get things matched up with main
. One of us can do that, @benlipkin, once you've reached a good stopping point, if you'd like.
Just rebased. That edge case I raised still needs to be covered at some point, but otherwise should be set. |
abc0b17
to
dd92642
Compare
Great addition! I rebased the code on |
This is because the If we remove the following lines from the grammar:
leaving:
then, generations look as follows:
|
dd92642
to
5c59632
Compare
There is a merge conflict to resolve. We will need a few tests for |
Thanks, have resolved the merge conflict and added a few test cases. Let me know if others would be beneficial. I also outlined the previously raised edge case in a commented-out test for subsequent consideration once there is consensus on how to use the sampler to drive that decision point. |
LGTM, we just need to rebase on |
8ab8be6
to
317c793
Compare
Thank you for adding the tests! I'm merging and cutting a new release |
Hi - I subscribed to this because this feature is quite useful for me. I wonder if it's also worth putting in the README? Both in the There are other types of guided generation described there, so it seems a shame to miss this one (but maybe as someone interested in this feature, I'm overestimating how many others are interested). |
Sure, I would be happy to review a PR if you feel up to adding a simple example in the README. Can also be added in the "features" bullet points :) |
Just went to look at this and saw you did it yourself - thanks, appreciate it, and Outlines! |
Thanks to the outlines team for putting together a great library. Had been rolling my own limited solutions for constrained generation tasks until I came across this. outlines has now been subbed into most of my use cases.
One additional feature that I've waiting on a nice solution for is CFG-guided generation. Since I like the outlines API, I took a peak at the library, and have put together an initial draft of a
CFG
class here along with a working example here.The high-level summary is as follows:
Regex
class'screate_proposal
method is called repeatedly until the regex is completed.This has been checked for batch_size>1, but still needs to be stress-tested quite a bit more in terms of the range of grammars, etc. Looking forward to comments and feedback.