Skip to content

Conversation

@gante
Copy link
Contributor

@gante gante commented Aug 23, 2024

What does this PR do?

#31292 added test_forward_with_num_logits_to_keep. The test needs a higher atol at least for modern CUDA devices, it was failing locally on my end.

[Forward pass in the lm head with different shape due to num_logits_to_keep -> different order of FP operations (maybe different optimal kernels too) -> slightly different output. See this comment for more info.]

FYI @Cyrilvallez

@gante gante requested a review from ArthurZucker August 23, 2024 10:39
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Cyrilvallez
Copy link
Member

Thanks for fixing and pointing that out! I had no idea that input shape only could impact outputs of linear layers so significantly for lower precisions. Very good thing to keep in mind in the future.
Do you know why the test did not fail on the CIs when I committed? I must be missing something on how the tests are automatically run/what tests are actually run

@gante
Copy link
Contributor Author

gante commented Aug 23, 2024

@Cyrilvallez

Do you know why the test did not fail on the CIs when I committed?

Our CI on push runs on a machine without GPUs :D CPU computations tend to be less sensible to these tiny fluctuations

Screenshot 2024-08-23 at 15 21 18

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

thanks for fixing!

@gante gante merged commit 894d421 into huggingface:main Aug 26, 2024
@gante gante deleted the atol_in_num_logits_to_keep_test branch August 26, 2024 14:23
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants