marian-nmt / marian-regression-tests

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

Add tests for intgemm refactor #73

Closed snukky closed 3 years ago

snukky commented 3 years ago

This updates https://github.com/marian-nmt/marian-regression-tests/pull/56 for changes proposed in https://github.com/marian-nmt/marian-dev/pull/762.

It seems packing a model on the fly is not supported there (options --int8/--int16 are removed), so the following tests fail:

test_intgemm_16bit.sh
test_intgemm_8bit.sh

Also, I don't know yet how to combine it with fbgemm, and the option for shifted version is not exposed, so the following fails:

test_fbgemm_packed8_intgemm_int8.sh
test_fbgemm_packed8_intgemm_int8_shifted.sh
test_intgemm_8bit_shifted.sh

The last two tests produce different outputs than the original https://github.com/marian-nmt/marian-dev/pull/595:

test_intgemm_16bit_preprocessed.sh
test_intgemm_8bit_preprocessed.sh

@XapaJIaMnu Could you check if the different outputs produced by the intgemm_refactor branch are expected and update other tests or temporarily turn them off? All tests work with intgemm_refactorized.

Also, we don't have tests with sse2/sse3 yet.

XapaJIaMnu commented 3 years ago

This will get slightly different results as the output layer no longer goes through intgemm. Have you checked that it passes?

snukky commented 3 years ago

I'm not sure what you are referring to. I have only run the tests you see. They fail mostly because of lack of --int8/--int16/--int8shift. I updated two tests that convert a model with marian-conv and they fail too, which is expected if I understand you correctly. Take a look to other tests and decide what to do with them. Maybe they should wait until all expected changes to intgemm_reintegrated are done.

XapaJIaMnu commented 3 years ago

Then this should go in and then we'll fix the expected outputs and probabilities?

snukky commented 3 years ago

Yes, I think we can just update the tests and add new tests (some of the existing tests will need to be removed) when all changes are done.