-
Notifications
You must be signed in to change notification settings - Fork 485
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
Major bug & fix: Fix bug in batched multi sample generation #1025
Conversation
Co-authored-by: Patrice Bechard <[email protected]>
folks this is serious |
Merci @patricebechard are you able to give approval / merge? |
Thanks so much for finding and fixing this bug! Could you please add a test case which fails in Also as an alternative we might consider #966 |
Nop I am not a maintainer, just trying to help :) |
It's, when you use a sampler with multiple sequences returned per batch
sample (so, like generating 10 possible outputs for each input with
multinomial generation), the batch and sequences are merged into a single
super batch of size n_samples * batch_size, on which the constraints &
generation are applied (correctly) as is it was a regular batch.
Everything is fine until you try to re split these back into a list of
batch size with n_samples generations for each input, giving a dimension of
(batch_size, num_samples), this is done incorrectly. In practice you almost
only get samples from the first batch generations in the second batch
units, it's completely broken.
…On Tue, Jul 9, 2024, 7:41 PM Andrew Lapp ***@***.***> wrote:
Thanks so much for finding and fixing this bug!
Am I understanding correctly that the pattern is enforced within each
sequence, however the next token from sequence A ends up in sequence B?
Could you please add a test case which fails in main and passes with your
fix?
—
Reply to this email directly, view it on GitHub
<#1025 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYU34P72TYYCA3IIWOXSL3ZLSNMFAVCNFSM6AAAAABKP7VPICVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJZGM4TSNZRGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Making a test that fails is really hard without an option to return the inputs as part of the outputs. Having that option would make it easier though. |
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 was able to reproduce your results on main
and verify your fix resolves the issue!
Thank you for contributing a fix! In the future, please do not tag maintainers and other users in the PR. |
Sorry, I thought you would have liked to know that was broken, wanted to be
sure that you would see it. It was a very serious bug.
…On Thu, Jul 11, 2024, 12:03 PM Rémi Louf ***@***.***> wrote:
Merged #1025 <#1025> into
main.
—
Reply to this email directly, view it on GitHub
<#1025 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYU34P6E6JMZ7UZ3FOEIE3ZL3JG5AVCNFSM6AAAAABKP7VPICVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGQ3TMMRZHE4TGNA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The following lines break in batched generation.
There is a single list that is
[b_0_s_0, b_0_s_1, b_0_s_2, b_0_s_3, b_1_s_0, b_1_s_1, ...]
with
b_0_s_0
being the example generated for batch 0 and sample 0 of multi-sample batch generation.At the end of the generation code, the following tries to separate the generated samples in a
batch_size
quantity of sub lists. The problematic code is as follows:We indeed get a list of batch size, but not the expected ones. Instead it should be:
As an example, using the prompts:
'1 1 1 + 3 3 3 =? Solution: 1 1 1 + 3 3 3 =
and
'2 2 2 2 + 4 4 4 4 =? Solution: 2 2 2 2 + 4 4 4 4 =
which gives a batch size of 2, with the
\d( \d)+
regex, and with multinomial generation with a number of samples of 10, the current wrong output is (if you remove excluding the prompt from the output, which I did because the outputs looked fishy):We can see the problem, it returned
next_tokens[0 : num_samples]
andnext_tokens[1 : num_samples + 1]
which is not what we want.The new code returns
Which corresponds to the index ranges
[0 : num_samples]
and[num_samples : 2 * num_samples]
, which is what we want.A more simple complete example is
gives
when not fixed (see how the second list is just the first list with one item at the start fewer, and one reasonable-looking generation at the end),
and, when fixed: