[WIP] HuggingFaceModelTokenizer #2723
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/2723
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
Thanks @krammnic for taking this one on! This will be huge for lowering the barrier to onboard new models. Let's definitely make sure to add unit tests for this one. (You can likely create some dummy tokenizer_config.json files and check them directly into the repo, since they should be pretty small.)
| special_tokens_mapping = {} | ||
| for token in self.special_tokens: | ||
| special_tokens_mapping[token] = self.base_tokenizer.encode(token) | ||
| rendered_template = self.template.render( |
There was a problem hiding this comment.
Wow this actually wound up being quite easy lol
There was a problem hiding this comment.
Unfortunately, tool calling will still be quite tricky
There was a problem hiding this comment.
@krammnic Other than the lack of tool calls in tt Message class, is there any other reasons behind why tool calling will be tricky?
| if content := token_info.get("content"): | ||
| special_tokens.add(content) | ||
|
|
||
| # We sort lexicographically in order to get real tokens after all <|dummy_x|> |
There was a problem hiding this comment.
Sorry I don't fully understand this comment. I assume this is referring to reserved special tokens? If so, why is string sort the thing to use here?
There was a problem hiding this comment.
Probably we can drop it, it might just simplify debugging in case if we face some problems with new configs.
| self.base_tokenizer = HuggingFaceBaseTokenizer( | ||
| tokenizer_json_path=tokenizer_json_path, | ||
| tokenizer_config_json_path=tokenizer_config_json_path, | ||
| generation_config_path=generation_config_path, | ||
| ) |
There was a problem hiding this comment.
I know @joecummings had some thoughts on whether we should use generic base_tokenizer instead of constraining to use HuggingFaceBaseTokenizer. I suspect the latter is better for making sure everything works together, but I know at least Qwen2Tokenizer still relies on the merges + vocab files instead of the tokenizer.json file (I alluded to this at the very bottom of #2706). So we should figure out if this will work for that case
| {"role": m.role, "content": m.content[0]["content"]} for m in messages | ||
| ], | ||
| add_generation_prompt=add_eos, | ||
| **special_tokens_mapping, # We assume that the naming is consitent |
There was a problem hiding this comment.
Yeah I think this should be a reasonable assumption (as long as we are also getting the special_tokens from the same place as the template)
Context
What is the purpose of this PR? Is it to
Please link to any issues this PR addresses.
Changelog
What are the changes made in this PR?
Test plan
Please make sure to do each of the following if applicable to your PR. If you're unsure about any one of these just ask and we will happily help. We also have a contributing page for some guidance on contributing.
pre-commit install)pytest testspytest tests -m integration_testUX
If your function changed a public API, please add a dummy example of what the user experience will look like when calling it.
Here is a docstring example
and a tutorial example
Basically, this is a first pass (I'm thinking about how to add masking), but jinja render works surprisingly well.