Skip to content

passing explicit slit to fix guidellm 0.7.1#861

Open
shanjiaz wants to merge 1 commit into
mainfrom
fix-eval
Open

passing explicit slit to fix guidellm 0.7.1#861
shanjiaz wants to merge 1 commit into
mainfrom
fix-eval

Conversation

@shanjiaz

@shanjiaz shanjiaz commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Fix guidellm HuggingFace data loading by adding explicit split=train to load_kwargs. Without this, guidellm 0.7.x receives a DatasetDict instead of a Dataset, causing benchmark runs to fail.

Tests

Run python evaluate.py throughput --target /v1 --subsets HumanEval against a live vLLM server to verify the fix

Checklist

I have filled in:

  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan/results, such as providing test command and pasting the results.
  • (Optional) The necessary documentation update.
  • I (a human) have written or reviewed the code in this pr to the best of my ability.

Signed-off-by: Helen Zhao <shanjiaz@ip-172-31-44-201.us-west-2.compute.internal>
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

run_guidellm now specifies load_kwargs.split=train when loading a dataset subset, while preserving the existing subset JSONL file configuration and non-subset path logic.

Changes

Guidellm dataset loading

Layer / File(s) Summary
Subset split configuration
scripts/evaluate/perf_utils.py
Subset-based HuggingFace loads now include load_kwargs.split=train alongside load_kwargs.data_files={subset}.jsonl; other dataset path logic is unchanged.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is clearly about adding an explicit split to fix guidellm 0.7.1, which matches the change.
Description check ✅ Passed The description matches the change by explaining the split=train fix and the benchmark failure it resolves.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-eval

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mergify

mergify Bot commented Jul 24, 2026

Copy link
Copy Markdown

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews

Protection Waiting on
🔴 Require approval from approved reviewers list 👀 reviews

🔴 Require approval from approved reviewers list

Waiting for any of

  • approved-reviews-by = dsikka
  • approved-reviews-by = fynnsu
  • approved-reviews-by = orestis-z
  • approved-reviews-by = rahul-tuli
  • approved-reviews-by = shanjiaz
This rule is failing.

All pull requests must have at least one approving review from a member of the approved reviewers list before merging.

  • any of:
    • approved-reviews-by = dsikka
    • approved-reviews-by = fynnsu
    • approved-reviews-by = orestis-z
    • approved-reviews-by = rahul-tuli
    • approved-reviews-by = shanjiaz

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
scripts/evaluate/perf_utils.py (1)

568-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a regression assertion for the explicit split.

tests/unit/scripts/test_perf_utils.py currently verifies load_kwargs.data_files=qa.jsonl but not load_kwargs.split=train, so the reported regression could return without failing the test.

Suggested test assertion
 assert "load_kwargs.data_files=qa.jsonl" in data
+assert "load_kwargs.split=train" in data
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/evaluate/perf_utils.py` at line 568, Update the relevant test in
tests/unit/scripts/test_perf_utils.py to assert that the generated configuration
includes load_kwargs.split=train, alongside the existing data_files=qa.jsonl
assertion. Use the test covering the perf utility output affected by the
load_kwargs construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@scripts/evaluate/perf_utils.py`:
- Line 568: Update the relevant test in tests/unit/scripts/test_perf_utils.py to
assert that the generated configuration includes load_kwargs.split=train,
alongside the existing data_files=qa.jsonl assertion. Use the test covering the
perf utility output affected by the load_kwargs construction.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 23628c6f-27cd-4b27-a9d6-defcfd988887

📥 Commits

Reviewing files that changed from the base of the PR and between a491dda and c3efd91.

📒 Files selected for processing (1)
  • scripts/evaluate/perf_utils.py

@shanjiaz shanjiaz changed the title Without an explicit split, guidellm 0.7.x receives a DatasetDict passing explicit slit to fix guidellm 0.7.1 Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant