Skip to content

Mamba & RecurrentGemma: enable strict signature#31549

Merged
gante merged 3 commits intohuggingface:mainfrom
gante:mamba_strict_signature
Jul 8, 2024
Merged

Mamba & RecurrentGemma: enable strict signature#31549
gante merged 3 commits intohuggingface:mainfrom
gante:mamba_strict_signature

Conversation

@gante
Copy link
Copy Markdown
Contributor

@gante gante commented Jun 22, 2024

What does this PR do?

Fixes #31540

Mamba accepts **kwargs, and thus attention_mask can be passed. Many users thus assume it behaves just like other models and can support left-padding.

RecurrentGemma also accept **kwargs, but simply not to crash generate.

This PR enables a strict signature on Mamba and RecurrentGemma.

@gante gante requested a review from ArthurZucker June 22, 2024 11:27
use_cache: Optional[bool] = None,
output_hidden_states: Optional[bool] = None,
return_dict: Optional[bool] = None,
**kwargs, # `attention_mask` is passed by the tokenizer and we don't want it
Copy link
Copy Markdown
Contributor Author

@gante gante Jun 22, 2024

Choose a reason for hiding this comment

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

alternatively, we can accept attention_mask and raise an exception when it is not None or not all ones

@gante gante changed the title Mamba: enable strict signature Mamba & RecurrentGemma: enable strict signature Jun 22, 2024
@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

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.

Copy link
Copy Markdown
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.

Let's googoogogogogo 🚀

Comment on lines +2646 to +2647
model_inputs.update({"output_attentions": output_attentions} if output_attentions else {})
model_inputs.update({"output_hidden_states": output_hidden_states} if output_hidden_states else {})
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yesssss I think I have a PR open where I dod this! Finally!

use_cache: Optional[bool] = None,
output_hidden_states: Optional[bool] = None,
return_dict: Optional[bool] = None,
**kwargs, # `attention_mask` is passed by the tokenizer and we don't want it
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.

Removing this will break FDSP :( See #31161

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@amyeroberts I had a look and it should be fine: this PR removes **kwargs from the model class (e.g. MambaModel), while the FSDP PR ensures there are **kwargs in the decoder layers (e.g. FalconDecoderLayer).

We can see on main that the model themselves don't have **kwargs, even after the FSDP fix (e.g. llama) 🤗

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.

OK!

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.

Batch Generation giving different output when using batch size > 1 or when using padding in MambaForCausalLM

4 participants