Skip to content

Conversation

@gante
Copy link
Member

@gante gante commented Sep 19, 2024

What does this PR do?

test_generate_from_inputs_embeds_decoder_only had low probability of failure... but not zero (see #33593 and its CI runs)

Instead of assuming that calling generate with random inputs_embeds will result in a different text, let's check the scores instead. Even if the output text is the same (unlikely), the token scores are almost guaranteed to be different.

@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.

@gante
Copy link
Member Author

gante commented Sep 19, 2024

hah, a PR to fight flakiness got stuck in another flaky test 🙃

self.assertListEqual(outputs_from_ids.sequences.tolist(), outputs_from_embeds.sequences.tolist())

# But if we pass different inputs_embeds, we should get different outputs
torch.manual_seed(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

very nice to remove torch.manual_seed!

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Great! I am up to merge as it is.

Another flaky test is worth another PR, and let's have fewer failures progressively.

But up to you :-)

@gante
Copy link
Member Author

gante commented Sep 20, 2024

@ydshieh I don't have merge permissions, and I agree with you -- can you merge this PR for me? :)

@ydshieh ydshieh merged commit 266d0a6 into huggingface:main Sep 20, 2024
17 of 19 checks passed
@gante gante deleted the flaky_inputs_embeds branch September 20, 2024 13: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.

3 participants