Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
github-actions
Bot
added
the
size/normal
There was a problem hiding this comment.
Pull request overview
Adds first-run, out-of-the-box download + local caching for the official MLPerf Agentic Inference dataset when the predefined agentic_inference_conversations dataset is used without a local path, so example configs can reference the predefined dataset ID directly.
Changes:
- Implemented download/caching + SHA-256 verification for
agentic_inference_conversationsin the dataset manager. - Updated the Agentic Inference example configs to use the predefined dataset (no hard-coded
path). - Added a unit test for the download/caching flow and updated the example README to document the behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/inference_endpoint/dataset_manager/agentic_inference_dataset.py |
Adds automatic download/caching and SHA-256 verification for the official Agentic Inference dataset. |
tests/unit/dataset_manager/test_agentic_inference_dataset.py |
Adds a unit test that exercises the auto-download/cache behavior via monkeypatching. |
examples/10_Agentic_Inference/README.md |
Documents automatic download behavior and how to use a custom JSONL instead. |
examples/10_Agentic_Inference/qwen_agentic_benchmark.yaml |
Switches to the predefined agentic_inference_conversations dataset ID (no path). |
examples/10_Agentic_Inference/kimi_agentic_benchmark.yaml |
Switches to the predefined agentic_inference_conversations dataset ID (no path). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
=======================================
Coverage ? 81.40%
=======================================
Files ? 152
Lines ? 20522
Branches ? 0
=======================================
Hits ? 16706
Misses ? 3816
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/inference_endpoint/dataset_manager/agentic_inference_dataset.py:263
_verify_sha256()usespath.read_bytes(), which loads the entire dataset into memory. The official Agentic dataset can be large, so this risks high peak RAM usage (and OOM) just to compute the checksum. Stream the file in chunks when hashing.
def _verify_sha256(cls, path: Path) -> None:
"""Raise ValueError if the cached dataset is not the official artifact."""
digest = hashlib.sha256(path.read_bytes()).hexdigest()
if digest != cls.DATASET_SHA256:
raise ValueError(
hvagadia
left a comment
hvagadia
left a comment
There was a problem hiding this comment.
Thanks for getting this in.
|
probably need to address those comments from copilot but good feature to have |
| def _verify_sha256(cls, path: Path) -> None: | ||
| """Raise ValueError if the cached dataset is not the official artifact.""" | ||
| digest = hashlib.sha256(path.read_bytes()).hexdigest() | ||
| if digest != cls.DATASET_SHA256: | ||
| raise ValueError( | ||
| f"SHA-256 mismatch for {path.name}: " | ||
| f"expected {cls.DATASET_SHA256}, got {digest}" | ||
| ) |
There was a problem hiding this comment.
Can we chunk/paginate this file read and also extract this function to a utility file - this would be useful for other files where we would like to compute the SHA.
There was a problem hiding this comment.
Centralized MLCommons R2 downloader and sha256 verification from agentic inference, bfcl, and open-orca. It became a decently-sized refactor so please let me know if this is the right direction.
There was a problem hiding this comment.
🟡 Changes recommended
The new download helper uses Path.rglob(artifact_name) which interprets the artifact name as a glob pattern and can select the wrong file or report ambiguity for certain filenames.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new downloader utility executes a fetched shell script but is missing a couple of low-cost hardening checks (commit ref validation, clearer fetch failure context, and symlink filtering) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/inference_endpoint/dataset_manager/download.py:90
download_r2_artifact()acceptsdownloader_commitas a free-form string, so callers can pass a mutable ref likemain(or a non-SHA path fragment), undermining the “pinned script” guarantee and weakening the trust model for executing the downloaded shell script. Consider validating this argument as a 40-character hex commit SHA before building the URL.
src/inference_endpoint/dataset_manager/download.py:100- If fetching the downloader script fails (DNS, TLS, 404, etc.), the current code will surface a raw
requestsexception without context about which URL was being fetched. Wrapping it in aRuntimeErrorwith the URL makes CLI failures much easier to diagnose.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| candidates = [ | ||
| path | ||
| for path in destination_dir.rglob("*") | ||
| if path.is_file() and path.name == artifact_name and path != script_path | ||
| ] |
feat(agentic): auto-download official dataset
What does this PR do?
Add out-of-the-box download support for the official MLPerf Agentic Inference
dataset when the predefined
agentic_inference_conversationsdataset isconfigured without a local path.
The Kimi and Qwen agentic inference configs now use the predefined dataset ID
without a hard-coded dataset path. The example README documents the automatic
download behavior and the fallback for users supplying a custom JSONL path.
Type of change
Related issues
Related to #488, which added
the official dataset link and checksum to the Agentic Inference README.
Testing
Checklist