Simplify TokenizerArgs.__post_init__ with Enum Tokenizer Type
#1535
Conversation
Summary: Simplify `TokenizerArgs.__post_init__` with enum tokenizer type, since only one of the tokenizer type can be true. We want to touch as less code outside of `__post_init__` as possible at the moment. Test Plan: python torchchat.py generate llama2|llama3|granite-code Reviewers: @Jack-Khuu Subscribers: Issue: #1518
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchchat/1535
Note: Links to docs will display an error until the docs builds have been completed. ⏳ No Failures, 23 PendingAs of commit 03e2019 with merge base 5f8f35d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
torchchat/cli/builder.py
Outdated
| self.tokenizer_type = TokenizerType.NONE | ||
| self.t = None |
There was a problem hiding this comment.
Do we really have to set these as none again since we already set them at the very top.
There was a problem hiding this comment.
We can actually drop all the logic here after the HF tokenizer check, tokenizer_type and .t are already set to these by default
torchchat/cli/builder.py
Outdated
| is_sentencepiece = self.is_sentencepiece() | ||
| is_hf_tokenizer = self.is_hf_tokenizer() | ||
|
|
||
| if sum([is_tiktoken, is_hf_tokenizer, is_sentencepiece]) != 1: |
There was a problem hiding this comment.
We can replace this by just checking if the tokenizer enum is None
torchchat/cli/builder.py
Outdated
| self.tokenizer_type = TokenizerType.NONE | ||
| self.t = None |
There was a problem hiding this comment.
We can actually drop all the logic here after the HF tokenizer check, tokenizer_type and .t are already set to these by default
Summary: Simplify `TokenizerArgs.__post_init__` with enum tokenizer type, since only one of the tokenizer type can be true. We want to touch as less code outside of `__post_init__` as possible at the moment. Test Plan: python torchchat.py generate llama2|llama3|granite-code Reviewers: @Jack-Khuu Subscribers: Issue: #1518
torchchat/cli/builder.py
Outdated
| self.is_tiktoken = False | ||
| self.is_sentencepiece = False | ||
| self.is_hf_tokenizer = False | ||
| self.t = None | ||
| return |
There was a problem hiding this comment.
nit: return is not needed
torchchat/cli/builder.py
Outdated
| @@ -294,12 +304,13 @@ def validate_model( | |||
| if model is None: | |||
| return | |||
|
|
|||
| if sum([self.is_tiktoken, self.is_hf_tokenizer, self.is_sentencepiece]) != 1: | |||
| if self.is_tokenizer_none(): | |||
There was a problem hiding this comment.
Let's keep it simple and check for self.tokenizer_type == TokenizerType.NONE here
We can skip the the assert/helper
Summary:
Simplify
TokenizerArgs.__post_init__with enum tokenizer type, since only one of the tokenizer type can be true.We want to touch as less code outside of
__post_init__as possible at the moment.Test Plan:
python torchchat.py generate llama2|llama3|granite-code
Reviewers:
@Jack-Khuu
Subscribers:
Issue:
#1518