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.

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

refactor: change default block_size #26229

Merged
merged 13 commits into from
Oct 4, 2023
Merged

refactor: change default block_size #26229

merged 13 commits into from
Oct 4, 2023

Conversation

pphuc25
Copy link
Contributor

@pphuc25 pphuc25 commented Sep 18, 2023

Hi,
As mentioned in issue #26069, I have created a new PR with the aim of modifying the block_size for all files. The goal is to set the block size = min(1024, config.max_position_embeddings). This change is intended to ensure synchronization and prevent errors that can occur when the block size exceeds the maximum position embeddings value.
I would like to cc @sanchit-gandhi to review my PR, thank you so much

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 18, 2023

however, I have some confuse because why I can not pass the test case

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

Hey! Running make fix-copies should work.
Also if you plan to do this for a lot of models, make sure to groupe in a single PR.

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 19, 2023

thank you @ArthurZucker, but I have some confuse why I just change 1024 to min(1024, config.max_position_embeddings) and docs and then I can not pass the test case, still stuck, can you help me

@sanchit-gandhi
Copy link
Contributor

sanchit-gandhi commented Sep 19, 2023

Hey @pphuc25 - don't worry, your PR is perfect! All you need to do is rebase onto main:

git fetch upstream
git rebase upstream main

And then force push:

git commit -m "rebase" --allow-empty
git push -f origin flax_min_block_size

This should fix your CI issues! The test that's failing isn't related to your PR and was fixed yesterday on main.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 19, 2023

thanks @sanchit-gandhi so much on very helpful information, but this seem to still bug.

@sanchit-gandhi
Copy link
Contributor

Hmm could you try running: make fix-copies and pushing? Does this change the code for you at all?

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 21, 2023

Thank you for really helpful support @sanchit-gandhi, this help me so much and gain me more knowledge, thank you really much.

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

Actually sorry for the past review, I'll comment the same thing about the example folder, which we usually don't maintain. All the improvements that you added are great! But would recommend having your training script shared! This will also bring the spotlight to your work! 🤗

@pphuc25
Copy link
Contributor Author

pphuc25 commented Sep 25, 2023

but @sanchit-gandhi, I think you should review the code to merge PR

@sanchit-gandhi
Copy link
Contributor

Shall we merge this one @ArthurZucker since it protects against an edge case, and then add follow-up features to your own codebase / the HF Hub @pphuc25?

@ArthurZucker
Copy link
Collaborator

Sure!

@sanchit-gandhi sanchit-gandhi merged commit 6015f91 into huggingface:main Oct 4, 2023
@pphuc25 pphuc25 deleted the flax_min_block_size branch October 4, 2023 14:54
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