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

Switch export_model to use AutoModel and AutoTokenizer #1260

Merged
merged 8 commits into from
Jan 19, 2021

Conversation

jeswan
Copy link
Collaborator

@jeswan jeswan commented Jan 5, 2021

This PR refactors export_model to use AutoModel and AutoTokenizer. It must be approved after the tests for export_model have been merged (#1259). This PR reduces is part of the effort to reduce the if-else statements regarding model types in our codebase. model_tokenizer_path has been removed and replaced by hf_pretrained_model_name_or_path.

@zphang
Copy link
Collaborator

zphang commented Jan 6, 2021

If I'm reading this right, this PR needs to be updated to pass the new tests, right? export_model has a different signature I think.

@jeswan jeswan marked this pull request as draft January 6, 2021 16:05
jiant/proj/main/modeling/model_setup.py Outdated Show resolved Hide resolved
jiant/proj/main/modeling/model_setup.py Outdated Show resolved Hide resolved
jiant/shared/model_setup.py Show resolved Hide resolved
…ath. Update notebooks with AutoClass changes.
@jeswan jeswan force-pushed the js/feature/export_model_auto branch 2 times, most recently from 8ffa0ea to b269107 Compare January 12, 2021 19:13
@jeswan jeswan force-pushed the js/feature/export_model_auto branch from b269107 to f7b8b55 Compare January 12, 2021 19:16
@jeswan jeswan requested a review from zphang January 12, 2021 19:20
@jeswan jeswan marked this pull request as ready for review January 12, 2021 19:20
@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #1260 (1b9848c) into master (6e6c2e3) will increase coverage by 0.00%.
The diff coverage is 85.36%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1260   +/-   ##
=======================================
  Coverage   55.51%   55.52%           
=======================================
  Files         149      149           
  Lines       10903    10878   -25     
=======================================
- Hits         6053     6040   -13     
+ Misses       4850     4838   -12     
Impacted Files Coverage Δ
jiant/scripts/download_data/constants.py 100.00% <ø> (ø)
jiant/shared/model_setup.py 64.86% <ø> (+2.22%) ⬆️
jiant/shared/model_resolution.py 33.60% <37.50%> (-0.82%) ⬇️
jiant/proj/main/export_model.py 89.65% <90.00%> (+7.83%) ⬆️
jiant/proj/main/modeling/model_setup.py 37.93% <100.00%> (+1.24%) ⬆️
jiant/proj/main/modeling/taskmodels.py 39.06% <100.00%> (ø)
jiant/proj/main/runscript.py 73.04% <100.00%> (ø)
jiant/proj/main/tokenize_and_cache.py 86.76% <100.00%> (+0.19%) ⬆️
jiant/proj/simple/runscript.py 80.64% <100.00%> (+0.31%) ⬆️

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 6e6c2e3...1b9848c. Read the comment docs.

Copy link
Collaborator

@zphang zphang left a comment

Choose a reason for hiding this comment

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

Nothing jumps out to me, also relatively low risk to merge now, I think.

Potentially one thing you might want to look at is if updating to transformers 4+ will be complicated by any of this.

@jeswan
Copy link
Collaborator Author

jeswan commented Jan 14, 2021

@zphang A few notes on how this PR will be affected by Transformers v4:

  • AutoTokenizer will default to using the fast (rust) tokenizers. I am currently planning on switching to fast tokenizers as part of the update to Transformers v4.
  • slow versions of the following tokenizers will not be available since SentencePiece is removed from required dependencies:
    • XLNetTokenizer
    • AlbertTokenizer
    • CamembertTokenizer
    • MBartTokenizer
    • PegasusTokenizer
    • T5Tokenizer
    • ReformerTokenizer
    • XLMRobertaTokenizer
  • Side note: Model forward will have return_dict set to True as default and will require jiant forward passes to use False if using tuple unpacking.

@zphang
Copy link
Collaborator

zphang commented Jan 14, 2021

Sounds good, moving remaining discussion to other PR.

@jeswan jeswan merged commit 723786a into master Jan 19, 2021
@jeswan jeswan deleted the js/feature/export_model_auto branch January 19, 2021 05:35
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.

2 participants