-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Benchmarking] Add disable_shuffle option for dataset loading #26258
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
Conversation
Added 'disable_shuffle' argument to control the dataset shuffling behaviour. The option keeps the dataset in the original order in the result to be able to evaluate the responses against the ground truth. Signed-off-by: Yasmin Moslem <[email protected]>
👋 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 You ask your reviewers to trigger select CI tests on top of 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 If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
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.
Code Review
This pull request introduces a disable_shuffle
option to control dataset shuffling, which is a valuable addition for ensuring deterministic evaluation. The implementation correctly adds the necessary command-line argument and propagates it to the dataset classes. However, I've identified a critical issue regarding reproducibility. In several dataset loading methods, the random.seed()
call has been moved inside the conditional shuffling block. This causes the random number generator to be unseeded when shuffling is disabled, leading to non-reproducible behavior for other random operations within the benchmark. I've provided comments with suggestions to fix this.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
random.seed(self.random_seed) if not getattr(self, 'disable_shuffle', False): random.shuffle(self.data) Signed-off-by: Yasmin Moslem <[email protected]>
Signed-off-by: Yasmin Moslem <[email protected]>
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.
Looks fine to me!
…roject#26258) Signed-off-by: Yasmin Moslem <[email protected]> Signed-off-by: Karan Goel <[email protected]>
…roject#26258) Signed-off-by: Yasmin Moslem <[email protected]>
…roject#26258) Signed-off-by: Yasmin Moslem <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
Added
disable_shuffle
argument to control the dataset shuffling behaviour. The option keeps the dataset in the original order in the result to be able to evaluate the responses against the ground truth.Currently, the dataset is shuffled, which make the requests in a different order than the original dataset. To make evaluation easier, the shuffling should be optional. This
disable_shuffle
argument disables data shuffling.The change was tested with a
custom
dataset where the input is a *.jsonl file.The main change is modifying:
to be:
If the change is merged, the new argument should be added to this documentation page.