Skip to content

DOC: Clarify attention_mask usage in BertModel forward method#38877

Open
dhyeyinf wants to merge 3 commits into
huggingface:mainfrom
dhyeyinf:improve-doc-example-bert
Open

DOC: Clarify attention_mask usage in BertModel forward method#38877
dhyeyinf wants to merge 3 commits into
huggingface:mainfrom
dhyeyinf:improve-doc-example-bert

Conversation

@dhyeyinf

Copy link
Copy Markdown

This PR improves the documentation of the attention_mask parameter in the forward() method of the BertModel class.

Changes made:

  • Added a clear explanation of how 1s and 0s in attention_mask control attention behavior
  • Included a minimal example tensor to demonstrate typical usage
  • Follows the Hugging Face docstring style for consistency

The goal is to make the docstring more understandable and helpful for beginners exploring the source code.

Please let me know if you'd like any phrasing or formatting adjusted — happy to revise!

@dhyeyinf

Copy link
Copy Markdown
Author

Hi! This is my first contribution to Hugging Face. I see that one CircleCI check failed (check_repository_consistency). Please let me know if I should rebase or update something — happy to do it. Otherwise, I’ll wait for your feedback. Thank you!

@Rocketknight1

Copy link
Copy Markdown
Member

Hi @dhyeyinf, the CI issues are caused by other models inheriting from BERT's code. You need to run make fix-copies in the transformers dir to propagate the change to them, and then everything should pass.

cc @stevhliu for docstrings!

@stevhliu stevhliu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

I think you should edit the line in the file below because it is a common argument and let @auto_docstring update it for all the other models that use it. cc @yonigozlan, is this the correct way?

Comment on lines +920 to +923
Mask used to avoid performing attention on padding tokens.
Positions with a value of 1 indicate tokens that should be attended to,
while 0 indicates padding tokens which are ignored by the model.
Typically used to differentiate between actual tokens and padding in batched sequences.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Mask used to avoid performing attention on padding tokens.
Positions with a value of 1 indicate tokens that should be attended to,
while 0 indicates padding tokens which are ignored by the model.
Typically used to differentiate between actual tokens and padding in batched sequences.
Mask used to avoid performing attention on padding tokens indices.
Positions with a value of 1 indicates a token should be attended to
and 0 means it should be ignored.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems very similar to what we have already in args_doc, minus the link to the glossary, so not sure we should override the doc for bert here, and even less override the one in args_doc

Comment on lines +925 to +927
Example:
>>> attention_mask = torch.tensor([[1, 1, 1, 0, 0]])
>>> # 1s represent actual tokens, 0s are padding.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think its necessary to include an example here since there's already a link to a more detailed description of it

Suggested change
Example:
>>> attention_mask = torch.tensor([[1, 1, 1, 0, 0]])
>>> # 1s represent actual tokens, 0s are padding.

@dhyeyinf

Copy link
Copy Markdown
Author

Thanks for the review!
I've moved the attention_mask docstring improvement to args_doc.py as suggested, and ran make fix-copies to propagate the change across all relevant models.
Let me know if any other adjustments are needed!

@dhyeyinf

Copy link
Copy Markdown
Author

Hi! I've applied make fix-copies and also ran make fix-all to ensure consistency across all models. If anything else is required from my side to pass the remaining checks, happy to assist. Thanks for reviewing!

@stevhliu

Copy link
Copy Markdown
Member

Thanks for your PR, but since your changes aren't all that different from what we currently have, I don't think we need to update this particular arg. Happy to review other areas where you think it can be improved though! 🙂

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