Skip to content

BLOOM - modifying slow tests #17963

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.

Sign up for GitHub

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

Jump to bottom
Closed
younesbelkada wants to merge 3 commits into from
Closed

BLOOM - modifying slow tests #17963

younesbelkada wants to merge 3 commits into from

Conversation

@younesbelkada
Copy link
Contributor

@younesbelkada younesbelkada commented Jun 30, 2022

What does this PR do?

  • changed the non passing tests to fp32
  • reduced sequence length
  • remove padding test

All these matters have been discussed on Slack but mainly:

1- Generations tests were not passing because the linear layers does not give the same results between torch 1.11 and torch 1.12
2- batched generation can be flaky sometimes in half precision mode, this should be expected. Therefore we reduce the sequence length of the generated output
3- One should always use padding_side=left when doing batched generations

cc @ydshieh @patrickvonplaten

@younesbelkada
modifying tests
564314d 
- changed to fp32
- reduced sequence length
- remove padding test
@younesbelkada
make quality
6982516
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 30, 2022

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@younesbelkada younesbelkada changed the title modifying tests BLOOM - modifying slow tests Jun 30, 2022
@younesbelkada younesbelkada marked this pull request as ready for review June 30, 2022 15:41
@ydshieh
Copy link
Collaborator

ydshieh commented Jun 30, 2022

Hi, @younesbelkada

Could you explain a bit more on One should always use padding_side=left when doing batched generations?
And what are examples of test failures when using padding=right ? I couldn't find you mentioning this on Slack.

Thanks!

@younesbelkada
Copy link
Contributor Author

younesbelkada commented Jul 4, 2022

Hi @ydshieh !
From the internal discussions here is a summary of why one should always use padding_side=left (cc @patrickvonplaten ):

  • Imagine: ["hello my name is", "hey <pad> <pad> <pad>"]
    For the first input the correct token will be sampled from "is" - however for the second input, generate would incorrectly sample from "<pad>" where as it should sample from "hey". Making sure everything is batched on the left circumvents this problem !

@younesbelkada younesbelkada requested a review from sgugger July 4, 2022 09:10
@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Jul 4, 2022

@younesbelkada - IMO we should not expect the generation to be flaky ever, why is this the case here?

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 5, 2022 •
edited
Loading

Hi @younesbelkada : I have 3 questions 🙏

  • with fp16:
    • do we get stable results in a specific torch version (i.e. the same result across many runs)
  • after changing to fp32 (without reducing the seq length)
    • do we get the same results across torch 1.11 and 1.12?
    • do we get stable results in a specific torch version (i.e. the same result across many runs)

@younesbelkada
Copy link
Contributor Author

younesbelkada commented Jul 13, 2022

Hi @ydshieh !
After merging this PR: #17866 the slow tests are now passing. Our conclusion is that:
1- In half precision mode we might not get the same results across batched generation and it should be expected
2- This behavior is observed ONLY on small models !

@younesbelkada younesbelkada mentioned this pull request Jul 18, 2022
Merged
patrickvonplaten
patrickvonplaten reviewed Jul 27, 2022
View reviewed changes
tests/models/bloom/test_modeling_bloom.py
path_350m = "bigscience/bloom-350m"
model = BloomForCausalLM.from_pretrained(path_350m, torch_dtype="auto", use_cache=True).cuda()
model = BloomForCausalLM.from_pretrained(
path_350m, torch_dtype=torch.float32, use_cache=False, revision="gs555750"
Copy link
Contributor

@patrickvonplaten patrickvonplaten Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the most up-to-date model here?

tests/models/bloom/test_modeling_bloom.py
)

@slow
def test_right_left_batched_input(self):
Copy link
Contributor

@patrickvonplaten patrickvonplaten Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we delete this?

Copy link
Contributor Author

@younesbelkada younesbelkada Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we always use padding_side=left for batched generation for autoregressive models so for me there is no point keeping this test

tests/models/bloom/test_modeling_bloom.py
path_350m = "bigscience/bloom-350m"
model = BloomForCausalLM.from_pretrained(path_350m, torch_dtype="auto", use_cache=True).cuda()
model = BloomForCausalLM.from_pretrained(
path_350m, torch_dtype="auto", use_cache=True, revision="gs555750"
Copy link
Contributor

@patrickvonplaten patrickvonplaten Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we add a revision here?

Copy link
Contributor Author

@younesbelkada younesbelkada Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we don't need it there as well you are right

@patrickvonplaten
Copy link
Contributor

patrickvonplaten commented Jul 27, 2022

I'm still a bit confused by this PR - generations are not flaky for pretrained models normally and it's a bit weird to me that all this test does is modifying slow generation tests

@younesbelkada
Copy link
Contributor Author

younesbelkada commented Aug 16, 2022

Closing as it has been fixed by #18344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sgugger sgugger Awaiting requested review from sgugger

1 more reviewer

@patrickvonplaten patrickvonplaten patrickvonplaten left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@younesbelkada @HuggingFaceDocBuilderDev @ydshieh @patrickvonplaten