search

marian-nmt / marian-regression-tests

Regression tests for the marian-dev repository
Other
4 stars 9 forks source link

tests for factors embeddings combined with concat and for usage with the transformer model #78

Closed pedrodiascoelho closed 3 years ago

pedrodiascoelho commented 3 years ago

Regression tests done in the scope of the marian-cef EU project to test the factors code. This tests follow the same approach of using factors as explained here (#77).

Added two tests that test combining lemma and factor embeddings with concatenation, for training and decoding.

Also added an extra test that tests the usage of the transformer in a factored model with the original combining approach (combining with sum).

The .tar of the new pretrained model can be found here: https://drive.google.com/file/d/1opWBWxqeh75qVcKJn0mHdzA1gd-NKgZv/view?usp=sharing

cc @snukky @kpu

snukky commented 3 years ago

I've added the new pretrained model to factors.tar.gz on the Marian's Blob Storage. Running make models should take care of updating the model in an existing work directory. If I'm not wrong, the expected directory structure is the following (and this is what I used):

└── factors
   ├── factors_concat
   │   └── model.npz
   ├── model.npz
   ├── vocab.de.fsv
   ├── vocab.de.yml
   └── vocab.en.fsv

@pedrodiascoelho Could you confirm that this is the structure that you expect in new tests? Thanks!

pedrodiascoelho commented 3 years ago

Hello @snukky yes this is the correct. Do you have a reorganization suggestion?

snukky commented 3 years ago

No, just wanted to double check that I did it correctly. Thanks! We will merge it when we have your PR with factors in the master.

graemenail commented 3 years ago

.gitignore needs to be updated to ignore factors/factors_concat/model.npz

snukky commented 3 years ago

Good catch, thanks! @pedrodiascoelho, could you update this later?

snukky commented 3 years ago

The test

tests/factors/test_factors_decoder_concat.sh

fails with the newest master from marian-cef (cb44bbc435afdd65619bb1cba317c3748983ba5f) because 5 lines differ from the expected output. @pedrodiascoelho Could you please take a look and update the test if necessary? Thanks!

I use the standard compilation command:

CC=/usr/bin/gcc-8 CXX=/usr/bin/g++-8 CUDAHOSTCXX=/usr/bin/g++-8 cmake -DUSE_SENTENCEPIECE=on -DCUDA_TOOLKIT_ROOT_DIR=/usr/local/cuda-10.1 -DCOMPILE_CPU=on -DUSE_FBGEMM=on -DCOMPILE_SERVER=on -DCOMPILE_TESTS=on -DCOMPILE_EXAMPLES=on -DCOMPILE_MAXWELL=on ..

and run on GeForce GTX Titan X.

pedrodiascoelho commented 3 years ago

Hello @snukky Thank you so much for the review and feedback. I recompiled marian-cef (cb44bbc435afdd65619bb1cba317c3748983ba5f) from scratch with your compilation line, and I did pass the test you mentioned failing. I run it in an AWS Tesla T4 GPU. Although it did fail in one line in the test_factors_decoder.sh:

78c78
< in|s0 Haus|s1 besit|s1 zer|s0 fallenden|s1 enden|s0 Zentren|s0
---
> in|s0 den|s0 Heimat|s1 besit|s0 fallenden|s0 Zentren|s0

Could this different behaviour we experienced be GPU related? If you agree could you update the expected lines you are failing? Or tell me which lines are they and I'll be glad to update it myself.

Once again, thanks for the review

snukky commented 3 years ago

Thanks for checking this, Pedro. OK, that's fine, let's assume these differences are due to different GPUs. I will run these tests again before merging the tests into master and will then update the expected output.

pedrodiascoelho commented 3 years ago

I ran the tests in a different GPU to confirm my hypothesis and by running in a GeForce GTX 1080 Ti, I only failed the tests/factors/test_factors_decoder_concat.sh in these five lines:

25c25
< die Änderung des Nationalen L@@ ad@@ line &quot; , die Arbeitnehmer &quot; vor der Aufnahme einer Diskriminierung des Gewerkschafts@@ bun@@ des für die zivil@@ rechtlichen Rahmens ermöglichen soll - und Ausgleichs@@ maßnahmen in Erwägung gezogen werden .
---
> die Änderung des Nationalen L@@ ad@@ line &quot; , die Arbeitnehmer &quot; vor der Aufnahme einer Diskriminierung in das Gesetz des Heimat@@ landes um@@ ge@@ kehr@@ te Gewerk@@ schaft@@ s Act &quot; ermöglicht , sich nachteilig und die erforderliche Korrektur .
34c34
< als ehemalige Erste den Vorsitz der Menschenrechtskommission der Internationalen Kommission für Menschenrechte , die in der Allgemeinen Erklärung der Menschenrechte verfasst , die von den Vereinten Nationen als ein globales Monopol eingeführt wurde , und die Dra@@ ch@@ ter beteiligt .
---
> als ehemalige Erste den Vorsitz der Menschenrechtskommission der Internationalen Kommission für Menschenrechte , die in der Allgemeinen Erklärung der Menschenrechte verfasst wird , die von den Vereinten Nationen als ein globales Monopol eingeführt wurde , das die Vereinten Nationen einschließt und die Dra@@ men eine garantieren .
78c78
< Eigentums@@ anlagen fallen Eigentums@@ di@@ chter
---
> Eigentums@@ - Eigentums@@ zentren
89c89
< die Information Ministerin , Per@@ je@@ z Ras@@ etes , sagte jedoch , dass Soldaten die Möglichkeit hätten , den Preis für das Gebäude zu streichen .
---
> doch die Information Ministerin , Per@@ je@@ z Ras@@ etes , hat gesagt , dass Soldaten die Möglichkeit hätten zur K@@ asse gebeten .
91c91
< gleichzeitig demonstri@@ erten rund um den Demonstranten , dass die offiziellen Versuche , den offiziellen Kor@@ respon@@ den@@ ten von Premierminister Na@@ wa@@ gen@@ if zu erreichen .
---
> gleichzeitig demonstri@@ erten rund um den Demonstranten , dass die offiziellen Bemühungen um eine offizielle Empf@@ an@@ gs@@ region von Premierminister N@@ wa@@ z zu erreichen .

Are these the same ones that gave you errors? Also these confirms that the difference in the outputs seem to be GPU related as we suspected.

snukky commented 3 years ago

Yes, from what I remember, these differences look very similar. Feel free to update the outputs. Thanks for checking this!

pedrodiascoelho commented 3 years ago

Output file updated! Let me know if you run into any other issue.