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
Move tokenization logic to central JiantModelTransformers method #1290
Move tokenization logic to central JiantModelTransformers method #1290
Conversation
de4c5e9
to
291166d
Compare
291166d
to
21e7736
Compare
Codecov Report
@@ Coverage Diff @@
## js/feature/easy_add_model #1290 +/- ##
=============================================================
+ Coverage 49.75% 49.82% +0.06%
=============================================================
Files 162 163 +1
Lines 11112 11166 +54
=============================================================
+ Hits 5529 5563 +34
- Misses 5583 5603 +20
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I noticed that ELECTRA doesn't have a normalize tokenizations implementation. Can we add an abstractmethod to
JiantTransformersModel
and araise NotImplemented
for ELECTRA? (It should be possible to add the implementation for ELECTRA, but I'm okay with leaving it as a future to-do) - I was wondering if it might be a better idea to pull the normalize tokenization logic out into individual functions (in some cases, similar models have the same logic), but I can also see how that might be excessive delegation. I don't feel strongly either way, what do you think?
68cc8b5
to
fc64dcd
Compare
…tion normalization
fc64dcd
to
33faa25
Compare
Comments implemented! For point 2 above, discussed offline to refactor if there is additional duplicate code in the future. |
d1b6367
to
ad49c64
Compare
* 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>
This PR refactors
normalize_tokenizations()
by moving the functions toJiantTransformerModel
s. Although the location of this logic is not ideal, it was the easiest way to have a central location for editing new models/tokenizers. This PR also addsresolve_model_arch_tokenizer(tokenizer)
to map the tokenizers to model_archs.