-
Notifications
You must be signed in to change notification settings - Fork 500
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[Feature] Add Tulu3 SFT Mixture Dataset Support #1381
Conversation
@@ -0,0 +1,37 @@ | |||
model: |
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 config is based on https://github.com/allenai/open-instruct/blob/main/configs/train_configs/sft/mini.yaml
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 for the contribution, bwalshe! Overall things look good. I left a few small style comments.
The location you chose for your config is fine for a demonstration. If you've tuned parameters and get good results, I'd add it to the recipes
section instead.
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.
One more small issue that came up when running pytest.
raise ValueError(f"Unknown role {e.args[0]}, was expecting one of { | ||
roles.keys()}") from e |
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.
raise ValueError(f"Unknown role {e.args[0]}, was expecting one of { | |
roles.keys()}") from e | |
raise ValueError(f"Unknown role {e.args[0]}, was expecting one of " | |
f"{roles.keys()}") from e |
return Message(role=role, content=content) | ||
except KeyError as e: | ||
raise ValueError( | ||
f"Unknown role {message['role']}, " |
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.
Technically message['role']
can throw another exception if 'role' is not in message.
How about pre-computing the value once at the beginning of the function:
role_str = message["role"].lower().strip()
?
Description
This adds support for the Tülu 3 SFT Mixture dataset. (https://huggingface.co/datasets/allenai/tulu-3-sft-mixture)
Currently this PR is a work in progress, as I am unsure what testing process is needed and also what would make a suitable example config for demonstrating the dataset in use. I have included some basic unit tests, and the example config demonstrates the data being used in fine-tuning, but it serves more as a test things are working instead of being a good demonstration of what to do with the data.
Related issues
Fixes #1361
Before submitting
Reviewers
At least one review from a member of
oumi-ai/oumi-staff
is required.