Skip to content

[EncoderDecoder Tests] Improve tests#4046

Merged
patrickvonplaten merged 4 commits intohuggingface:masterfrom
patrickvonplaten:improve_tests_for_encoder_decoder
May 4, 2020
Merged

[EncoderDecoder Tests] Improve tests#4046
patrickvonplaten merged 4 commits intohuggingface:masterfrom
patrickvonplaten:improve_tests_for_encoder_decoder

Conversation

@patrickvonplaten
Copy link
Copy Markdown
Contributor

@patrickvonplaten patrickvonplaten commented Apr 28, 2020

Currently when executing:
pytest tests/test_modeling_encoder_decoder.py all BERT tests are run as well due to problems with the import of BertModelTester (see PR #3383).

This PR takes the PR: #4027 and does some minimal changes in the encoder decoder test file.

I think @sshleifer found a great solution to circumvent that by moving the BertModelTester class out of the BertModelTest class.

What do you think @julien-c , @LysandreJik ?

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 28, 2020

Codecov Report

Merging #4046 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4046      +/-   ##
==========================================
+ Coverage   78.84%   78.85%   +0.01%     
==========================================
  Files         114      114              
  Lines       18689    18689              
==========================================
+ Hits        14735    14737       +2     
+ Misses       3954     3952       -2     
Impacted Files Coverage Δ
src/transformers/modeling_tf_utils.py 92.93% <0.00%> (+0.16%) ⬆️
src/transformers/file_utils.py 73.55% <0.00%> (+0.41%) ⬆️

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 6af3306...0ed64db. Read the comment docs.

Copy link
Copy Markdown
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

This looks great! What do you think @julien-c ?

Copy link
Copy Markdown
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

Small nit but otherwise looks good to me

Comment thread tests/test_modeling_bert.py Outdated
@patrickvonplaten patrickvonplaten force-pushed the improve_tests_for_encoder_decoder branch from 90657fe to 0ed64db Compare May 4, 2020 00:07
@patrickvonplaten patrickvonplaten merged commit 8e67573 into huggingface:master May 4, 2020
@patrickvonplaten patrickvonplaten deleted the improve_tests_for_encoder_decoder branch May 4, 2020 00:18
tianleiwu pushed a commit to tianleiwu/transformers that referenced this pull request May 8, 2020
* Hoist bert model tester for patric

* indent

* make tests work

* Update tests/test_modeling_bert.py

Co-authored-by: Julien Chaumond <chaumond@gmail.com>

Co-authored-by: sshleifer <sshleifer@gmail.com>
Co-authored-by: Julien Chaumond <chaumond@gmail.com>
@julien-c
Copy link
Copy Markdown
Member

By the way, do you guys want to do this for all other ModelTester objects @patrickvonplaten and @sshleifer? And maybe (if relevant) define a common abstract class they inherit from?

@patrickvonplaten
Copy link
Copy Markdown
Contributor Author

By the way, do you guys want to do this for all other ModelTester objects @patrickvonplaten and @sshleifer? And maybe (if relevant) define a common abstract class they inherit from?

I think this would be a good idea. We can probably clean up the tests a lot this way! I will note it down on my To-Do-List

@sshleifer
Copy link
Copy Markdown
Contributor

LMK if you want to split it up @patrickvonplaten

@patrickvonplaten
Copy link
Copy Markdown
Contributor Author

LMK if you want to split it up @patrickvonplaten

I tihnk I won't find time in the next 2 weeks to do this - feel free to start working on it if you want :-)

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.

5 participants