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

[AutoModels] Fix config params handling of all PT and TF AutoModels #5665

Conversation

patrickvonplaten
Copy link
Contributor

@patrickvonplaten patrickvonplaten commented Jul 10, 2020

As shown in #5474, currently, a command like:

from transformers import AutoModelForCausalLM]
model = AutoModelForCausalLM.from_pretrained('bert-base-uncased', is_decoder=True)

fails because is_decoder is carried on as a model init argument even though it should only be used as a config init argument.

This PR fixes one AutoModelFor.... for this, but this still has be applied for other AutoModelFor... classes.

Pinging @LysandreJik @sgugger @thomwolf - are you guys ok with this change (bug fix) in general?

@patrickvonplaten patrickvonplaten requested review from LysandreJik and sgugger and removed request for LysandreJik July 10, 2020 15:46
@patrickvonplaten patrickvonplaten linked an issue Jul 10, 2020 that may be closed by this pull request
1 task
@sgugger
Copy link
Collaborator

sgugger commented Jul 10, 2020

Isn't the canonical way:

config, kwargs = AutoConfig.from_pretrained(pretrained_model_name_or_path, return_unused_kwargs=True, **kwargs)

in the test?

@codecov
Copy link

codecov bot commented Jul 10, 2020

Codecov Report

Merging #5665 into master will decrease coverage by 1.11%.
The diff coverage is 55.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5665      +/-   ##
==========================================
- Coverage   78.43%   77.32%   -1.12%     
==========================================
  Files         146      146              
  Lines       26002    26002              
==========================================
- Hits        20395    20105     -290     
- Misses       5607     5897     +290     
Impacted Files Coverage Δ
src/transformers/modeling_tf_auto.py 63.03% <50.00%> (ø)
src/transformers/modeling_auto.py 74.41% <60.00%> (ø)
src/transformers/modeling_tf_mobilebert.py 23.38% <0.00%> (-73.39%) ⬇️
src/transformers/generation_tf_utils.py 86.46% <0.00%> (+0.25%) ⬆️
src/transformers/file_utils.py 81.49% <0.00%> (+0.29%) ⬆️
src/transformers/modeling_tf_distilbert.py 98.79% <0.00%> (+33.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce374ba...3bb7966. Read the comment docs.

@patrickvonplaten
Copy link
Contributor Author

Isn't the canonical way:

config, kwargs = AutoConfig.from_pretrained(pretrained_model_name_or_path, return_unused_kwargs=True, **kwargs)

in the test?

Oh yeah, that's much cleaner. We should probably update all AutoModels in PT and TF with this then, no?

@LysandreJik
Copy link
Member

I think that's correct, and the way it was always meant to be 🤨

@sgugger
Copy link
Collaborator

sgugger commented Jul 10, 2020

We should probably update all AutoModels in PT and TF with this then, no?

Yes, I agree.

@patrickvonplaten patrickvonplaten force-pushed the fix_auto_model_with_config_kwargs branch from 754d5e2 to 873887a Compare July 13, 2020 15:01
@patrickvonplaten patrickvonplaten changed the title [WIP - Don't merge yet][AutoModels] Fix config params handling of AutoModels [AutoModels] Fix config params handling of all PT and TF AutoModels Jul 13, 2020
Copy link
Member

@thomwolf thomwolf left a comment

Choose a reason for hiding this comment

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

LGTM!

@patrickvonplaten patrickvonplaten merged commit ec0a945 into huggingface:master Jul 15, 2020
LoicGrobol added a commit to hopsparser/hopsparser that referenced this pull request Nov 9, 2020
(transformers >3.1.0 fixed the config kwargs handling in
huggingface/transformers#5665
)
LoicGrobol added a commit to hopsparser/hopsparser that referenced this pull request Nov 9, 2020
(transformers >3.1.0 fixed the config kwargs handling in
huggingface/transformers#5665
)
LoicGrobol added a commit to hopsparser/hopsparser that referenced this pull request Nov 11, 2020
(transformers >3.1.0 fixed the config kwargs handling in
huggingface/transformers#5665
)
LoicGrobol added a commit to hopsparser/hopsparser that referenced this pull request Nov 12, 2020
(transformers >3.1.0 fixed the config kwargs handling in
huggingface/transformers#5665
)
LoicGrobol added a commit to hopsparser/hopsparser that referenced this pull request Nov 12, 2020
(transformers >3.1.0 fixed the config kwargs handling in
huggingface/transformers#5665
)
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.

Can't use AutoModelForCausalLM with bert
4 participants