[None][fix] namespace cnn_dailymail calibration dataset id#16247
[None][fix] namespace cnn_dailymail calibration dataset id#16247lonexreb wants to merge 2 commits into
Conversation
The bare 'cnn_dailymail' repo id was relocated to 'abisee/cnn_dailymail'; newer huggingface_hub rejects un-namespaced ids, so load_calib_dataset failed with HfUriError. Rewrite the bare id before load_dataset. Revives closed PR NVIDIA#16124 (issue NVIDIA#15802). Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthrough
ChangesCalibration Dataset Namespace Handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@tests/unittest/others/test_calib_dataset_namespace.py`:
- Around line 32-38: Add a test case alongside
test_other_dataset_ids_are_passed_through for
load_calib_dataset("ccdv/cnn_dailymail"), mocking convert_utils.load_dataset and
asserting it receives the qualified identifier unchanged; retain the existing
bare-identifier mapping behavior separately.
🪄 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: 6b3bb42b-ac6f-4792-9c49-1ad3a184cae6
📒 Files selected for processing (2)
tensorrt_llm/models/convert_utils.pytests/unittest/others/test_calib_dataset_namespace.py
| def test_other_dataset_ids_are_passed_through() -> None: | ||
| with mock.patch.object( | ||
| convert_utils, "load_dataset", return_value={"text": ["x"]} | ||
| ) as load_dataset: | ||
| load_calib_dataset("lambada") | ||
|
|
||
| assert load_dataset.call_args.args[0] == "lambada" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the qualified CNN/DailyMail identifier as a pass-through case.
lambada verifies generic pass-through behavior, but ccdv/cnn_dailymail is the important boundary case: it must not be rewritten because only the bare identifier should map to abisee/cnn_dailymail.
Proposed test extension
-def test_other_dataset_ids_are_passed_through() -> None:
+@pytest.mark.parametrize("dataset_name", ["lambada", "ccdv/cnn_dailymail"])
+def test_other_dataset_ids_are_passed_through(dataset_name: str) -> None:
...
- load_calib_dataset("lambada")
+ load_calib_dataset(dataset_name)
...
- assert load_dataset.call_args.args[0] == "lambada"
+ assert load_dataset.call_args.args[0] == dataset_nameAs per path instructions, important edge cases should be covered; no QA-list update is needed for this unit-only change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_other_dataset_ids_are_passed_through() -> None: | |
| with mock.patch.object( | |
| convert_utils, "load_dataset", return_value={"text": ["x"]} | |
| ) as load_dataset: | |
| load_calib_dataset("lambada") | |
| assert load_dataset.call_args.args[0] == "lambada" | |
| `@pytest.mark.parametrize`("dataset_name", ["lambada", "ccdv/cnn_dailymail"]) | |
| def test_other_dataset_ids_are_passed_through(dataset_name: str) -> None: | |
| with mock.patch.object( | |
| convert_utils, "load_dataset", return_value={"text": ["x"]} | |
| ) as load_dataset: | |
| load_calib_dataset(dataset_name) | |
| assert load_dataset.call_args.args[0] == dataset_name |
🤖 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 `@tests/unittest/others/test_calib_dataset_namespace.py` around lines 32 - 38,
Add a test case alongside test_other_dataset_ids_are_passed_through for
load_calib_dataset("ccdv/cnn_dailymail"), mocking convert_utils.load_dataset and
asserting it receives the qualified identifier unchanged; retain the existing
bare-identifier mapping behavior separately.
Source: Path instructions
Ensure the already-qualified id is not rewritten (only the bare cnn_dailymail maps to abisee/cnn_dailymail). Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
|
Addressed in ee0ce56: parametrized the pass-through test to also cover |
|
@lonexreb , |
2 participants
Description
Revives the stale, closed-unmerged PR #16124 (issue #15802).
load_calib_dataset("cnn_dailymail")passes the bare repo id todatasets.load_dataset. Newerhuggingface_hubrequires anamespace/nameid and rejects the barecnn_dailymailwithHfUriError: Repository id must be ..., breaking INT8/SmoothQuant calibration.Change
Rewrite the bare
cnn_dailymailid to the relocated canonicalabisee/cnn_dailymailbefore callingload_dataset(intensorrt_llm/models/convert_utils.py). Other dataset ids are untouched.Test
New CPU-only unit test in
tests/unittest/others/test_calib_dataset_namespace.py(mocksload_dataset, no network): the bare id becomesabisee/cnn_dailymail; other ids (e.g.lambada) pass through unchanged.Summary by CodeRabbit
Bug Fixes
Tests