semantic-kernel .Net: Inconsistent Temperature data type in PromptExecutionSettings classes

Working with SK it came to my attention that the data type of the Temperature property in PromptExecutionSettings classes is inconsistent. Some have the data type float others have double.

Is that intentional? When not wouldn't it make sense to have the same data type in all PromptExecutionSettings classes? As the expected values are mostly in range of -1.0 to 1.0 it would make sens to use the float data type (float 32bit vs double 64 bit). If wished I could provide PR for this issue. Thank you for consideration.

float

double

Sep 05 '24 14:09 akordowski

@akordowski - thanks for reaching out on this. Each connector has its own data type and matches the type by the underlining service. We abstract on top of these.

Sep 09 '24 15:09 evchaki

@evchaki Thank you for your reponse.

Each connector has its own data type and matches the type by the underlining service. We abstract on top of these.

After investigating the source code this statement is not quite true. For the OpenAIPromptExecutionSettings class the double values are cast from double to float as you can see below:

https://github.com/microsoft/semantic-kernel/blob/642691158c17c74499039c8c87867cebff042781/dotnet/src/Connectors/Connectors.AzureOpenAI/Core/AzureClientCore.ChatCompletion.cs#L39-L50

And for the GeminiPromptExecutionSettings and MistralAIPromptExecutionSettings classes the values are converted to JSON strings. As we know that the values are at so low range that we don't have to expect an overflow, I would recommend to streamline the data type for a cleaner design. It may not have not that big impact on memory, but why to cast double to float when there is no explicit necessity for it. What do you think?

Sep 09 '24 17:09 akordowski

This issue is stale because it has been open for 90 days with no activity.

Dec 09 '24 02:12 github-actions[bot]

@evchaki Any remarks regarding my response?

Dec 09 '24 11:12 akordowski

@akordowski I think your concerns are valid. On the other hand, I'm not sure if it will be possible to update property type at this point, since it's going to be a breaking change. The alternative solution would be to mark current property as obsolete and introduce another one with correct type, but we will have to use new name for that property, and this is a problem, because temperature is a correct name, and I can't think about an alternative name as for now.

Dec 10 '24 14:12 dmytrostruk

@dmytrostruk Thank you for your response.

I'm not sure if it will be possible to update property type at this point, since it's going to be a breaking change.

Do you see the breaking change on the property type side or otherwise?
To my knowledge the temperature values can not exced the float 32bit value, which implications do you see?

The only errors I can imagine would be compile/cast errors. As the SK project is still experimental I find it justified to fix the issue and don't drag the "technical" debt in future versions. The changes could be communicated through release notes or something similar.

If the team is willing to accept a fix regarding this issue I would love to provide a PR. Feel free to let me know. Thank you!

Dec 10 '24 16:12 akordowski

@akordowski Thanks for your reply!

Do you see the breaking change on the property type side or otherwise?

That's correct, property type.

The only errors I can imagine would be compile/cast errors. As the SK project is still experimental I find it justified to fix the issue and don't drag the "technical" debt in future versions. The changes could be communicated through release notes or something similar.

While some of SK functionality is marked as experimental, the functionality for execution settings, and specifically Temperature property is not experimental. We are trying to avoid delivering compilation errors for non-experimental functionality, unless it's something critical or it is experimental.

I'm not sure this issue is critical in the way that we want to provide a breaking change for it, but we can discuss it with internal team and make some decision. I think this property was added at the very early stage of Semantic Kernel, so maybe back then it wasn't clear which values it will accept in the future.

If the team is willing to accept a fix regarding this issue I would love to provide a PR

Your contribution is much appreciated and in case we decide to proceed with this change, we will definitely let you know.

Thank you!

Dec 11 '24 00:12 dmytrostruk

@dmytrostruk Thank you for the clarification.

We are trying to avoid delivering compilation errors for non-experimental functionality, unless it's something critical or it is experimental.

I would like to point out, that I already expierienced some breaking changes (as you can read here #8658) which were more sever than a data type change. But I understand and respect the teams decision.

but we can discuss it with internal team and make some decision.

Thank you very much! Looking forward to the teams decision.

Dec 11 '24 06:12 akordowski

This issue is stale because it has been open for 90 days with no activity.

Apr 06 '25 02:04 github-actions[bot]

@dmytrostruk

but we can discuss it with internal team and make some decision.

Any news regarding this topic?

Apr 06 '25 13:04 akordowski