-
Notifications
You must be signed in to change notification settings - Fork 3
Padding free squashing #12
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
Conversation
|
@garrett361 i didnt realize there is a quality check, its failing the check now. otherwise looks quite good to me |
open_instruct/dataset_processor.py
Outdated
| "{% endif %}" | ||
| "{% endfor %}" | ||
| ), | ||
| "granite": ( |
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.
not sure if this is meaningful, since there are going to be many iterations on these templates. I feel its better to allow the template to be a file path, and it gets read in
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.
Divya required this for her work. Easy to drop this later since it's a standalone commit.
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.
could we have a feature that we can set chat_template_name to reference a file, and that it could be read and set here. two options;
- set
chat_template_name="custom"and have a few flagchat_template_custom_fileto point to the new file - overload
chat_template_nameto simply point to a new file.
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, I think I'll do 2: if chat_template_name isn't in CHAT_TEMPLATES, then try to read in the text from a file at path chat_template_name. Kinda hacky, but then we don't have to add more flags.
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.
hmm.. but maybe 1. is better because its more consistent..
|
@fabianlim also didn't realize about the check; guess it's only activated when PR-ing against main. |
eed9785 to
76dde18
Compare
|
Fixed the CI lint failures. |
76dde18 to
2b1cfaf
Compare
|
I think this will be a good to merge soon, although we need to document down what is missing in comparison to padding-free branch
|
71ef31f to
b2fe4b9
Compare
b2fe4b9 to
1ca40dc
Compare
|
this seems to be superceeded by the series of PRs #17 , etc.. so maybe we should close it? |
|
Closing, due to superceeding PRs. |
Squashes various commits and rebases on top of latest upstream main.