-
Notifications
You must be signed in to change notification settings - Fork 31.9k
BLOOM minor fixes small test #18175
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
BLOOM minor fixes small test #18175
Conversation
- add correct revision - corrected dosctring for test - removed a test
Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com> Co-authored-by: Nouamane Tazi <nouamane98@gmail.com>
|
The documentation is not available anymore as the PR was closed or merged. |
|
|
||
| path_350m = "bigscience/bloom-350m" | ||
| model = BloomForCausalLM.from_pretrained(path_350m, use_cache=True).cuda() | ||
| model = BloomForCausalLM.from_pretrained(path_350m, use_cache=True, revision="gs555750").cuda() |
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.
What does the revision argument do? Are we sure we want to add that?
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.
yes, this argument loads the model from the initial model we were using when designing those tests: https://huggingface.co/bigscience/bloom-350m/tree/gs555750
@Muennighoff recently wanted to push the newest weights of 350m so just adding the revision flag should prevent us breaking those tests
sgugger
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 for the fixes!
| tokenizer.decode(greedy_output_without_pad[0, :-3], skip_special_tokens=True), | ||
| ) | ||
|
|
||
| @slow |
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 you just explain why this test is removed please?
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.
Following a discussion that we had on Slack, it appears that one should always use padding_side=left
Here is a link that quickly summarizes the issue: #17963 (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 for clarifying!
| tokenizer.decode(greedy_output_without_pad[0, :-3], skip_special_tokens=True), | ||
| ) | ||
|
|
||
| @slow |
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 clarifying!
* minor fixes - add correct revision - corrected dosctring for test - removed a test * contrib credits Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com> Co-authored-by: Nouamane Tazi <nouamane98@gmail.com> Co-authored-by: Yih-Dar <2521628+ydshieh@users.noreply.github.com> Co-authored-by: Nouamane Tazi <nouamane98@gmail.com>
Small modifications
cc @ydshieh @NouamaneTazi @Muennighoff