-
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
Restore the ability to stop when a specified string has been generated #451
Conversation
Go ahead, this is the right direction! We'll be able to simplify greatly |
outlines/text/generate/api.py
Outdated
@@ -17,13 +18,16 @@ def json( | |||
"The old import path will be removed in Outlines v0.1.0.", | |||
DeprecationWarning, | |||
) | |||
return outlines.generate.json(model, schema_object, max_tokens, sampler=sampler) | |||
return outlines.generate.json( | |||
model, schema_object, max_tokens, stop, sampler=sampler |
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.
The old interface used stop_at
, we should keep that name at least here
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.
I put stop
because it's what continuation
seemed to expect on line 49 and what you removed in your big PR. Was it stop_at
in an earlier version?
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.
Yes it was. stop_at
sounds like a better name anyway, wdyt?
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.
Absolutely!
I have a few questions @rlouf:
|
We can do both with a deprecation warning if passed in
Yes. Should call it
I don't think it's necessary for JSON, Regex, format. But might be useful for grammar-guided generation. |
Is it ready for review? I can take a look at these merge conflicts later. |
32d835e
to
9e9478a
Compare
Yes! |
…ng has been generated
…erator instead of when initializing it
9e9478a
to
e7e9e08
Compare
I made a few cosmetic changes (naming and docstrings) + rebased on |
Looks great! Thanks for adding the docstrings |
This PR aims at solving #417
The proposed change does 2 main things:
SequenceGenerator.__call__
if all sequences contain at least one of the stop_sequences provided by the userA downside of adding this logic at the level of the
SequenceGenerator
is that we generate text that is later discarded. However, incorporating this logic in the fsm would require much bigger change as we need to decode the token_ids to check whether the stop_sequences are found. Maybe this could be a first solution before some future refactoring.As suggested in #433, I think we could treat the max_tokens parameter in a similar way as what's proposed here for the stop_sequences
I've only modified the tests required to pass the tests and the linting, but will modify them further along with documentation in another commit if you think this PR is going in the right direction.