Skip to content

Conversation

@patrickvonplaten
Copy link
Contributor

Add integration tests for all LM models that are able to generate language.

  • All integration tests use do_sample=False (greedy) generation and verify that TF 2.0 and PT yield the same results.
  • Fixed a small bug with TFXLMModelWithLMHead

@julien-c
Copy link
Member

julien-c commented Mar 9, 2020

Approx. how long do these tests take on a single V100?

@patrickvonplaten
Copy link
Contributor Author

Approx. how long do these tests take on a single V100?

Don't know how long they take on a V100. On a cpu, all tests combined (7 model tests for PT and 6 model tests for TF) take less than 10min (whereas test_modeling_tf_xlnet.py, test_modeling_xlnet.py and test_modeling_transfo_xl.py combined take ca. 8min)

@codecov-io
Copy link

Codecov Report

Merging #3191 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3191      +/-   ##
==========================================
+ Coverage   78.15%   78.16%   +0.01%     
==========================================
  Files          98       98              
  Lines       16641    16641              
==========================================
+ Hits        13006    13008       +2     
+ Misses       3635     3633       -2
Impacted Files Coverage Δ
src/transformers/modeling_tf_xlm.py 90.4% <100%> (ø) ⬆️
src/transformers/modeling_utils.py 94.56% <0%> (+0.15%) ⬆️
src/transformers/modeling_tf_utils.py 93.89% <0%> (+0.19%) ⬆️

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 e03129a...9050ffe. Read the comment docs.

Copy link
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.

Nice, LGTM!


tensor = inputs_embeds + self.position_embeddings(position_ids)
if langs is not None and self.use_lang_emb:
if langs is not None and self.use_lang_emb and self.n_langs > 1:
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

@patrickvonplaten
Copy link
Contributor Author

Good to merge for me

@patrickvonplaten patrickvonplaten merged commit 31f2437 into huggingface:master Mar 10, 2020
@patrickvonplaten patrickvonplaten deleted the add_integration_tests_lm_generate_torch_tf branch March 10, 2020 10:29
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.

4 participants