search

browsermt / marian-dev

Fast Neural Machine Translation in C++ - development repository
https://marian-nmt.github.io
Other
20 stars 7 forks source link

Remove `static` precomputedAlphas bool in `affine` operation #78

Closed jelmervdl closed 2 years ago

jelmervdl commented 2 years ago

This fixes a bug when switching between translation models that have different gemm-precision precisions specified in their configuration file. Which is why we never noticed it with our own student models (all int8shiftAlpha), but do see it now with the Ukrainian model (which uses just int8)

I tried to go through git blame to figure out why it was marked static. @XapaJIaMnu please correct me if I'm wrong.

  1. It was introduced in b75168dcc826ff0a74fac6d74ad067fc9fa977bc. Probably as an optimisation?
  2. Then in 2b9e69dbb3f9d7a4c4d05269c9a4492cf49ec01b the other static variable, static auto map = b->graph()->params()->getMap(); in fetchAlphaFromModel(Expr b) disappears when that function is rewritten into a UnaryNodeOp. But the one removed in this pull request remained.
XapaJIaMnu commented 2 years ago

Ah yes, now that the marian instance is kept around, my static optimisations will burn.... And probably a bunch of other things.

And yes the static optimisation was exactly this: avoid doing this check for every affine call as this value would never change once a model is loaded... As long as models are not hot swapped.

This shouldn't show up in the runtime like the calls to shape that you fixed earlier...