Fix extra start token in phi3 tokenizer. Address #1063

[hmosousa]

Context

What is the purpose of this PR? Is it to

• add a new feature
• fix a bug
• update tests and/or documentation
• other (please add here)

Please link to any issues this PR addresses.
Issue #1063

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 not sure about any one of these just ask and we will happily help.)

• run pre-commit hooks and linters (make sure you've first installed via pre-commit install)
• add unit tests for any new functionality
• update docstrings for any new or updated methods or classes
• run unit tests via pytest tests
• run recipe tests via pytest tests -m integration_test
• manually run any new or modified recipes with sufficient proof of correctness
  • include relevant commands and any other artifacts in this summary (pastes of loss curves, eval results, etc.)

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1065

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit a6fd548 with merge base 161ac3d ():

NEW FAILURE - The following job has failed:

• Build Linux Wheels / __default / upload / manywheel-py3_8-cuda12_4 (gh)
  Unable to find an artifact with the name: pytorch_torchtune__3.8_cu124_x86_64

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[joecummings]

Can you include a gist with your test confirming this works?

Thanks!

[hmosousa]

Sure. Gist follows:

from torchtune.data import Message
from torchtune.models.phi3 import phi3_mini_tokenizer

messages = [
    Message(role="system", content="system content"),
    Message(role="user", content="users content")
]
tokenizer = phi3_mini_tokenizer("tmp/tokenizer.model")
input_ids, _ = tokenizer.tokenize_messages(messages, ignore_system_prompts=False)
print(input_ids)  # [1, 32010, 29871, 13, 7193, 2793, 32007, 29871, 13]

[RdoubleA]

Sure. Gist follows:

from torchtune.data import Message
from torchtune.models.phi3 import phi3_mini_tokenizer

messages = [
    Message(role="system", content="system content"),
    Message(role="user", content="users content")
]
tokenizer = phi3_mini_tokenizer("tmp/tokenizer.model")
input_ids, _ = tokenizer.tokenize_messages(messages, ignore_system_prompts=False)
print(input_ids)  # [1, 32010, 29871, 13, 7193, 2793, 32007, 29871, 13]

why not just add this as a quick test :) you can use the sentencepiece test file we use here: https://github.com/pytorch/torchtune/blob/main/tests/torchtune/modules/tokenizers/test_sentencepiece.py

[ebsmothers]

@hmosousa thanks for fixing this bug! I am actually gonna veto @RdoubleA's suggestion.. in this case you already did most of the important work by finding and fixing the bug with a reasonable repro. We should definitely have a unit test for the Phi3 tokenizer, and it should ideally test this behavior. But given no such unit test currently exists, I don't think it should be on you to add it in this PR. (If you want to add one separately we would of course love to have it, but we don't need to block this fix on it, and otherwise we can and should add it ourselves.)

[hmosousa]

Thanks @RdoubleA and @ebsmothers. I might be able to add the tests. Will create a new issue for that.