cleanup tf unittests: part 2 #6260

stas00 · 2020-08-05T04:22:33Z

This is part 2 of #6196 and the original issue #5973. This PR brings the tf and pt tests and templates to an almost identical template.

edit: as other parts of code have evolved, the part of this comment below this line is no longer relevant - see the development of this PR in the subsequent comments

As discussed at #5973 (comment), a wrapper was needed to turn plain results dicts into object-like dicts, so that the results.key works the same whether it's returned by a class or it was just a local dict. I'm not sure DictAttr is the best name, but I had to start somewhere - it'll be an easy thing to rename it once you help me to find a better name. I did look for a pre-existing library to facilitate this, but didn't find anything that doesn't require installing another package.

To remind - the key change in part1 and in this clean up is to replace:

-            result = {
-                "sequence_output": sequence_output.numpy(),
-                "pooled_output": pooled_output.numpy(),
-            }
-            self.parent.assertListEqual(
-                list(result["sequence_output"].shape), [self.batch_size, self.seq_length, self.hidden_size]
-            )
-            self.parent.assertListEqual(list(result["pooled_output"].shape), [self.batch_size, self.hidden_size])

with:

+            result = DictAttr({"sequence_output": sequence_output.numpy(), "pooled_output": pooled_output.numpy()})
+            self.parent.assertEqual(result.sequence_output.shape, (self.batch_size, self.seq_length, self.hidden_size))
+            self.parent.assertEqual(result.pooled_output.shape, (self.batch_size, self.hidden_size))

like it was done in part 1, except part 1 didn't need the wrapper - results were bona fide objects there.

there is also one torch test in this batch that required the wrapper too: test_modeling_transfo_xl.py

@sshleifer

stas00 · 2020-08-05T04:31:29Z

Hmm, looks like isort and flake are in conflict with each other. make style reformatted the code to:

        result = DictAttr({"sequence_output": sequence_output.numpy(), "pooled_output": pooled_output.numpy(),})

and flake complains:

tests/test_modeling_tf_albert.py:139:110: E231 missing whitespace after ','

if I add the desired by flake whitespace, isort removes it.

How do I resolve such conflict?

edit: found a workaround, by removing the trailing comma:

perl -pi -e 's|\),\}\)|)})|' tests/test_modeling_tf* templates/adding_a_new_model/tests/test_modeling_tf_xxx.py

stas00 · 2020-08-05T04:37:41Z

99% of this PR has been facilitated by running:

perl -0777 -pi -e 's#self.parent.assertListEqual\(
[\s\n]*
list\((result\w*)\[" ([^"]+) "\].(?:shape|size\(\))\),[\s\n]+\[ ( [^\]]* ) \],?
[\s\n]*
\)
#self.parent.assertEqual($1.$2.shape, ($3))#xmsg' tests/test*py templates/adding_a_new_model/tests/test_modeling_tf_xxx.py

perl -0777 -pi -e 's#
result\s=\s\{
([^}]+)
\}
#result = DictAttr({$1}) #xmsg' tests/test*py templates/adding_a_new_model/tests/test_modeling_tf_xxx.py

perl -pi -e 's|^(from transformers.testing_utils import .*?)$|$1, DictAttr|' tests/test_modeling_tf_albert.py tests/test_modeling_tf_bert.py tests/test_modeling_tf_ctrl.py tests/test_modeling_tf_distilbert.py tests/test_modeling_tf_electra.py tests/test_modeling_tf_flaubert.py tests/test_modeling_tf_gpt2.py tests/test_modeling_tf_mobilebert.py tests/test_modeling_tf_openai_gpt.py tests/test_modeling_tf_roberta.py tests/test_modeling_tf_t5.py tests/test_modeling_tf_transfo_xl.py tests/test_modeling_tf_xlm.py tests/test_modeling_tf_xlnet.py test_modeling_transfo_xl.py templates/adding_a_new_model/tests/test_modeling_tf_xxx.py

make style
# remove trailing comma that breaks flake
perl -pi -e 's|\),\}\)|)})|' tests/test_modeling_tf* templates/adding_a_new_model/tests/test_modeling_tf_xxx.py

codecov · 2020-08-05T04:42:52Z

Codecov Report

Merging #6260 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6260   +/-   ##
=======================================
  Coverage   80.08%   80.08%           
=======================================
  Files         153      153           
  Lines       27984    27984           
=======================================
  Hits        22412    22412           
  Misses       5572     5572

Impacted Files	Coverage Δ
src/transformers/modeling_tf_distilbert.py	`64.47% <0.00%> (-32.95%)`	⬇️
src/transformers/file_utils.py	`82.44% <0.00%> (+0.25%)`	⬆️
src/transformers/modeling_tf_gpt2.py	`94.72% <0.00%> (+22.87%)`	⬆️
src/transformers/tokenization_albert.py	`87.50% <0.00%> (+58.65%)`	⬆️

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 0735def...d3380ba. Read the comment docs.

sgugger · 2020-08-05T13:15:16Z

Hi @stas00. I'm in the process of changing all model outputs of TF models to the same as PyTorch and updated all the tests. This is in #6247 that I expect to merge today (once merge conflicts are handled and I've added the templates).

Could you wait a tiny bit and then apply the same PERL commands you did in #6196 ?

stas00 · 2020-08-05T14:21:00Z

Oh, that's even better - yes, of course, I will wait, @sgugger! If you remember please ping me when this is done. Thank you!

Could you also check on this one?
https://github.com/huggingface/transformers/blob/master/tests/test_modeling_transfo_xl.py#L94
It's the only remaining torch test with results dict.

sgugger · 2020-08-05T15:17:16Z

For the slow test, we can use outputs1["last_hidden_state"] and so forth in the asserts after. I went for the quickest fixes in the tests.

stas00 · 2020-08-05T15:25:30Z

For the slow test, we can use outputs1["last_hidden_state"] and so forth in the asserts after. I went for the quickest fixes in the tests.

Part of this rewrite was replacing results["key"] with results.key as specified. So this one would be different. There are two constructors in test_modeling_transfo_xl.py that do that. All other tests have or will follow the latter style once you finish your work and I will update mine.

sgugger · 2020-08-05T15:35:13Z

The outputs PR has been merged, so pinging you @stas00

stas00 · 2020-08-05T15:40:21Z

I will rework this PR to match @sgugger changes.

…ixes to match huggingface#6247

stas00 · 2020-08-05T16:33:09Z

@sgugger, after your merge, these 3 tf tests are still using the old style it seems:

FAILED tests/test_modeling_tf_transfo_xl.py::TFTransfoXLModelTest::test_transfo_xl_lm_head - AttributeError: 'dict' object has no attribute 'lm_logits_1'
FAILED tests/test_modeling_tf_transfo_xl.py::TFTransfoXLModelTest::test_transfo_xl_model - AttributeError: 'dict' object has no attribute 'hidden_states_1'
FAILED tests/test_modeling_tf_xlnet.py::TFXLNetModelTest::test_xlnet_lm_head - AttributeError: 'dict' object has no attribute 'all_logits_1'

I reverted to the ["key"] style for these for now.

sgugger

Thanks for taking care of the merge conflicts. I guess we can deal with the 4 remaining tests (one PT, 3 TF) in a separate PR.

stas00 · 2020-08-06T03:58:51Z

I guess we can deal with the 4 remaining tests (one PT, 3 TF) in a separate PR.

Yes. Could you please instruct me on how to proceed with those or will you take care of them?

sgugger · 2020-08-06T14:18:48Z

I think just using the result1/result2 in the assers instead of grouping everything in a dict should be fine.

sgugger · 2020-08-06T19:11:45Z

tests/test_modeling_tf_transfo_xl.py

-        self.parent.assertEqual(result["hidden_states_2"].shape, (self.batch_size, self.seq_length, self.hidden_size))
+        hidden_states_1 = hidden_states_1.numpy()
+        hidden_states_2 = hidden_states_2.numpy()
+        mems_1 = [mem.numpy() for mem in mems_1]


I don't think all the calls to numpy are necessary. Do the tests pass without them?

It's not needed, indeed. removed and pushed.

stas00 · 2020-08-06T19:21:51Z

please re-run CI - unrelated failure - thank you.

sgugger

Thanks @stas00 , there are a few .numpy left that we can clean up and this will be good to merge. Pinging @sshleifer or @LysandreJik for a second review.

sgugger · 2020-08-06T19:56:22Z

tests/test_modeling_tf_transfo_xl.py

@@ -92,30 +92,21 @@ def create_and_check_transfo_xl_model(self, config, input_ids_1, input_ids_2, lm
        model = TFTransfoXLModel(config)

        hidden_states_1, mems_1 = model(input_ids_1).to_tuple()
+        mems_1 = [mem.numpy() for mem in mems_1]


Same for those here, I don't think you need that line and the equivalent for mems2

sgugger · 2020-08-06T19:56:42Z

tests/test_modeling_tf_xlnet.py

-            "mems_2": [mem.numpy() for mem in mems_2],
-            "all_logits_2": all_logits_2.numpy(),
-        }
+        mems_1 = [mem.numpy() for mem in mems_1]


Same remark for the .numpy in this file.

sshleifer · 2020-08-06T20:14:31Z

tests/test_modeling_xlnet.py

            [[self.seq_length, self.batch_size, self.hidden_size]] * self.num_hidden_layers,
        )

        self.parent.assertEqual(result2.loss.shape, ())
        self.parent.assertEqual(result2.logits.shape, (self.batch_size, self.seq_length, self.vocab_size))
        self.parent.assertListEqual(
-            list(list(mem.size()) for mem in result2["mems"]),
+            list(list(mem.size()) for mem in result2.mems),


can this be
self.assertListEqual([mem.size() for mem in result2.mems], [(self.seq_length, self.batch_size, self.hidden_size)]*self.num_hidden_layers)

somewhat cleaner IMO but no big deal.

plus .size() => .shape - will do. thank you.

So changing:

- list(list(mem.shape) for mem in mems_1), - [[self.mem_len, self.batch_size, self.hidden_size]] * self.num_hidden_layers, + [mem.shape for mem in mems_1], + [(self.mem_len, self.batch_size, self.hidden_size)] * self.num_hidden_layers,

Perl:

perl -0777 -pi -e 's#self.parent.assertListEqual$ [\s\n]* list \( list \( mem.(?:shape|size\($)\)\sfor\smem\s(.*?) \), [\s\n]* \[\[ (.*?) \]\]\s\*\sself.num_hidden_layers, #self.parent.assertListEqual( [mem.shape for mem $1], [($2)] * self.num_hidden_layers,#xmsg' tests/test*py

edit: pushed

stas00 · 2020-08-06T20:58:23Z

If there is any more cleanup you'd like to see send me the before and after code snippets.

LysandreJik

LGTM, thanks @stas00!

This reverts commit 61131c6.

cleanup torch unittests: part 2

68b7b04

stas00 added 2 commits August 4, 2020 21:34

remove trailing comma added by isort, and which breaks flake

6906427

one more comma

4228e16

stas00 changed the title ~~[WIP] cleanup torch unittests: part 2~~ [WIP] cleanup tf unittests: part 2 Aug 5, 2020

stas00 added 3 commits August 5, 2020 09:14

Merge remote-tracking branch 'origin/master' into unittests-p2

d943205

Merge remote-tracking branch 'origin/master' into unittests-p2 with f…

454bab6

…ixes to match huggingface#6247

revert odd balls

b4e890d

stas00 mentioned this pull request Aug 5, 2020

Tf model outputs #6247

Merged

sgugger approved these changes Aug 5, 2020

View reviewed changes

part 3: odd cases

65a431f

sgugger reviewed Aug 6, 2020

View reviewed changes

stas00 added 2 commits August 6, 2020 12:17

more ["key"] -> .key refactoring

3b93811

.numpy() is not needed

b22826d

sgugger approved these changes Aug 6, 2020

View reviewed changes

sgugger requested review from LysandreJik and sshleifer August 6, 2020 19:57

sshleifer approved these changes Aug 6, 2020

View reviewed changes

stas00 added 2 commits August 6, 2020 13:17

more unncessary .numpy() removed

7a72f33

more simplification

a4924d1

sshleifer approved these changes Aug 6, 2020

View reviewed changes

sshleifer changed the title ~~[WIP] cleanup tf unittests: part 2~~ cleanup tf unittests: part 2 Aug 6, 2020

LysandreJik approved these changes Aug 12, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into unittests-p2

d3380ba

LysandreJik merged commit e983da0 into huggingface:master Aug 13, 2020

stas00 deleted the unittests-p2 branch August 13, 2020 16:53

fabiocapsouza added a commit to fabiocapsouza/transformers that referenced this pull request Nov 15, 2020

Revert "cleanup tf unittests: part 2 (huggingface#6260)"

0c22ba2

This reverts commit 61131c6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup tf unittests: part 2 #6260

cleanup tf unittests: part 2 #6260

stas00 commented Aug 5, 2020 •

edited

Loading

stas00 commented Aug 5, 2020 •

edited

Loading

stas00 commented Aug 5, 2020

codecov bot commented Aug 5, 2020 •

edited

Loading

sgugger commented Aug 5, 2020

stas00 commented Aug 5, 2020 •

edited

Loading

sgugger commented Aug 5, 2020

stas00 commented Aug 5, 2020 •

edited

Loading

sgugger commented Aug 5, 2020

stas00 commented Aug 5, 2020

stas00 commented Aug 5, 2020

sgugger left a comment

stas00 commented Aug 6, 2020 •

edited

Loading

sgugger commented Aug 6, 2020

sgugger Aug 6, 2020

stas00 Aug 6, 2020

stas00 commented Aug 6, 2020

sgugger left a comment

sgugger Aug 6, 2020

sgugger Aug 6, 2020

sshleifer Aug 6, 2020

stas00 Aug 6, 2020

stas00 Aug 6, 2020 •

edited

Loading

stas00 commented Aug 6, 2020

LysandreJik left a comment

cleanup tf unittests: part 2 #6260

cleanup tf unittests: part 2 #6260

Conversation

stas00 commented Aug 5, 2020 • edited Loading

stas00 commented Aug 5, 2020 • edited Loading

stas00 commented Aug 5, 2020

codecov bot commented Aug 5, 2020 • edited Loading

Codecov Report

sgugger commented Aug 5, 2020

stas00 commented Aug 5, 2020 • edited Loading

sgugger commented Aug 5, 2020

stas00 commented Aug 5, 2020 • edited Loading

sgugger commented Aug 5, 2020

stas00 commented Aug 5, 2020

stas00 commented Aug 5, 2020

sgugger left a comment

Choose a reason for hiding this comment

stas00 commented Aug 6, 2020 • edited Loading

sgugger commented Aug 6, 2020

sgugger Aug 6, 2020

Choose a reason for hiding this comment

stas00 Aug 6, 2020

Choose a reason for hiding this comment

stas00 commented Aug 6, 2020

sgugger left a comment

Choose a reason for hiding this comment

sgugger Aug 6, 2020

Choose a reason for hiding this comment

sgugger Aug 6, 2020

Choose a reason for hiding this comment

sshleifer Aug 6, 2020

Choose a reason for hiding this comment

stas00 Aug 6, 2020

Choose a reason for hiding this comment

stas00 Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

stas00 commented Aug 6, 2020

LysandreJik left a comment

Choose a reason for hiding this comment

stas00 commented Aug 5, 2020 •

edited

Loading

stas00 commented Aug 5, 2020 •

edited

Loading

codecov bot commented Aug 5, 2020 •

edited

Loading

stas00 commented Aug 5, 2020 •

edited

Loading

stas00 commented Aug 5, 2020 •

edited

Loading

stas00 commented Aug 6, 2020 •

edited

Loading

stas00 Aug 6, 2020 •

edited

Loading