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 to task model/head factories instead of embedded if-else statements #1268

Merged

Conversation

jeswan
Copy link
Collaborator

@jeswan jeswan commented Jan 20, 2021

The goal of these changes is to streamline adding a model to jiant. This PR implements factories to create task models and task heads. In addition, JiantTransformersModelWrapper has been added to abstract and inconsistencies in HuggingFace's Transformers library.

@jeswan jeswan changed the title Use jiant transformers model wrapper (only supports BERT for feedback… Switch to task model/head factories instead of if-else statements Jan 20, 2021
@jeswan jeswan requested a review from zphang January 20, 2021 00:59
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.

Some comments for organization of logic, but overall no opposition.

jiant/proj/main/modeling/heads.py Show resolved Hide resolved
ModelArchitectures.MBART,
ModelArchitectures.ELECTRA,
):
raise NotImplementedError()
Copy link
Collaborator

Choose a reason for hiding this comment

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

f"MLM not supported for {model_arch}"

jiant/proj/main/modeling/model_setup.py Show resolved Hide resolved
jiant/proj/main/modeling/primary.py Outdated Show resolved Hide resolved
@jeswan jeswan changed the title Switch to task model/head factories instead of if-else statements Switch to task model/head factories instead of embedded if-else statements Jan 20, 2021
@jeswan jeswan force-pushed the js/feature/add_taskmodel_and_head_factory branch 3 times, most recently from 07d3fdb to ee889fa Compare January 27, 2021 04:58
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #1268 (9e495aa) into js/feature/easy_add_model (b2cfb2a) will increase coverage by 0.77%.
The diff coverage is 63.82%.

Impacted file tree graph

@@                      Coverage Diff                      @@
##           js/feature/easy_add_model    #1268      +/-   ##
=============================================================
+ Coverage                      48.52%   49.29%   +0.77%     
=============================================================
  Files                            162      162              
  Lines                          11195    11186       -9     
=============================================================
+ Hits                            5432     5514      +82     
+ Misses                          5763     5672      -91     
Impacted Files Coverage Δ
jiant/proj/main/modeling/model_setup.py 22.30% <17.64%> (+5.55%) ⬆️
jiant/proj/main/modeling/taskmodels.py 32.46% <55.12%> (+7.70%) ⬆️
jiant/proj/main/modeling/primary.py 53.68% <66.15%> (+24.65%) ⬆️
jiant/proj/main/modeling/heads.py 43.27% <75.47%> (+11.97%) ⬆️
jiant/shared/model_resolution.py 44.61% <90.00%> (+12.64%) ⬆️
jiant/tasks/core.py 64.34% <100.00%> (ø)

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 b2cfb2a...9e495aa. Read the comment docs.

@jeswan jeswan force-pushed the js/feature/add_taskmodel_and_head_factory branch from ee889fa to 3df2b38 Compare January 28, 2021 19:13
@jeswan jeswan marked this pull request as ready for review January 29, 2021 00:12
@jeswan jeswan changed the base branch from master to js/feature/easy_add_model February 2, 2021 07:56
@jeswan jeswan requested a review from zphang February 25, 2021 15:07
@jeswan jeswan merged commit e004af8 into js/feature/easy_add_model Feb 25, 2021
@jeswan jeswan deleted the js/feature/add_taskmodel_and_head_factory branch February 25, 2021 15:25
jeswan added a commit that referenced this pull request May 4, 2021
* Update to Transformers v4.3.3 (#1266)

* use default return_dict in taskmodels and remove hidden state context manager in models.

* return hidden states in output of model wrapper

* Switch to task model/head factories instead of embedded if-else statements (#1268)

* Use jiant transformers model wrapper instead of if-else. Use taskmodel and head factory instead of if-else.

* switch to ModelArchitectures enum instead of strings

* Refactor get_output_from_encoder() to be member of JiantTaskModel (#1283)

* refactor getting output from encoder to be member function of jiant model

* switch to explicit encode() in jiant transformers model

* fix simple runscript test

* update to tokenizer 0.10.1

* Add tests for flat_strip() (#1289)

* add flat_strip test

* add list to test cases flat_strip

* mlm_weights(), feat_spec(), flat_strip() if-else refactors (#1288)

* moves remaining if-else statments to jiant model or replaces with model agnostic method

* switch from jiant_transformers_model to encoder

* fix bug in flat_strip()

* Move tokenization logic to central JiantModelTransformers method (#1290)

* move model specific tokenization logic to JiantTransformerModels

* implement abstract methods for JiantTransformerModels

* fix tasks circular import (#1296)

* Add DeBERTa (#1295)

* Add DeBERTa with sanity test

* fix tasks circular import

* [WIP] add deberta tests

* Revert "fix tasks circular import"

This reverts commit f924640.

* deberta tests passing with transformers 6472d8

* switch to deberta-v2

* fix get_mlm_weights_dict() for deberta-v2

* update to transformers 4.5.0

* mark deberta test_export as slow

* Update test_tokenization_normalization.py

* add guide to add a model

* fix test_expor_model tests

* minor pytest fixes (add num_labels for rte, overnight flag fix)

* bugfix for simple api notebook

* bugfix for #1310

* bugfix for #1306: simple api notebook path name

* squad running

* 2nd bugfix for #1310: not all tasks have num_labels property

* simple api notebook back to roberta-base

* run test matrix for more steps to compare to master

* save last/best model test fix

Co-authored-by: Jesse Swanson <js11133Wnyu.edu>
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.

None yet

2 participants