Skip to content

[https://nvbugs/6215107][fix] Namespace cnn_dailymail repo id in load_calib_dataset#15802

Closed
Tabrizian wants to merge 1 commit into
NVIDIA:mainfrom
Tabrizian:cnn-dailymail-calib-dataset-6215107
Closed

[https://nvbugs/6215107][fix] Namespace cnn_dailymail repo id in load_calib_dataset#15802
Tabrizian wants to merge 1 commit into
NVIDIA:mainfrom
Tabrizian:cnn-dailymail-calib-dataset-6215107

Conversation

@Tabrizian

@Tabrizian Tabrizian commented Jun 30, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes
    • Improved dataset loading compatibility by recognizing the cnn_dailymail dataset name in its newer namespaced form, helping avoid load failures with updated Hugging Face requirements.

Description

Newer huggingface_hub releases reject the legacy bare cnn_dailymail
dataset id and raise:

huggingface_hub.errors.HfUriError: Invalid HF URI
'hf://datasets/cnn_dailymail@.../.huggingface.yaml'.
Repository id must be 'namespace/name', got 'cnn_dailymail'.

The ModelOpt calibration path (quantize_by_modelopt.get_calib_dataloader /
get_nemo_calib_dataloader) was already fixed to rewrite the bare id to the
namespaced abisee/cnn_dailymail repo (#14930). However,
tensorrt_llm/models/convert_utils.py::load_calib_dataset — the calibration
dataset loader used by the TensorRT-backend convert.py scripts (llama, gpt,
qwen, chatglm, baichuan, bert, gemma, …), which all default calib_dataset
to cnn_dailymail — still forwarded the bare id straight to
datasets.load_dataset() and hit the same HfUriError.

This PR closes that remaining gap: load_calib_dataset now rewrites the bare
cnn_dailymail id to the namespaced abisee/cnn_dailymail repo before
loading, mirroring the existing ModelOpt fix. Already-namespaced ids (e.g.
ccdv/cnn_dailymail) and local dataset directories are unaffected.

NVBug: https://nvbugs/6215107

Test Coverage

The change mirrors the already-merged ModelOpt fix (#14930) for the identical
root cause. It is exercised by the existing Triton-backend FP8 quantization
tests that originally hit the HfUriError, e.g.:

  • triton_server/test_triton_llm.py::test_llama_v3_speculative_decoding_bls
  • triton_server/test_triton_rcca.py::test_rcca_bug_4714193

as well as any TensorRT-backend convert.py calibration run that uses the
default --calib_dataset cnn_dailymail.

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

@Tabrizian

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56734 [ run ] triggered by Bot. Commit: d03a874 Link to invocation

…_calib_dataset

Newer huggingface_hub rejects the legacy bare "cnn_dailymail" dataset id
and raises HfUriError ("Repository id must be 'namespace/name'") while
resolving the dataset. load_calib_dataset() in convert_utils.py still
forwarded the bare id to datasets.load_dataset(), breaking the default
calibration path of the TensorRT convert scripts. This is the same root
cause already fixed for the ModelOpt path in quantize_by_modelopt.py.

Rewrite the bare id to the namespaced "abisee/cnn_dailymail" repo before
loading, mirroring the existing fix.

Signed-off-by: Iman Tabrizian <10105175+tabrizian@users.noreply.github.com>
@Tabrizian
Tabrizian force-pushed the cnn-dailymail-calib-dataset-6215107 branch from d03a874 to 85881a7 Compare June 30, 2026 22:08
@Tabrizian

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4508a55b-45ff-4a6c-a64e-c3daf23cb5fd

📥 Commits

Reviewing files that changed from the base of the PR and between a721a08 and 85881a7.

📒 Files selected for processing (1)
  • tensorrt_llm/models/convert_utils.py

📝 Walkthrough

Walkthrough

This change modifies load_calib_dataset() in convert_utils.py to rewrite the dataset identifier "cnn_dailymail" to the namespaced "abisee/cnn_dailymail" before invoking load_dataset, accommodating updated HuggingFace Hub naming requirements.

Changes

Calibration Dataset Loading

Layer / File(s) Summary
Dataset id rewrite
tensorrt_llm/models/convert_utils.py
load_calib_dataset now rewrites the bare identifier "cnn_dailymail" to "abisee/cnn_dailymail" before calling load_dataset.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, specific, and matches the bug-fix scope and NVBugs format.
Description check ✅ Passed The description follows the required template and includes a clear problem statement, test coverage, and checklist completion.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56736 [ run ] triggered by Bot. Commit: 85881a7 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #56736 [ run ] completed with state SUCCESS. Commit: 85881a7
/LLM/main/L0_MergeRequest_PR pipeline #45556 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@Tabrizian

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57399 [ run ] triggered by Bot. Commit: 85881a7 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57399 [ run ] completed with state FAILURE. Commit: 85881a7
/LLM/main/L0_MergeRequest_PR pipeline #46144 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@Tabrizian

Copy link
Copy Markdown
Member Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57789 [ run ] triggered by Bot. Commit: 85881a7 Link to invocation

@Tabrizian

Copy link
Copy Markdown
Member Author

/bot kill

@Tabrizian Tabrizian closed this Jul 6, 2026
@Tabrizian

Copy link
Copy Markdown
Member Author

The tests are being removed here: #15907

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57791 [ ] completed with state FAILURE. Commit: 85881a7
Not allowed on merged PR

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #57789 [ run ] completed with state SUCCESS. Commit: 85881a7
/LLM/main/L0_MergeRequest_PR pipeline #46488 completed with status: 'SUCCESS'

CI Report

Link to invocation

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.

2 participants