Remove additional float/clone() for perf

[jiminha]

What does this PR do?

Remove float() from 4.45 upgrade due to perf issue
Extra aten::item, cast is causing perf degradation

DONOTMERGE NOW : Need accuracy test to see if there is any accuracy drop.

[mgonchar]

well, I think you should at least remove this comment

[regisss]

@jiminha Have you got time to check if we get the same output sequences removing all these .float()?

[jiminha]

@jiminha Have you got time to check if we get the same output sequences removing all these .float()?

I tested all text_generation and test_encoder_decoder test cases from pytest and compared the outputs and it all looked the same. We'd like to understand more though why the original transformer added this float() for all logits' last output computation, what specific test cases that they ran and found this float() is needed. Would you be able to check with them?

[regisss]

@jiminha It's explained in the first message of this issue: huggingface/transformers#30860

Basically, we don't want to cast all the logits to float in the forward causal-lm modelsand we only do it:

• if labels are provided to the forward
• and in generate for the last logit which is needed for computing the next token

So we should probably remove this float here and keep the ones in generate. It's not done in this version of Transformers because it's a breaking change (the model outputs won't have the same type anymore) but they'll do it in v4.46.

[jiminha]

@jiminha It's explained in the first message of this issue: huggingface/transformers#30860

Basically, we don't want to cast all the logits to float in the forward causal-lm modelsand we only do it:

• if labels are provided to the forward
• and in generate for the last logit which is needed for computing the next token

So we should probably remove this float here and keep the ones in generate. It's not done in this version of Transformers because it's a breaking change (the model outputs won't have the same type anymore) but they'll do it in v4.46.

Thanks for the link and explanation. So are we actually having double float cast on this v4.45 in the code since we added float() on the generation/utils.py and also kept the same logic in the model(causal LM) file as well? Maybe that's why I didn't see any regression even after removing float() on the util.

The perf regression what I saw was in this t5 model (t5 test_encoder_decoder test) which doesn't use the causalLM and didn't have float conversion to begin with, so the duration increased with this change.

https://github.com/huggingface/transformers/blob/main/src/transformers/models/t5/modeling_t5.py#L1768C8-L1768C50

[libinta]

consider keeping this

[libinta]

we don't normally run to here

[jiminha]

For T5 model, I think it shows bigger diff in the perf in smaller batch/samples. For bigger batch/samples the difference is very small.

Batch2, sample 200 (current test)

Original
predict_runtime = 0:00:17.73
predict_samples = 200
predict_samples_per_second = 11.28

remove float from _sample

predict_runtime = 0:00:16.51
predict_samples = 200
predict_samples_per_second = 12.108

**bs128/ sample 1000 **

Original
predict_runtime = 0:00:23.91
predict_samples = 1000
predict_samples_per_second = 41.814

Remove float from _sample
predict_runtime = 0:00:23.86
predict_samples = 1000
predict_samples_per_second = 41.909

[regisss]

For T5 it would be in T5ForConditionalGeneration but it's true that I don't see any cast to float in the forward.
Let me try with a check on the model type in generate.

[jiminha]

Closing this PR since it's not adding much perf improvement for most of the model. T5 has small regression, but it's very little.