Skip to content

[None][fix] Update dataset identifier for cnn_dailymail to use namespaced repo in quantization scripts - #14930

Merged
yufeiwu-nv merged 3 commits into
NVIDIA:mainfrom
yufeiwu-nv:bug
Jun 4, 2026
Merged

[None][fix] Update dataset identifier for cnn_dailymail to use namespaced repo in quantization scripts#14930
yufeiwu-nv merged 3 commits into
NVIDIA:mainfrom
yufeiwu-nv:bug

Conversation

@yufeiwu-nv

@yufeiwu-nv yufeiwu-nv commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Signed-off-by: yufeiwu-nv 230315618+yufeiwu-nv@users.noreply.github.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved compatibility with newer HuggingFace Hub versions for calibration dataset loading.

Description

Test Coverage

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • 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.

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds dataset id remapping logic to two quantization dataloader functions to address HuggingFace Hub compatibility. When "cnn_dailymail" is passed as the dataset identifier, both functions now remap it to the namespaced repository "ccdv/cnn_dailymail" before calling load_dataset.

Changes

HuggingFace dataset id compatibility

Layer / File(s) Summary
Remap bare cnn_dailymail across calibration loaders
tensorrt_llm/quantization/quantize_by_modelopt.py
get_calib_dataloader and get_nemo_calib_dataloader both conditionally remap "cnn_dailymail" to "ccdv/cnn_dailymail" before invoking load_dataset to maintain compatibility with newer HuggingFace Hub versions.

Possibly related PRs

  • NVIDIA/TensorRT-LLM#14671: Adjusts how the cnn_dailymail dataset is resolved, complementing this PR's HuggingFace id remapping with local path handling changes.
  • NVIDIA/TensorRT-LLM#13722: Also modifies get_calib_dataloader() and get_nemo_calib_dataloader() logic for cnn_dailymail-related dataset inputs via different matching rules.

Suggested labels

Community want to contribute

Suggested reviewers

  • longlee0622
  • ruodil

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is entirely empty except for template content and a sign-off. Required sections like Description and Test Coverage are not filled in, providing no explanation of the issue, solution, or test coverage. Provide a Description section explaining the issue (why bare dataset ID no longer works) and the solution. Add a Test Coverage section documenting how the changes are tested.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: updating dataset identifier for cnn_dailymail to use namespaced repo in quantization scripts, matching the code changes.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch bug

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

@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.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@tensorrt_llm/quantization/quantize_by_modelopt.py`:
- Around line 529-532: The remap for the "cnn_dailymail" dataset should point to
the canonical Hugging Face repo "abisee/cnn_dailymail" (and optionally pin the
documented revision 3.0.0) rather than "ccdv/cnn_dailymail"; update the
conditional in quantize_by_modelopt.py that handles dataset_name_or_dir (the
branches checking for "cnn_dailymail") to set dataset_name_or_dir =
"abisee/cnn_dailymail" (or "abisee/cnn_dailymail@3.0.0" if you want to pin the
version), and apply the same fix to both occurrences mentioned in the review
(the blocks around where dataset_name_or_dir is reassigned). Ensure the logic
still preserves the original intent of replacing the bare "cnn_dailymail" id.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 155ed7b2-48c7-475e-9660-e3862b0e0db0

📥 Commits

Reviewing files that changed from the base of the PR and between e1212ad and b2a911f.

📒 Files selected for processing (1)
  • tensorrt_llm/quantization/quantize_by_modelopt.py

Comment thread tensorrt_llm/quantization/quantize_by_modelopt.py Outdated
…aced repo in quantization scripts

Signed-off-by: yufeiwu-nv <230315618+yufeiwu-nv@users.noreply.github.com>
@yufeiwu-nv

Copy link
Copy Markdown
Collaborator Author

/bot run

@yufeiwu-nv
yufeiwu-nv requested a review from ruodil June 4, 2026 03:38
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51957 [ run ] triggered by Bot. Commit: e6c013d Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51957 [ run ] completed with state FAILURE. Commit: e6c013d
/LLM/main/L0_MergeRequest_PR pipeline #41308 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

@yufeiwu-nv

Copy link
Copy Markdown
Collaborator Author

/bot skip --comment "only test list modify"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51979 [ skip ] triggered by Bot. Commit: b3fa2d4 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #51979 [ skip ] completed with state SUCCESS. Commit: b3fa2d4
Skipping testing for commit b3fa2d4

Link to invocation

@yufeiwu-nv
yufeiwu-nv merged commit 00187c0 into NVIDIA:main Jun 4, 2026
7 checks passed
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.

3 participants