Simplify TokenizerArgs __post_init__: Unnecessarily verbose

[Jack-Khuu]

🚀 The feature, motivation and pitch

TokenizerArgs.__post_init__ has grown quite verbose/redundant and could use a bit of simplification

torchchat/torchchat/cli/builder.py

Lines 244 to 289 in 1384f7d

	class TokenizerArgs:
	tokenizer_path: Optional[Union[Path, str]] = None
	is_sentencepiece: bool = False
	is_tiktoken: bool = False
	is_hf_tokenizer: bool = False
	t: Optional[Any] = None
	def __post_init__(self):
	try:
	from tokenizer.tiktoken import Tokenizer as TiktokenTokenizer
	self.t = TiktokenTokenizer(model_path=str(self.tokenizer_path))
	self.is_tiktoken = True
	self.is_sentencepiece = False
	self.is_hf_tokenizer = False
	return
	except:
	pass
	try:
	from sentencepiece import SentencePieceProcessor
	self.t = SentencePieceProcessor(model_file=str(self.tokenizer_path))
	self.is_tiktoken = False
	self.is_sentencepiece = True
	self.is_hf_tokenizer = False
	return
	except:
	pass
	try:
	from tokenizer.hf_tokenizer import HFTokenizer
	self.t = HFTokenizer(str(self.tokenizer_path))
	self.is_tiktoken = False
	self.is_sentencepiece = False
	self.is_hf_tokenizer = True
	return
	except:
	pass
	self.is_tiktoken = False
	self.is_sentencepiece = False
	self.is_hf_tokenizer = False
	self.t = None
	return

Task: Simplify the logic in post_init to reduce redundancy

To test, run a model with each tokenizer type:

• python torchchat.py generate llama2
• python torchchat.py generate llama3
• python torchchat.py generate granite-code

Alternatives

No response

Additional context

No response

RFC (Optional)

No response

[mu-lippy]

I'd like to work on this, please.

[Jack-Khuu]

Give it a shot 😃

Also if you haven't already, definitely join the discord https://discord.com/invite/hm2Keduk3v

[mu-lippy]

I'm sorry if this question is silly, but do I need to write tests after I make changes? Thanks!

[Jack-Khuu]

Usually yes, but in this case the integration tests should catch it so you should be good without (bad tokenizer casting fails fast)

[mu-lippy]

I think dealing with the different parameter naming conventions for each tokenizer makes this hard to deal with. When I try to make it more concise, it looks a lot uglier and harder to read. I may be wrong, but it might be better to leave this method as is?
If not, I'd love to see a more experienced coder's solution.

[Jack-Khuu]

I can give a hint:

• For this task we don't need to dive deep into the tokenizers themselves
• At most one of these fields are set to true: {self.is_tiktoken, self.is_sentencepiece, self.is_hf_tokenizer } . Let's replace this arg with an enum to make it more legible

[mu-lippy]

I think this might be beyond me - it looks like I will have to make changes to other parts of this file besides the TokenizerArgs class, and I'm not confident I can do this without breaking it. I'm sorry

[Jack-Khuu]

No worries, I'll put this back up for grabs

If you're still interested, feel free to put up an initial attempt and we can work through it :)