Remove static precomputedAlphas bool in affine operation#78
Merged
XapaJIaMnu merged 1 commit intobrowsermt:masterfrom Mar 9, 2022
Merged
Remove static precomputedAlphas bool in affine operation#78XapaJIaMnu merged 1 commit intobrowsermt:masterfrom
static precomputedAlphas bool in affine operation#78XapaJIaMnu merged 1 commit intobrowsermt:masterfrom
Conversation
`b->graph()` changes when switching models in bergamot-translator. That graph can have a new backend that has a different gemm precision specified.
Collaborator
|
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... |
XapaJIaMnu
approved these changes
Mar 9, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a bug when switching between translation models that have different
gemm-precisionprecisions specified in their configuration file. Which is why we never noticed it with our own student models (allint8shiftAlpha), but do see it now with the Ukrainian model (which uses justint8)I tried to go through git blame to figure out why it was marked
static. @XapaJIaMnu please correct me if I'm wrong.static auto map = b->graph()->params()->getMap();infetchAlphaFromModel(Expr b)disappears when that function is rewritten into aUnaryNodeOp. But the one removed in this pull request remained.