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

[bnb] Small improvements on utils #18646

Merged
merged 11 commits into from
Sep 15, 2022
Merged

[bnb] Small improvements on utils #18646

merged 11 commits into from
Sep 15, 2022

Conversation

younesbelkada
Copy link
Contributor

What does this PR do?

Fixes a small typo in bitsandbytes.py, should address huggingface/blog#463 (comment)
I will have to test it first and mark it as ready for review!

@younesbelkada
Small replacement
ea155ed 
- replace `modules_to_not_convert` by `module_to_not_convert`
@younesbelkada younesbelkada changed the title Small replacement [bnb] Fixes a small typo on utils Aug 16, 2022
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 16, 2022 •
edited
Loading

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

@younesbelkada
Copy link
Contributor Author

Can confirm the tests pass!

@younesbelkada younesbelkada marked this pull request as ready for review August 16, 2022 13:40
@younesbelkada younesbelkada requested a review from stas00 August 16, 2022 13:40
@stas00
Copy link
Contributor

stas00 commented Aug 16, 2022

so will there always be just one module not to convert?

won't it be safer to have modules instead and work with the list?

@younesbelkada
refactor a bit
a7731f7 
- changed variables name
- now output a list
- change error message
@younesbelkada
make style
bf59f9f
@younesbelkada
Copy link
Contributor Author

younesbelkada commented Aug 16, 2022 •
edited
Loading

I have proposed a small refactoring that includes:

The bnb slow tests are passing with this fix!

@younesbelkada younesbelkada changed the title [bnb] Fixes a small typo on utils [bnb] Small improvements on utils Aug 17, 2022
@younesbelkada younesbelkada mentioned this pull request Aug 17, 2022
Closed
@younesbelkada
Copy link
Contributor Author

From #18660 I also just added a commit to support having a custom list of the keys to ignore

stas00
stas00 suggested changes Aug 17, 2022
View reviewed changes
src/transformers/modeling_utils.py Outdated Show resolved Hide resolved
src/transformers/modeling_utils.py Outdated Show resolved Hide resolved
src/transformers/modeling_utils.py Outdated Show resolved Hide resolved
@younesbelkada
Copy link
Contributor Author

younesbelkada commented Aug 17, 2022 •
edited
Loading

Thanks a lot @stas00 !
There is no rush at all for this PR, we can definitely wait for @sgugger before moving forward with it

sgugger
sgugger approved these changes Aug 31, 2022
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 working on this, I left some comments.

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved
src/transformers/modeling_utils.py Outdated
@@ -1839,6 +1842,7 @@ def from_pretrained(cls, pretrained_model_name_or_path: Optional[Union[str, os.P
offload_state_dict = kwargs.pop("offload_state_dict", False)
load_in_8bit = kwargs.pop("load_in_8bit", False)
int8_threshold = kwargs.pop("int8_threshold", 6.0)
no_load_in_8bit_modules = kwargs.pop("no_load_in_8bit_modules", None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make more sense to have this be a class variable of PreTrainedModel (like the no_split variable used for big model inference)? I'm afraid the user won't know what to set this too and it looks like it's something we should automatically handle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a strong opinion on that but this argument is optional because the function get_keys_not_to_convert should automatically take care of that except for some models like Jukebox where it is a bit trickier due to its architecture.
In this case the user will just have to manually set which modules should be kept in their native precision and specify them in the kwargs, so I feel like it is a bit easier than having it as an argument of PretrainedModel because you would need to open a PR to add the feature.

younesbelkada and others added 4 commits September 12, 2022 09:21
@younesbelkada
Merge branch 'main' into bnb-fix-change-arg-name
a84aaa7
@younesbelkada @stas00
change args name
27b0ef0 
Co-authored-by: stas00 <stas00@users.noreply.github.com>
@younesbelkada
fix comment
224b504
@younesbelkada @stas00
fix typo
01a4c0c 
Co-authored-by: stas00 <stas00@users.noreply.github.com>
@younesbelkada
Copy link
Contributor Author

younesbelkada commented Sep 12, 2022 •
edited
Loading

Can confirm the bnb slow tests are passing with the proposed fixes! Would love to have a final round of review 💪
cc @sgugger @stas00

sgugger
sgugger approved these changes Sep 12, 2022
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.

Still good for me. I'll let @stas00 have a second look since merging is blocked by his change request.

src/transformers/modeling_utils.py Outdated Show resolved Hide resolved
@younesbelkada @sgugger
Update src/transformers/modeling_utils.py
23fe74a 
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
stas00
stas00 approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

thank you for addressing the suggestions, @younesbelkada

@younesbelkada
Copy link
Contributor Author

Can confirm the slow tests pass after rebasing on main, will merge once it's green! 🟢

@younesbelkada younesbelkada merged commit 7743cac into huggingface:main Sep 15, 2022
oneraghavan pushed a commit to oneraghavan/transformers that referenced this pull request Sep 26, 2022
@younesbelkada @stas00
@sgugger @oneraghavan
[bnb] Small improvements on utils (huggingface#18646)
37fe083 
* Small replacement

- replace `modules_to_not_convert` by `module_to_not_convert`

* refactor a bit

- changed variables name
- now output a list
- change error message

* make style

* add list

* make style

* change args name

Co-authored-by: stas00 <stas00@users.noreply.github.com>

* fix comment

* fix typo

Co-authored-by: stas00 <stas00@users.noreply.github.com>

* Update src/transformers/modeling_utils.py

Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

Co-authored-by: stas00 <stas00@users.noreply.github.com>
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
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

@stas00 stas00 stas00 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.
4 participants
@younesbelkada @HuggingFaceDocBuilderDev @stas00 @sgugger