[https://nvbugs/6507080][fix] Override TokenizerBase.__repr__ to return f"{self.__class__.__name__}()"… - #16839
[https://nvbugs/6507080][fix] Override TokenizerBase.__repr__ to return f"{self.__class__.__name__}()"…#16839trtllm-agent wants to merge 3 commits into
TokenizerBase.__repr__ to return f"{self.__class__.__name__}()"…#16839Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a fallback ChangesTokenizer representation and test enablement
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e0af668 to
30b981f
Compare
30b981f to
ced2a75
Compare
fredricz-20070104
left a comment
fredricz-20070104
left a comment
There was a problem hiding this comment.
Review summary - Approve
Reviewed the full diff; no blocking or major issues found.
Minor, non-blocking notes:
tensorrt_llm/tokenizer/tokenizer.py: repr loses detail for fully-implemented subclasses
Automated review by NVCortex Lite, run by @fredricz-20070104.
|
/bot run |
|
PR_Github #63052 [ run ] triggered by Bot. Commit: |
|
PR_Github #63052 [ run ] completed with state
|
PreTrainedTokenizerBase.__repr__ reads properties (e.g.
added_tokens_decoder) that TokenizerBase subclasses are not required
to implement. When a user-supplied custom tokenizer becomes a field of
the pydantic llm_args model, logging f"LLM Args:\n{llm_args}" in
py_executor_creator triggers pydantic's __repr__, which recursively
calls repr() on the tokenizer and blows up with NotImplementedError.
Override __repr__ on TokenizerBase to return a class-name-only string
so pydantic reprs and startup logging never crash on custom tokenizers.
Signed-off-by: handongl <handongl@nvidia.com>
Signed-off-by: handongl <handongl@nvidia.com>
ced2a75 to
c8722e1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run |
|
PR_Github #64068 [ run ] triggered by Bot. Commit: |
|
PR_Github #64068 [ run ] completed with state
|
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Root cause looks right, and TransformersTokenizer already defines its own __repr__ (tokenizer.py:293) so the default path is unaffected.
Two asks before merge:
- The test is post-merge-only, so the standard pipeline won't re-run it after un-waiving — trigger the post-merge stage that covers
unittest/llmapi/test_llm.py(/bot run --extra-stage "...") and link the result. - Add a one-line assertion that
repr()on a minimalTokenizerBasesubclass doesn't raise. Without it, the only thing guarding this regression is a post-merge integration test.
Signed-off-by: Mike Iovine <miovine@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run |
|
PR_Github #64627 [ run ] triggered by Bot. Commit: |
|
PR_Github #64627 [ run ] completed with state
|
8 participants
Summary
__repr__, which reads the abstractadded_tokens_decoderproperty and raisesNotImplementedErroron user protocol subclassesTokenizerBase.__repr__to returnf"{self.__class__.__name__}()"so pydantic/logging never touches transformers-internal state on custom tokenizersTest plan
Links
Dev Engineer Review
TokenizerBase.__repr__intensorrt_llm/tokenizer/tokenizer.py.f"{self.__class__.__name__}()".PreTrainedTokenizerBase.__repr__from accessing unimplemented tokenizer state, such asadded_tokens_decoder.unittest/llmapi/test_llm.py::test_llm_with_customized_tokenizer.NotImplementedErrorfix.QA Engineer Review
tests/integration/test_lists/waives.txtunittest/llmapi/test_llm.py::test_llm_with_customized_tokenizer.