Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AdditionalPropertiesDictionary.TryGetValue<T> #5528

Merged
merged 2 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions eng/MSBuild/Shared.props
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
<Project>
<ItemGroup Condition="'$(InjectSharedCollectionExtensions)' == 'true'">
<Compile Include="$(MSBuildThisFileDirectory)\..\..\src\Shared\CollectionExtensions\*.cs" LinkBase="Shared\CollectionExtensions" />
</ItemGroup>

<ItemGroup Condition="'$(InjectSharedDiagnosticIds)' == 'true'">
<Compile Include="$(MSBuildThisFileDirectory)\..\..\src\Shared\DiagnosticIds\*.cs" LinkBase="Shared\DiagnosticIds" />
</ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;

namespace Microsoft.Extensions.AI;
Expand Down Expand Up @@ -45,7 +47,7 @@ public AdditionalPropertiesDictionary(IEnumerable<KeyValuePair<string, object?>>
/// A shallow clone of the properties dictionary. The instance will not be the same as the current instance,
/// but it will contain all of the same key-value pairs.
/// </returns>
public AdditionalPropertiesDictionary Clone() => new AdditionalPropertiesDictionary(_dictionary);
public AdditionalPropertiesDictionary Clone() => new(_dictionary);

/// <inheritdoc />
public object? this[string key]
Expand Down Expand Up @@ -94,6 +96,9 @@ public object? this[string key]
/// <inheritdoc />
public IEnumerator<KeyValuePair<string, object?>> GetEnumerator() => _dictionary.GetEnumerator();

/// <inheritdoc />
IEnumerator IEnumerable.GetEnumerator() => _dictionary.GetEnumerator();

/// <inheritdoc />
public bool Remove(string key) => _dictionary.Remove(key);

Expand All @@ -103,6 +108,52 @@ public object? this[string key]
/// <inheritdoc />
public bool TryGetValue(string key, out object? value) => _dictionary.TryGetValue(key, out value);

/// <inheritdoc />
IEnumerator IEnumerable.GetEnumerator() => _dictionary.GetEnumerator();
/// <summary>Attempts to extract a typed value from the dictionary.</summary>
/// <typeparam name="T">Specifies the type of the value to be retrieved.</typeparam>
/// <param name="key">The key to locate.</param>
/// <param name="value">
/// The value retrieved from the dictionary, if found and successfully converted to the requested type;
/// otherwise, the default value of <typeparamref name="T"/>.
/// </param>
/// <returns>
/// <see langword="true"/> if a non-<see langword="null"/> value was found for <paramref name="key"/>
/// in the dictionary and converted to the requested type; otherwise, <see langword="false"/>.
/// </returns>
/// <remarks>
/// If a non-<see langword="null"/> is found for the key in the dictionary, but the value is not of the requested type but is
/// an <see cref="IConvertible"/> object, the method will attempt to convert the object to the requested type.
/// </remarks>
public bool TryGetValue<T>(string key, [NotNullWhen(true)] out T? value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we explicitly ban nullable type parameters?

Suggested change
public bool TryGetValue<T>(string key, [NotNullWhen(true)] out T? value)
public bool TryGetValue<T>(string key, [NotNullWhen(true)] out T? value) where T : notnull

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that help? The parameter is already nullable in the case of reference types.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It explicitly prevents passing nullable type parameters even if it doesn't change the nullability of the out parameter.

Copy link
Member Author

@stephentoub stephentoub Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but what benefit does that provide here? What errors does it avoid? It seems like it'd only be a hindrance here, eg not being able to use this in a method with an unconstrained T. This T is only ever in an out position, and the out is already nullable regardless of the T.

{
if (TryGetValue(key, out object? obj))
{
switch (obj)
{
case T t:
// The object is already of the requested type. Return it.
value = t;
return true;

case IConvertible:
// The object is convertible; try to convert it to the requested type. Unfortunately, there's no
// convenient way to do this that avoids exceptions and that doesn't involve a ton of boilerplate,
// so we only try when the source object is at least an IConvertible, which is what ChangeType uses.
try
{
value = (T)Convert.ChangeType(obj, typeof(T), CultureInfo.InvariantCulture);
return true;
}
catch (Exception e) when (e is ArgumentException or FormatException or InvalidCastException or OverflowException)
{
// Ignore known failure modes.
}

break;
}
}

// Unable to find the value or convert it to the requested type.
value = default;
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
</PropertyGroup>

<PropertyGroup>
<InjectSharedCollectionExtensions>true</InjectSharedCollectionExtensions>
<InjectSharedEmptyCollections>true</InjectSharedEmptyCollections>
<InjectStringHashOnLegacy>true</InjectStringHashOnLegacy>
<InjectStringSyntaxAttributeOnLegacy>true</InjectStringSyntaxAttributeOnLegacy>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

<PropertyGroup>
<InjectCompilerFeatureRequiredOnLegacy>true</InjectCompilerFeatureRequiredOnLegacy>
<InjectSharedCollectionExtensions>true</InjectSharedCollectionExtensions>
<InjectSharedDiagnosticIds>true</InjectSharedDiagnosticIds>
<InjectSharedEmptyCollections>true</InjectSharedEmptyCollections>
<InjectStringHashOnLegacy>true</InjectStringHashOnLegacy>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
<PropertyGroup>
<InjectCompilerFeatureRequiredOnLegacy>true</InjectCompilerFeatureRequiredOnLegacy>
<InjectRequiredMemberOnLegacy>true</InjectRequiredMemberOnLegacy>
<InjectSharedCollectionExtensions>true</InjectSharedCollectionExtensions>
<InjectSharedDiagnosticIds>true</InjectSharedDiagnosticIds>
<InjectSharedEmptyCollections>true</InjectSharedEmptyCollections>
<InjectStringHashOnLegacy>true</InjectStringHashOnLegacy>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Shared.Collections;
using Microsoft.Shared.Diagnostics;

#pragma warning disable EA0011 // Consider removing unnecessary conditional access operator (?)
Expand Down Expand Up @@ -298,7 +297,7 @@ private OllamaChatRequest ToOllamaChatRequest(IList<ChatMessage> chatMessages, C

void TransferMetadataValue<T>(string propertyName, Action<OllamaRequestOptions, T> setOption)
{
if (options.AdditionalProperties?.TryGetConvertedValue(propertyName, out T? t) is true)
if (options.AdditionalProperties?.TryGetValue(propertyName, out T? t) is true)
{
request.Options ??= new();
setOption(request.Options, t);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using System.Net.Http.Json;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Shared.Collections;
using Microsoft.Shared.Diagnostics;

namespace Microsoft.Extensions.AI;
Expand Down Expand Up @@ -75,12 +74,12 @@ public async Task<GeneratedEmbeddings<Embedding<float>>> GenerateAsync(IEnumerab

if (options?.AdditionalProperties is { } requestProps)
{
if (requestProps.TryGetConvertedValue("keep_alive", out long keepAlive))
if (requestProps.TryGetValue("keep_alive", out long keepAlive))
{
request.KeepAlive = keepAlive;
}

if (requestProps.TryGetConvertedValue("truncate", out bool truncate))
if (requestProps.TryGetValue("truncate", out bool truncate))
{
request.Truncate = truncate;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
</PropertyGroup>

<PropertyGroup>
<InjectSharedCollectionExtensions>true</InjectSharedCollectionExtensions>
<InjectSharedEmptyCollections>true</InjectSharedEmptyCollections>
<InjectStringHashOnLegacy>true</InjectStringHashOnLegacy>
</PropertyGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
using System.Text.Json.Serialization;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Shared.Collections;
using Microsoft.Shared.Diagnostics;
using OpenAI;
using OpenAI.Chat;
Expand Down Expand Up @@ -410,37 +409,37 @@ private ChatCompletionOptions ToOpenAIOptions(ChatOptions? options)

if (options.AdditionalProperties is { Count: > 0 } additionalProperties)
{
if (additionalProperties.TryGetConvertedValue(nameof(result.EndUserId), out string? endUserId))
if (additionalProperties.TryGetValue(nameof(result.EndUserId), out string? endUserId))
{
result.EndUserId = endUserId;
}

if (additionalProperties.TryGetConvertedValue(nameof(result.IncludeLogProbabilities), out bool includeLogProbabilities))
if (additionalProperties.TryGetValue(nameof(result.IncludeLogProbabilities), out bool includeLogProbabilities))
{
result.IncludeLogProbabilities = includeLogProbabilities;
}

if (additionalProperties.TryGetConvertedValue(nameof(result.LogitBiases), out IDictionary<int, int>? logitBiases))
if (additionalProperties.TryGetValue(nameof(result.LogitBiases), out IDictionary<int, int>? logitBiases))
{
foreach (KeyValuePair<int, int> kvp in logitBiases!)
{
result.LogitBiases[kvp.Key] = kvp.Value;
}
}

if (additionalProperties.TryGetConvertedValue(nameof(result.AllowParallelToolCalls), out bool allowParallelToolCalls))
if (additionalProperties.TryGetValue(nameof(result.AllowParallelToolCalls), out bool allowParallelToolCalls))
{
result.AllowParallelToolCalls = allowParallelToolCalls;
}

#pragma warning disable OPENAI001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
if (additionalProperties.TryGetConvertedValue(nameof(result.Seed), out long seed))
if (additionalProperties.TryGetValue(nameof(result.Seed), out long seed))
{
result.Seed = seed;
}
#pragma warning restore OPENAI001

if (additionalProperties.TryGetConvertedValue(nameof(result.TopLogProbabilityCount), out int topLogProbabilityCountInt))
if (additionalProperties.TryGetValue(nameof(result.TopLogProbabilityCount), out int topLogProbabilityCountInt))
{
result.TopLogProbabilityCount = topLogProbabilityCountInt;
}
Expand Down Expand Up @@ -488,7 +487,10 @@ private ChatCompletionOptions ToOpenAIOptions(ChatOptions? options)
/// <summary>Converts an Extensions function to an OpenAI chat tool.</summary>
private ChatTool ToOpenAIChatTool(AIFunction aiFunction)
{
_ = aiFunction.Metadata.AdditionalProperties.TryGetConvertedValue("Strict", out bool strict);
bool? strict =
aiFunction.Metadata.AdditionalProperties.TryGetValue("Strict", out object? strictObj) &&
strictObj is bool strictValue ?
strictValue : null;

BinaryData resultParameters = OpenAIChatToolJson.ZeroFunctionParametersSchema;

Expand Down Expand Up @@ -643,7 +645,7 @@ private sealed class OpenAIChatToolJson
new(toolCalls.Values) { ParticipantName = input.AuthorName } :
new(input.Text) { ParticipantName = input.AuthorName };

if (input.AdditionalProperties?.TryGetConvertedValue(nameof(message.Refusal), out string? refusal) is true)
if (input.AdditionalProperties?.TryGetValue(nameof(message.Refusal), out string? refusal) is true)
{
message.Refusal = refusal;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Shared.Collections;
using Microsoft.Shared.Diagnostics;
using OpenAI;
using OpenAI.Embeddings;
Expand Down Expand Up @@ -144,12 +143,12 @@ void IDisposable.Dispose()
if (options?.AdditionalProperties is { Count: > 0 } additionalProperties)
{
// Allow per-instance dimensions to be overridden by a per-call property
if (additionalProperties.TryGetConvertedValue(nameof(openAIOptions.Dimensions), out int? dimensions))
if (additionalProperties.TryGetValue(nameof(openAIOptions.Dimensions), out int? dimensions))
{
openAIOptions.Dimensions = dimensions;
}

if (additionalProperties.TryGetConvertedValue(nameof(openAIOptions.EndUserId), out string? endUserId))
if (additionalProperties.TryGetValue(nameof(openAIOptions.EndUserId), out string? endUserId))
{
openAIOptions.EndUserId = endUserId;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
using System.Text.Json;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Shared.Collections;
using Microsoft.Shared.Diagnostics;

namespace Microsoft.Extensions.AI;
Expand Down Expand Up @@ -308,7 +307,7 @@ private static Dictionary<int, List<StreamingChatCompletionUpdate>> OrganizeStre
_ = activity.SetTag(OpenTelemetryConsts.GenAI.Request.Temperature, temperature);
}

if (options.AdditionalProperties?.TryGetConvertedValue("top_k", out double topK) is true)
if (options.AdditionalProperties?.TryGetValue("top_k", out double topK) is true)
{
_ = activity.SetTag(OpenTelemetryConsts.GenAI.Request.TopK, topK);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
</PropertyGroup>

<PropertyGroup>
<InjectSharedCollectionExtensions>true</InjectSharedCollectionExtensions>
<InjectSharedEmptyCollections>true</InjectSharedEmptyCollections>
<DisableMicrosoftExtensionsLoggingSourceGenerator>false</DisableMicrosoftExtensionsLoggingSourceGenerator>
</PropertyGroup>
Expand Down
72 changes: 0 additions & 72 deletions src/Shared/CollectionExtensions/CollectionExtensions.cs

This file was deleted.

11 changes: 0 additions & 11 deletions src/Shared/CollectionExtensions/README.md

This file was deleted.

Loading
Loading