[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 2 commits into
TokenizerBase.__repr__ to return f"{self.__class__.__name__}()"…#16839Conversation
|
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 | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
e0af668 to
30b981f
Compare
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>
30b981f to
ced2a75
Compare
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
|
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
TokenizerBaseintensorrt_llm/tokenizer/tokenizer.pyto override__repr__, returningf"{self.__class__.__name__}()".PreTrainedTokenizerBase.__repr__accessing tokenizer internals (e.g.,added_tokens_decoder) that customTokenizerBasesubclasses may not implement, preventingNotImplementedErrorwhen pydantic/logging triggersrepr().QA Engineer Review
tests/integration/test_lists/waives.txtunittest/llmapi/test_llm.py::test_llm_with_customized_tokenizer(no entries added).