[V1][Spec Decode] Add random seed for EAGLE and its test script #16235
Conversation
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
wwl2755
requested review from
WoosukKwon,
robertgshaw2-redhat,
njhill,
ywang96,
comaniac and
alexm-redhat
as code owners
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run 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 either: Add 🚀 |
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
|
This should be a minor fix that won't affect the codebase much. Any comments/reviews are appreciated! |
| batch_size = probs.size(0) | ||
| for i in range(batch_size): | ||
| generator = sampling_metadata.generators.get(i, None) | ||
| q[i].exponential_(generator=generator) |
There was a problem hiding this comment.
Is it possible to use q.exponential_(generator=generator) to avoid for loop and leverage vectorization?
the generator will anyway depend on the batch id of the request so it cannot be reproducible in all situation but having a single generator for the entire batch means that order of the seq in batch doesnt matter
There was a problem hiding this comment.
I just checked the torch.Tensor.exponential_(), and it seems can only take one generator instead of a vectorized one.
If it is only for reproduction issues, I think using one representative generator should be fine. But I'm concerned about any use case that different sequences of the batch may require different seed generators.
| for i in range(batch_size): | ||
| generator = sampling_metadata.generators.get(i, None) | ||
| q[i].exponential_(generator=generator) | ||
|
|
||
| next_token_ids = probs.div_(q).argmax(dim=-1).view(-1) |
There was a problem hiding this comment.
Do you know why probs need to be divided by q which is randomly init using an exponential distribution?
There was a problem hiding this comment.
I'm not quite sure about the rigious math proof behind, but I believe it is a simplified version of sampling from probs with randomness instead of always picking the max probability.
|
This pull request has merge conflicts that must be resolved before it can be |
This PR added and tested seed-based random generator in EAGLE for reproducibility, as mentioned in task 5 from #15901 .