Skip to content

Add recurrent gemma #30143

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
Merged
merged 97 commits into from
Apr 10, 2024
Merged

Add recurrent gemma #30143

merged 97 commits into from
Apr 10, 2024

Conversation

ArthurZucker
Copy link
Collaborator

What does this PR do?

Adds support for recurrent gemma

molbap and others added 30 commits March 4, 2024 15:58
@molbap
Merge pull request #9 from huggingface/update
e536f6a 
Update
@botev
Fork.
c8dfc84
@botev
RecurrentGemma initial commit.
a582187
@botev
Updating __init__.py.
a6ba85f
@botev
Minor modification to how we initialize the cache.
0ef8cca 
Changing how the config specifies the architecture.
@botev
Reformat code to 4 spaces.
102770e 
Fixed a few typos.
@botev
Fixed the forward pass.
67710b0 
Still unclear on the cache?
@botev
Fixed the RecurrentGemmaForCausalLM
6427da1
@botev
Minor comment that we might not need attention_mask and output_attent…
0b57d56 
…ion arguments.
@botev
Now cache should work as well.
4056277
@lberrada
Adding a temporary example to check whether the model generation works.
b3358c9
@botev
Adding the tests and updating imports.
91c64f9
@lberrada
Adding the example file missing in the previous commit.
19a008e
@botev
First working example.
6a78aef
@ArthurZucker
Merge branch 'main' of github.com:huggingface/new-model-addition
ef8c0fb
@Nush395
Removing .gitignore and reverting parts of __init__.
41d21dd
@Nush395
Re-add .gitignore.
74c4b86
@Nush395
Addressing comments for configuration.
0240031
@Nush395
Move mask creation to _prepare_inputs_for_generation.
b8e4de4
@botev
First try at integration tests:
6085aa5 
1. AttributeError: 'GriffinCausalLMOutput' object has no attribute 'attentions'.
2. `cache_position` not passed
@botev
Transfoering between machines.
6d1f486
@botev
Running normal tests.
211f06d
@botev
Minor fix.
e276b26
@botev
More fixes.
3887c14
@botev
Merge pull request #2 from botev/t1
66b3ca6 
Adding tests that work.
@Nush395
Addressing more comments.
b62bb8b
@botev
Minor fixes.
1644ee1
@ArthurZucker
first stab at cleanup
53f61e1
@ArthurZucker
more refactoring
9eb77d8
@ArthurZucker
fix copies and else
94441f0
LysandreJik
LysandreJik approved these changes Apr 10, 2024
View reviewed changes
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Ok great, LGTM!

ArthurZucker and others added 2 commits April 10, 2024 10:13
@ArthurZucker @LysandreJik
Apply suggestions from code review
77963ba 
Co-authored-by: Lysandre Debut <hi@lysand.re>
@ArthurZucker
update
cc80f2d
amyeroberts
amyeroberts reviewed Apr 10, 2024
View reviewed changes
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Very nice 🔥 Thanks for implementing!

Only thing that has to be addressed is the config saving and some outstanding todo comments dotted around

ArthurZucker and others added 17 commits April 10, 2024 12:28
@ArthurZucker @amyeroberts
Update src/transformers/models/recurrent_gemma/convert_recurrent_gemm…
308f3ea 
…a_to_hf.py

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@ArthurZucker @amyeroberts
Update tests/models/recurrent_gemma/test_modeling_recurrent_gemma.py
759bebe 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@ArthurZucker @amyeroberts
Update tests/models/recurrent_gemma/test_modeling_recurrent_gemma.py
4011bb0 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@ArthurZucker @amyeroberts
Update tests/models/recurrent_gemma/test_modeling_recurrent_gemma.py
f153601 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@ArthurZucker @amyeroberts
Update tests/models/recurrent_gemma/test_modeling_recurrent_gemma.py
2505ff8 
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@ArthurZucker
update based on review
a884996
@ArthurZucker
Merge branch 'add-recurrent-gemma' of github.com:huggingface/transfor…
9ad73e6 
…mers into add-recurrent-gemma
@ArthurZucker
doc nit
5a2ea8d
@ArthurZucker
fix quality
c77723e
@ArthurZucker
quality
19121e4
@ArthurZucker
fix slow test model path
c0af888
@ArthurZucker
update default dype
e20fb42
@ArthurZucker
ignore attributes that can be safely ignored in check config attributes
acf2c37
@ArthurZucker
0lallalala come on
542af28
@ArthurZucker
save nit
2d26d92
@ArthurZucker
style
ea19cfb
@ArthurZucker
remove to dict update
ec6e006
@ArthurZucker
Copy link
Collaborator Author

Just making sure the slow generations are correct, and merging!

@ArthurZucker ArthurZucker merged commit 0fe4405 into main Apr 10, 2024
@ArthurZucker ArthurZucker deleted the add-recurrent-gemma branch April 10, 2024 14:59
proger
proger reviewed Apr 10, 2024
View reviewed changes
src/transformers/models/recurrent_gemma/modeling_recurrent_gemma.py
return hidden_states

# TODO refactor
def _rnn_scan(
Copy link

Choose a reason for hiding this comment

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

Hi! I work on https://github.com/proger/accelerated-scan which is a package with CUDA/Triton kernels. It can speed up training and prompt reading for Recurrent Gemma. Would you consider a patch for the model that uses Accelerated Scan?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure but would love to share if you have a repo on the hub that integrates this? 🤗

Copy link

Choose a reason for hiding this comment

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

Thanks for the tip!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@proger proger proger left review comments

@amyeroberts amyeroberts amyeroberts left review comments

@LysandreJik LysandreJik LysandreJik 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.
9 participants
@ArthurZucker @HuggingFaceDocBuilderDev @proger @amyeroberts @LysandreJik @molbap @botev @lberrada @Nush395