Skip to content
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

Show a warning for missing attention masks when pad_token_id is not None #24510

Merged
merged 6 commits into from
Jun 30, 2023
Merged

Show a warning for missing attention masks when pad_token_id is not None #24510

merged 6 commits into from
Jun 30, 2023

Conversation

hackyon
Copy link
Contributor

@hackyon hackyon commented Jun 27, 2023

What does this PR do?

Fixes #16136

Shows a one-time warning message when the pad_token_id is not None and no attention masks are given.

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@gante @ydshieh

@hackyon hackyon force-pushed the warning_attention_mask branch 2 times, most recently from 156a9a6 to d007683 Compare June 27, 2023 01:47
@hackyon
Copy link
Contributor Author

hackyon commented Jun 27, 2023

@ydshieh @gante

I've added the warning message as per #21916 and also a short test. Please let me know if you have any suggestions. If it looks good, I can copy/paste the warning to more models as well. Thanks!

@hackyon hackyon force-pushed the warning_attention_mask branch 2 times, most recently from 6492be3 to e286374 Compare June 27, 2023 03:37
@gante
Copy link
Member

gante commented Jun 27, 2023

@hackyon LGTM 👍

But before you spend time propagating the change to the other models, let's also get a green light from a core maintainer (cc @sgugger )

sgugger
sgugger reviewed Jun 27, 2023
View reviewed changes
Copy link
Collaborator

@sgugger sgugger 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 your PR but I'm not in favor of this. There are a lot of false positives this is going to generate: an attention mask should be passed if there are any tokens in the inputs that are the pad token. It's not necessary to pass one just because the model has a pad token ID.

@hackyon
Copy link
Contributor Author

hackyon commented Jun 27, 2023

Thanks for the review.

Would it be better to check for the presence of the pad_token_id inside input_ids first before throwing the error, as per
https://github.com/huggingface/transformers/pull/17444/files? If so, I can make the change to reflect that here.

@hackyon
Copy link
Contributor Author

hackyon commented Jun 28, 2023

@sgugger

Thanks for the input. I changed my pull request up to be more like #17444. Let me know what you think. Thanks!

@hackyon hackyon force-pushed the warning_attention_mask branch from 2afcd03 to 6993429 Compare June 28, 2023 09:59
sgugger
sgugger reviewed Jun 28, 2023
View reviewed changes
Copy link
Collaborator

@sgugger sgugger 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 iterating, I think it's better this way. Let's jsut reuse the warning_once API we have.

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved
src/transformers/modeling_utils.py Outdated Show resolved Hide resolved
@hackyon
Copy link
Contributor Author

hackyon commented Jun 28, 2023

Thanks, I've updated the code accordingly.

JB Lau and others added 6 commits June 30, 2023 18:37
@hackyon
Adding warning messages to BERT for missing attention masks
4a23258 
These warning messages when there are pad tokens within the input ids and
no attention masks are given. The warning message should only show up once.
@hackyon
Adding warning messages to BERT for missing attention masks
24b70f5 
These warning messages are shown when the pad_token_id is not None
and no attention masks are given. The warning message should only
show up once.
@hackyon
Ran fix copies to copy over the changes to some of the other models
ef86660
@hackyon
Add logger.warning_once.cache_clear() to the test
92e45a7
@hackyon
Shows warning when there are no attention masks and input_ids start/e…
c919542 
…nd with pad tokens
@hackyon
Using warning_once() instead and fix indexing in input_ids check
da850a1
@hackyon hackyon force-pushed the warning_attention_mask branch from 2bdaa09 to da850a1 Compare June 30, 2023 10:41
@sgugger
Copy link
Collaborator

sgugger commented Jun 30, 2023

@gante could you have a second look here?

gante
gante approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

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

This is a nice comprehensive warning, I like it! 🔥

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 30, 2023 •
edited
Loading

The documentation is not available anymore as the PR was closed or merged.

@sgugger sgugger merged commit 78a2b19 into huggingface:main Jun 30, 2023
@gante
Copy link
Member

gante commented Jun 30, 2023

@hackyon Thank you for the contribution! Would you like to add it to the remaining models? 🤗

@hackyon
Copy link
Contributor Author

hackyon commented Jun 30, 2023

Sure, I'll look into it 👍

@ydshieh ydshieh mentioned this pull request Jul 7, 2023
Merged
@hackyon hackyon deleted the warning_attention_mask branch August 3, 2023 15:23
@hackyon
Copy link
Contributor Author

hackyon commented Aug 4, 2023

Thanks @ydshieh for fixing the flaky test!

I was busy in July, but will now add the warning to more models over the next couple of days.

@hackyon hackyon mentioned this pull request Aug 7, 2023
Merged
5 tasks
@chunyuan-w chunyuan-w mentioned this pull request Aug 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgugger sgugger sgugger approved these changes

@gante gante gante approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add warning message if model uses input_ids that include padding tokens, but no attention_mask is provided.
4 participants
@hackyon @gante @sgugger @HuggingFaceDocBuilderDev