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

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

Merged

Conversation

jeswan
Copy link
Collaborator

@jeswan jeswan commented Mar 4, 2021

This PR refactors the following:

  • adds getter for mlm_weights_dict to JiantTransformersModel
  • add getter for feat_spec to JiantTransformersModel
  • simplifies flat_strip()

@jeswan jeswan changed the base branch from master to js/feature/easy_add_model March 4, 2021 18:26
@jeswan jeswan changed the title Js/feature/remaining model if refactor 2 mlm_weights, feat_spec, flat_strip if-else refactors Mar 4, 2021
@jeswan jeswan changed the title mlm_weights, feat_spec, flat_strip if-else refactors mlm_weights(), feat_spec(), flat_strip() if-else refactors Mar 4, 2021
@jeswan jeswan marked this pull request as ready for review March 4, 2021 18:46
@jeswan jeswan requested review from HaokunLiu and zphang as code owners March 4, 2021 18:46
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #1288 (0a69e59) into js/feature/easy_add_model (c9c0410) will increase coverage by 0.00%.
The diff coverage is 35.91%.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           js/feature/easy_add_model    #1288   +/-   ##
==========================================================
  Coverage                      49.74%   49.75%           
==========================================================
  Files                            162      162           
  Lines                          11168    11112   -56     
==========================================================
- Hits                            5556     5529   -27     
+ Misses                          5612     5583   -29     
Impacted Files Coverage Δ
jiant/tasks/lib/ccg.py 57.14% <0.00%> (ø)
jiant/proj/main/modeling/model_setup.py 23.14% <7.69%> (+0.83%) ⬆️
jiant/proj/main/modeling/taskmodels.py 33.68% <18.46%> (ø)
jiant/proj/main/tokenize_and_cache.py 38.23% <50.00%> (ø)
jiant/proj/main/modeling/primary.py 55.06% <58.92%> (+3.36%) ⬆️
...tasks/lib/templates/hacky_tokenization_matching.py 22.22% <66.66%> (-37.78%) ⬇️
jiant/shared/model_resolution.py 75.00% <100.00%> (+28.84%) ⬆️

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 c9c0410...0a69e59. 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.

Looks good overall, see comment about flat_strip



def albert_flat_strip(tokens, return_indices=False):
def flat_strip(tokens, tokenizer, return_indices=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have tests for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We did not have tests before but I just added tests here: #1289. I'll check that the added tests pass on this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for requesting the tests! Found a bug with my implementation.

@jeswan jeswan merged commit f027a59 into js/feature/easy_add_model Mar 11, 2021
@jeswan jeswan deleted the js/feature/remaining_model_if_refactor_2 branch March 11, 2021 01:17
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.

2 participants