-
Notifications
You must be signed in to change notification settings - Fork 65
feat: Add multi and single turn chat support #415
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
feat: Add multi and single turn chat support #415
Conversation
|
Thanks for making a pull request! 😃 |
f44b370 to
692f84f
Compare
|
Are we planning to add training test cases for single turn and multi turn as well? |
Yes we are...for single turn and multi turn..working on a couple |
Signed-off-by: Dushyant Behl <[email protected]>
c51491c to
613527d
Compare
|
Added test cases for unit testing e2e using single and multi turn data. The testcases are e2e but a disection of the logs is shown below like above. Single turn Multi turn |
efa6a2c to
ae91828
Compare
Signed-off-by: Dushyant Behl <[email protected]>
Abhishek-TAMU
left a comment
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.
Thanks @dushyantbehl for the unit tests. Have few changes to suggest.
tuning/data/setup_dataprocessor.py
Outdated
| elif data_args.instruction_template and data_args.response_template: | ||
| # Data Format 3: Chat dataset with instruction and response template | ||
| # We don't do processing for chat dataset | ||
| handlers, dataset_text_field = [], None |
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.
In this unit test, data_args.dataset_text_field is not None so it will always satisfy elif of Line 239 instead of this elif. Hence add data_args.dataset_text_field to None in this unit test
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.
Also have a question that if we are giving data_args.dataset_text_field as None to SFTTrainer , then how would SFTTrainer get to know that messages key has to be picked for data as dataset has multiple keys :
'messages': [], 'group': 'lab_extension', 'dataset': 'base/full-extension', 'metadata': '{"num_turns": 2}'}
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.
This is fixed..I noticed that SFTTrainer was not properly applying chat templates so having an explicit handler which applies chat template is an easier option...we can keep the dataset_text_field intact and also have more control if we need to do any handling before or after applying chat template.
82113e4 to
a3f84a4
Compare
|
@dushyantbehl Are we planning to add usage docs to README.md? |
|
Yes @kmehant that will be in the documentation PR as discussed internally on slack |
|
@dushyantbehl Can you also elaborate what |
a3f84a4 to
d9f071b
Compare
@kmehant done |
d9f071b to
5236a6d
Compare
5236a6d to
6b8c155
Compare
Signed-off-by: Dushyant Behl <[email protected]>
6b8c155 to
ba9137e
Compare
kmehant
left a comment
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.
looks good to me.
|
Waiting for approval from @Abhishek-TAMU @ashokponkumar @willmj |
Abhishek-TAMU
left a comment
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.
Thanks @dushyantbehl for the fix and adding handler. LGTM!
willmj
left a comment
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.
LGTM! Thanks Dushyant
Description of the change
Multi turn chat support in SFTTrainer can be used via passing
responseandinstructiontemplate toDataCollatorForCompletionOnlyLMThis PR adds two new arguments to the
dataargsA combination of these along with
response_templateis used to trainsingle or multi turn chat data.chat_templateis supplied its passed to thetokenizerwhen initialised.responseandinstructiontemplates are provided then dataset is assumed to be a chat dataset and chat style collator is used by passing appropriate arguments toDataCollatorForCompletionOnlyLMRelated issue number
How to verify the PR
Was the PR tested
The testing of this PR was done locally as below
For single turn data
For multi turn data