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

Improve gguf tensor processing #34515

Merged
merged 7 commits into from
Nov 21, 2024

Conversation

VladOS95-cyber
Copy link
Contributor

What does this PR do?

Adding more and more architectures with custom weights conversion is making tensor processing system bigger, less flexible and readable.
Improve GGUF tensor processing system in modeling_gguf_pytorch_utils.py by adding TensorProcessor class for general cases without custom weights changes and all necessary separated tensor processor classes based on architecture with custom process logic.

Before submitting

Who can review?

Regarding the task @SunMarc @LysandreJik @ArthurZucker .

@VladOS95-cyber
Copy link
Contributor Author

Hey @SunMarc! I made some refactoring changes regarding tensor GGUF processing. Please, take a look. What do you think?

@VladOS95-cyber VladOS95-cyber force-pushed the improve-gguf-loading-system branch 3 times, most recently from 4b412ab to a090a84 Compare November 2, 2024 09:32
@VladOS95-cyber
Copy link
Contributor Author

Hey @SunMarc! Just a kind ping, does it make sense to have these changes?

Copy link
Member

@SunMarc SunMarc 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 making tensor processing cleaner. I think it makes sense to do that as it will be easier to read the code. Left a few minor comments. Can you have a second look @Isotr0py ?

src/transformers/modeling_gguf_pytorch_utils.py Outdated Show resolved Hide resolved
src/transformers/modeling_gguf_pytorch_utils.py Outdated Show resolved Hide resolved
@VladOS95-cyber VladOS95-cyber force-pushed the improve-gguf-loading-system branch from a090a84 to ddcea0f Compare November 6, 2024 16:21
@VladOS95-cyber
Copy link
Contributor Author

Hey @SunMarc , @Isotr0py! I made some refactoring based on your comments, please, take a look again.

@VladOS95-cyber VladOS95-cyber force-pushed the improve-gguf-loading-system branch from 8876cc6 to 98810a3 Compare November 7, 2024 15:42
Copy link
Contributor

@Isotr0py Isotr0py left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Thanks for improving this!

@VladOS95-cyber VladOS95-cyber force-pushed the improve-gguf-loading-system branch 2 times, most recently from 0060aac to 254b073 Compare November 11, 2024 10:31
@VladOS95-cyber
Copy link
Contributor Author

Hey @SunMarc! Do you have any further comments or it looks good to you?

@VladOS95-cyber
Copy link
Contributor Author

Hey @SunMarc! Just a kind reminder in order to not loose this PR.

@VladOS95-cyber VladOS95-cyber force-pushed the improve-gguf-loading-system branch from 254b073 to 4153231 Compare November 18, 2024 08:13
@VladOS95-cyber VladOS95-cyber force-pushed the improve-gguf-loading-system branch from 4153231 to 386d4d8 Compare November 18, 2024 12:05
Copy link
Member

@SunMarc SunMarc 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 the PR @VladOS95-cyber as always ! We were away for a week, sorry for the delay. LGTM, just a nit

Comment on lines +394 to +395
if name is None:
continue
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment above to say that you modified name to None in order to skip the processing done below as we already did it above with processor.process. Or we can refactor a bit to also include what we have below to be included in processor.process

@SunMarc SunMarc requested a review from LysandreJik November 18, 2024 14:12
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.

I very much like this split! Thanks @VladOS95-cyber!

@HuggingFaceDocBuilderDev

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.

@LysandreJik
Copy link
Member

Feel free to merge when happy @SunMarc

@SunMarc SunMarc merged commit ae5cbf8 into huggingface:main Nov 21, 2024
24 checks passed
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* add tensor processing system to separate logic for models

* format refactoring

* small fix

* make some methods private

* move custom methods to processors

* refactor tensor processing

* format fix
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
* add tensor processing system to separate logic for models

* format refactoring

* small fix

* make some methods private

* move custom methods to processors

* refactor tensor processing

* format fix
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.

5 participants