-
Notifications
You must be signed in to change notification settings - Fork 401
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
RegexPrefixAllowedTokens does not work for batch #789
RegexPrefixAllowedTokens does not work for batch #789
Comments
You are correct, this needs to be fixed. |
Thanks @rlouf . I'm looking to fix this too but just to get more context, it seems like this function generally does not allow parallelized operations across the batch is that true? I also wonder why not implement an equivalent of JSONLogitsProcessor, is this method faster? |
Thank you for considering contributing. I am not sure why this choice was made at the time, you'd have to ask @saattrupdan |
@rlouf what are your thoughts on replacing This will ensure consistency between different inference engines integrations and move us closer to having one method of applying a |
@lapp0 Here’s how I currently use it: Also, not sure if having a consistent interface between the transformers integration and the llama.cpp integration makes sense, as they are different packages and thus need different treatment? Were you thinking of any change in particular? |
Yes, this is where I was going with the change in the |
It works for me except for the case that no input IDs are passed as described in the issue. If you pass just a I think the main issue is this regression was introduced through a distinct interface.
It does exist. We should close this issue in favor of #806 |
@lapp0 What I don't understand is maybe exactly what you mean by "replacing the transformers integration with logits processors". Logits processors have a different type signature than the prefix allowed tokens functions (for instance, one outputs floats and the other ints), so can't see how they can replace the current integration. Further, and most importantly, will such a change still mean that I can take a |
Logits processors can accomplish the same goal, but they require code changes to use. I see only a handful of public repos using these classes. https://github.com/search?q=%22JSONPrefixAllowedTokens%22&ref=opensearch&type=code
Yes, I will design it such that you can use it directly with
|
@lapp0 But the transformers model classes don't accept a logits_processor argument. Are you talking about the outlines wrapper class here? |
Here's a dummy example that shows a logits processor which restricts tokens to those with IDs > 2000. Please make sure you're on the latest version of
|
@lapp0 Aha, I didn't realise they included that now - thanks for explaining! 😊 |
@saattrupdan could you please tell me if |
@lapp0 It's not working as expected. When it's in a loop, only the first generation will succeed. ##Code: model_uri = "microsoft/Phi-3-mini-4k-instruct" outlines_tokenizer = outlines.models.TransformerTokenizer( generator = transformers.pipeline('text-generation', model=model_uri) for _ in range(3): ##Output: |
@sigjhl logits processors are stateful and for one time use. I'll be adding this to the documentation, thanks for pointing it out. For now you could try
Please let me know if you have any other questions |
Describe the issue as clearly as possible:
In these lines of code,
outlines/outlines/integrations/transformers.py
Lines 87 to 100 in 3a41b0e
it seems that the batch id is not used at all, and there is an error triggered when batch is used and input_ids is empty.
outlines/outlines/integrations/transformers.py
Line 116 in 3a41b0e
Do you know how this can be fixed? Thanks! @saattrupdan @rlouf
Steps/code to reproduce the bug:
Expected result:
Error message:
No response
Outlines/Python version information:
Version information
Context for the issue:
No response
The text was updated successfully, but these errors were encountered: