Skip to content

fix get_keys_to_not_convert function#24095

Merged
SunMarc merged 2 commits intohuggingface:mainfrom
SunMarc:fix_get_keys_to_not_convert
Jun 8, 2023
Merged

fix get_keys_to_not_convert function#24095
SunMarc merged 2 commits intohuggingface:mainfrom
SunMarc:fix_get_keys_to_not_convert

Conversation

@SunMarc
Copy link
Member

@SunMarc SunMarc commented Jun 7, 2023

What does this PR do ?

Fix the behavior of the get_keys_to_not_convert function for the following cases:

  • If the lm_head is tied, we won't be able to see it using the method named_parameters() and the last visible module was added instead -> using named_children() instead
  • Fix tied_params variable that we should not crop.( Example of what was happening : [['lm_head.weight', 'model.decoder.embed_tokens.weight']] -> ['model.decoder.embed_tokens.weight'])

@SunMarc SunMarc requested review from sgugger and younesbelkada June 7, 2023 22:28
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 7, 2023

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

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.

Will defer to @younesbelkada here. Note that the first changes make tied_keys contain the name of all tied parameters (whereas it was ignoring the first key for each group of tied parametrs before), not sure if this is intended or not.

Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Awesome work! Thanks a lot for fixing

@SunMarc SunMarc merged commit 71a114d into huggingface:main Jun 8, 2023
@SunMarc SunMarc deleted the fix_get_keys_to_not_convert branch June 8, 2023 14:14
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
* fix get_keys_to_not_convert funct

* Fix style
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