-
Notifications
You must be signed in to change notification settings - Fork 431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove ChatFormat, InstructTemplate, old message converters #1895
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/1895
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 81b6abe with merge base 2c948c6 (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@@ -0,0 +1,52 @@ | |||
.. _packing_usage_label: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you snuck this in here you sneaky lil man
i love it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I just now realized the joke
recipes/generate.py
Outdated
chat_format = _get_component_from_path(chat_format) | ||
messages = chat_format.format(messages) | ||
return self._tokenizer.tokenize_messages(messages)[0] | ||
messages = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noobq: Is this identical to free-form generation when a prompt template isn't provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this assumes instruct-based finetuned models I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big complaints - thx!
|
||
get_sharegpt_messages | ||
get_openai_messages | ||
|
||
.. _message_transforms_ref: | ||
|
||
Message transforms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we call these ToMessage
transforms to convey that they convey immediately that they convert data to message format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used "Message transforms" throughout the docs, so I'll leave updating all those references for a future PR and keep this as is
@@ -0,0 +1,52 @@ | |||
.. _packing_usage_label: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh nice
recipes/configs/generation.yaml
Outdated
@@ -27,11 +27,10 @@ tokenizer: | |||
_component_: torchtune.models.llama2.llama2_tokenizer | |||
path: /tmp/Llama-2-7b-hf/tokenizer.model | |||
max_seq_len: null | |||
prompt_template: null | |||
|
|||
# Generation arguments; defaults taken from gpt-fast | |||
prompt: "Tell me a joke?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should just adopt a format like generation_v2 here where we have the default be to make it clear:
prompt:
user: Tell me a joke.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that works. was just trying to make the minimal changes needed since we are migrating to generate_c2
Context
What is the purpose of this PR? Is it to
Closes #1839. Closes #1849.
Changelog
What are the changes made in this PR?
torchtune/data/_converters.py
. These are replaced by the transforms in_messages.py
Test plan
Ran
tune run generate --config generation
. The output on main is just not good:After changes, it is somewhat more logical, although it still doesn't know how to stop: