[Bugfix] Multi-sequence broken #11898
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can do one of these:
🚀 |
vllm/sequence.py
Outdated
| n = self.sampling_params.n | ||
| assert isinstance(n, int) | ||
| if n > self.num_seqs(): | ||
| # At prompt stage, the sequence group is not yet filled up | ||
| # and only have one sequence running. However, in the | ||
| # generation stage, we will have `n` sequences | ||
| # running. | ||
| return n | ||
| # At sampling stages, return the number of actual sequences | ||
| # that are not finished yet. | ||
| return self.num_seqs() - self.num_finished_seqs() |
There was a problem hiding this comment.
when will we hit this? I think the engine will only see single-sequence request
There was a problem hiding this comment.
When you construct the output when n > 1 you access the "master group".
There was a problem hiding this comment.
| params = copy.deepcopy(original_params) | ||
| params.n = 1 | ||
| if params.seed is not None: | ||
| params.seed += i |
There was a problem hiding this comment.
this part makes sense to me.
|
@andylolu2 thanks for the fix! can you add a test case for |
andylolu2
force-pushed
the
main
branch
3 times, most recently
from
7c31b9c to
debff7f
Compare
|
@youkaichao I added new asserts in the current tests to ensure each sample in the same parallel-sampling group gives different results. |
|
@andylolu2 please fix the format. |
|
Woops, thanks for reminding |
Signed-off-by: Andy Lo <[email protected]>
Signed-off-by: Andy Lo <[email protected]>
Signed-off-by: Andy Lo <[email protected]>
|
@youkaichao Should be good now |
|
cases with |
|
|
||
| # verify generations within the same parallel sampling group differ | ||
| for output in outputs: | ||
| for sub_output_a, sub_output_b in combinations(output, 2): | ||
| assert sub_output_a != sub_output_b |
There was a problem hiding this comment.
would this test be flaky? e.g. when we generate with n=10, if two sequences happen to generate the same answer ...
|
@andylolu2 We have switched to use pre-commit, can you please update this PR with the latest changes in main? Thanks! |
Signed-off-by: Andy Lo <[email protected]> Signed-off-by: Bowen Wang <[email protected]>
Signed-off-by: Andy Lo <[email protected]>
|
Sorry about that, was caught up in other matters. Glad that it got merged :) |
Fixes the bugs introduced in #9569
SequenceGroupdoes not necessarily contain only one sequence (e.g. whenn> 1), so many of the optimisations don't make sense.n> 1 with seed set, all completions give the same output.Andy@MistralAI