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

fix retie_parameters #2137

Merged
merged 1 commit into from
Nov 10, 2023
Merged

fix retie_parameters #2137

merged 1 commit into from
Nov 10, 2023

Conversation

SunMarc
Copy link
Member

@SunMarc SunMarc commented Nov 9, 2023

What does this PR do ?

This PR fixes the retie_parameters function. The issue is that we were making the assumption that the first elem of the tied_params was the param to tie. This should solves the BNB tests that are failing on the CI because we switched to safetensors.

@SunMarc SunMarc requested a review from muellerzr November 9, 2023 17:54
@SunMarc SunMarc changed the title fix retie fix retie_parameters Nov 9, 2023
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 9, 2023

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

Copy link
Collaborator

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

A question: Something that can happen now is that no parameters are being tied, right? (when param_to_tie is None after finishing the first loop.) Can this be problematic?

Also, just a tip, code like this can be simplified with the attrgetter function from the operator module. E.g. in the first loop:

        for param_name in tied_group:
            param = attrgetter(param_name)(module)
            if param_to_tie is ...

should work.

In the second loop, a bit of extra work is required:

            for param_name in tied_group:
                mod, _, param = param_name.rpartition(".")
                module = attrgetter(mod)(model)
                setattr(module, param, param_to_tie)

No need to change here, just something I find neat.

@SunMarc
Copy link
Member Author

SunMarc commented Nov 10, 2023

A question: Something that can happen now is that no parameters are being tied, right? (when param_to_tie is None after finishing the first loop.) Can this be problematic?

If param_to_tie is None, it means that all the tensors are on the meta device and they should also have the same shape/dtype since they were tied before (here it is a retie function). So I think it should be fine.

Also, just a tip, code like this can be simplified with the attrgetter function from the operator module. E.g. in the first loop:

Indeed! Thx for sharing, I will use that more often from now =)

@SunMarc SunMarc merged commit 8256a9c into huggingface:main Nov 10, 2023
24 checks passed
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