Skip to content

Conversation

@stas00
Copy link
Contributor

@stas00 stas00 commented Nov 23, 2020

This PR addresses #7258

(the proposal has evolved a bit since the initial PR, this comments reflects the current state)

  • renames optional model attributes:
  - authorized_missing_keys    => _keys_to_ignore_on_load_missing
  - authorized_unexpected_keys => _keys_to_ignore_on_load_unexpected
  - keys_to_never_save         => _keys_to_ignore_on_save

to (1) make them consistent (2) make them private

  • removes these from public API docstring (documents them privately as comments in place)

This is a breaking change.

Fixes #7258

@LysandreJik, @sgugger

p.s. if we want to postpone it for v5, this PR was a quick one liner:

find . -type d -name ".git" -prune -o -type f -exec perl -pi -e 's|authorized_missing_keys|_keys_to_ignore_on_load_missing|g; s|authorized_unexpected_keys|_keys_to_ignore_on_load_unexpected|g'; s|keys_to_never_save|_keys_to_ignore_on_save|g' {} \;

and then manually adjusting the docs.

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.

Way cleaner, thanks! Just some doc nits on the side.

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.

This is great, thanks for being so fast @stas00!

@stas00
Copy link
Contributor Author

stas00 commented Nov 23, 2020

oh, boy, there is also authorized_unexpected_keys

- **authorized_missing_keys** (:obj:`List[str]`, `optional`) -- A list of re pattern of tensor names to ignore
from the model when loading the model weights (and avoid unnecessary warnings).
- **authorized_unexpected_keys** (:obj:`List[str]`, `optional`) -- A list of re pattern of tensor names to
ignore from the weights when loading the model weights (and avoid unnecessary warnings).
"""
config_class = None
base_model_prefix = ""
authorized_missing_keys = None
authorized_unexpected_keys = None

@LysandreJik
Copy link
Member

Indeed, very nice catch! How should we rename that one?

@stas00
Copy link
Contributor Author

stas00 commented Nov 23, 2020

Current authorized_missing_keys and authorized_unexpected_keys do the same thing overall, just 2 different categories.

perhaps?

  - authorized_missing_keys    => _keys_to_ignore_on_load_missing
  - authorized_unexpected_keys => _keys_to_ignore_on_load_unexpected
  - keys_to_never_save         => _keys_to_ignore_on_save

@sgugger
Copy link
Collaborator

sgugger commented Nov 23, 2020

For me authorized_unexpected_keys should be the _keys_to_ignore_on_load: they are in the state dict but we ignore them.
The authorized_missing_keys should have another name such as _keys_missing_to_ignore_on_load or just _keys_missing_to_ignore.

Re- documentation. We usually document private stuff in comments in the code, so I think we should remove the public documentation and change it in comments.

@sgugger
Copy link
Collaborator

sgugger commented Nov 23, 2020

We were writing at the same time @stas00 , your names are better than mine. Go ahead!

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 really like the new comments.

@LysandreJik
Copy link
Member

You can safely ignore the failed connections. It's been happening since the change to git-based repos. We're looking into fixing it with @julien-c, it happens very often.

@stas00 stas00 merged commit e84786a into huggingface:master Nov 23, 2020
@stas00 stas00 deleted the ignore-keys branch November 23, 2020 20:33
@stas00
Copy link
Contributor Author

stas00 commented Nov 23, 2020

@LysandreJik, I trust you will document this breaking change - I just don't know where I'd do that...

@LysandreJik
Copy link
Member

Yes, I'm currently documenting all breaking changes in the release notes.

LysandreJik pushed a commit that referenced this pull request Nov 23, 2020
* consistent ignore keys + make private

* style

* - authorized_missing_keys    => _keys_to_ignore_on_load_missing
  - authorized_unexpected_keys => _keys_to_ignore_on_load_unexpected

* move public doc of private attributes to private comment
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.

[save/load model] authorized keys, no save keys, etc.

3 participants