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

.Net: Making ContextVariables into a proper Dictionary. #2852

Merged
merged 9 commits into from
Sep 20, 2023
6 changes: 3 additions & 3 deletions dotnet/src/IntegrationTests/Fakes/EmailSkillFake.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ internal sealed class EmailSkillFake
{
[SKFunction, Description("Given an email address and message body, send an email")]
public Task<string> SendEmailAsync(
[Description("The body of the email message to send.")] string input,
[Description("The body of the email message to send.")] string input = "",
markwallace-microsoft marked this conversation as resolved.
Show resolved Hide resolved
[Description("The email address to send email to.")] string? email_address = "default@email.com")
{
email_address ??= string.Empty;
Expand All @@ -20,8 +20,8 @@ internal sealed class EmailSkillFake

[SKFunction, Description("Lookup an email address for a person given a name")]
public Task<string> GetEmailAddressAsync(
[Description("The name of the person to email.")] string input,
ILogger logger)
ILogger logger,
[Description("The name of the person to email.")] string? input = null)
{
if (string.IsNullOrEmpty(input))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ public string MyFunction(string input)
}

[SKFunction, Description("This is a test"), SKName("asis")]
public string MyFunction2(string input)
public string? MyFunction2(string? input = null)
{
return input;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,16 +225,16 @@ public async Task AddEventAsyncWithoutSubjectFailsAsync()

CalendarPlugin target = new(connectorMock.Object);

// Act
var ex = await Assert.ThrowsAsync<ArgumentException>(() => FunctionHelpers.CallViaKernelAsync(target, "AddEvent",
// Act & Assert
var ex = await Assert.ThrowsAsync<SKException>(() => FunctionHelpers.CallViaKernelAsync(target, "AddEvent",
("start", anyStartTime.ToString(CultureInfo.InvariantCulture)),
("end", anyEndTime.ToString(CultureInfo.InvariantCulture)),
("location", anyLocation),
("content", anyContent),
("attendees", string.Join(";", anyAttendees)))
);

// Assert
Assert.Equal("subject", ex.ParamName);
Assert.True(ex.InnerException is ArgumentException);
Assert.Equal("input", ((ArgumentException)ex.InnerException).ParamName);
}
}
Original file line number Diff line number Diff line change
@@ -1,18 +1,11 @@
// Copyright (c) Microsoft. All rights reserved.

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.SemanticKernel.Diagnostics;

#pragma warning disable CA1710 // ContextVariables doesn't end in Dictionary or Collection
#pragma warning disable CA1725, RCS1168 // Uses "name" instead of "key" for some public APIs
#pragma warning disable CS8767 // Reference type nullability doesn't match because netstandard2.0 surface area isn't nullable reference type annotated
// TODO: support more complex data types, and plan for rendering these values into prompt templates.

namespace Microsoft.SemanticKernel.Orchestration;

/// <summary>
Expand All @@ -21,15 +14,16 @@ namespace Microsoft.SemanticKernel.Orchestration;
/// </summary>
[DebuggerDisplay("{DebuggerDisplay,nq}")]
[DebuggerTypeProxy(typeof(ContextVariables.TypeProxy))]
public sealed class ContextVariables : IDictionary<string, string>
public sealed class ContextVariables : Dictionary<string, string>
shawncal marked this conversation as resolved.
Show resolved Hide resolved
{
/// <summary>
/// Constructor for context variables.
/// </summary>
/// <param name="value">Optional value for the main variable of the context including trust information.</param>
public ContextVariables(string? value = null)
: base(StringComparer.OrdinalIgnoreCase)
{
this._variables[MainKey] = value ?? string.Empty;
this.Set(MainKey, value);
}

/// <summary>
Expand All @@ -39,7 +33,7 @@ public ContextVariables(string? value = null)
public ContextVariables Clone()
{
var clone = new ContextVariables();
foreach (KeyValuePair<string, string> x in this._variables)
foreach (KeyValuePair<string, string> x in this)
shawncal marked this conversation as resolved.
Show resolved Hide resolved
{
clone.Set(x.Key, x.Value);
}
Expand All @@ -49,7 +43,7 @@ public ContextVariables Clone()

/// <summary>Gets the main input string.</summary>
/// <remarks>If the main input string was removed from the collection, an empty string will be returned.</remarks>
public string Input => this._variables.TryGetValue(MainKey, out string? value) ? value : string.Empty;
public string Input => this.TryGetValue(MainKey, out string? value) ? value : string.Empty;
shawncal marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Updates the main input text with the new value after a function is complete.
Expand All @@ -59,30 +53,7 @@ public ContextVariables Clone()
/// <returns>The current instance</returns>
public ContextVariables Update(string? value)
{
this._variables[MainKey] = value ?? string.Empty;
return this;
}

/// <summary>
/// Updates all the local data with new data, merging the two datasets.
/// Do not discard old data
/// </summary>
/// <param name="newData">New data to be merged</param>
/// <param name="merge">Whether to merge and keep old data, or replace. False == discard old data.</param>
/// <returns>The current instance</returns>
public ContextVariables Update(ContextVariables newData, bool merge = true)
{
if (!object.ReferenceEquals(this, newData))
{
// If requested, discard old data and keep only the new one.
if (!merge) { this._variables.Clear(); }

foreach (KeyValuePair<string, string> varData in newData._variables)
{
this._variables[varData.Key] = varData.Value;
}
}

this.Set(MainKey, value);
markwallace-microsoft marked this conversation as resolved.
Show resolved Hide resolved
return this;
}

Expand All @@ -98,111 +69,38 @@ public void Set(string name, string? value)
Verify.NotNullOrWhiteSpace(name);
if (value != null)
{
this._variables[name] = value;
this[name] = value;
}
else
{
this._variables.TryRemove(name, out _);
}
}

/// <summary>
/// Gets the variable value associated with the specified name.
/// </summary>
/// <param name="name">The name of the variable to get.</param>
/// <param name="value">
/// When this method returns, contains the variable value associated with the specified name, if the variable is found;
/// otherwise, null.
/// </param>
/// <returns>true if the <see cref="ContextVariables"/> contains a variable with the specified name; otherwise, false.</returns>
public bool TryGetValue(string name, [NotNullWhen(true)] out string? value)
{
if (this._variables.TryGetValue(name, out value))
{
return true;
}

value = null;
return false;
}

/// <summary>
/// Array of all variables in the context variables.
/// </summary>
/// <param name="name">The name of the variable.</param>
/// <returns>The value of the variable.</returns>
public string this[string name]
{
get => this._variables[name];
set
{
this._variables[name] = value;
this.Remove(name);
}
}

/// <summary>
/// Determines whether the <see cref="ContextVariables"/> contains the specified variable.
/// </summary>
/// <param name="name">The name of the variable to locate.</param>
/// <returns>true if the <see cref="ContextVariables"/> contains a variable with the specified name; otherwise, false.</returns>
public bool ContainsKey(string name)
{
return this._variables.ContainsKey(name);
}

/// <summary>
/// Print the processed input, aka the current data after any processing occurred.
/// </summary>
/// <returns>Processed input, aka result</returns>
public override string ToString() => this.Input;

/// <summary>
/// Get an enumerator that iterates through the context variables.
/// </summary>
/// <returns>An enumerator that iterates through the context variables</returns>
public IEnumerator<KeyValuePair<string, string>> GetEnumerator() => this._variables.GetEnumerator();

IEnumerator IEnumerable.GetEnumerator() => this._variables.GetEnumerator();
void IDictionary<string, string>.Add(string key, string value) => ((IDictionary<string, string>)this._variables).Add(key, value);
bool IDictionary<string, string>.Remove(string key) => ((IDictionary<string, string>)this._variables).Remove(key);
void ICollection<KeyValuePair<string, string>>.Add(KeyValuePair<string, string> item) => ((ICollection<KeyValuePair<string, string>>)this._variables).Add(item);
void ICollection<KeyValuePair<string, string>>.Clear() => ((ICollection<KeyValuePair<string, string>>)this._variables).Clear();
bool ICollection<KeyValuePair<string, string>>.Contains(KeyValuePair<string, string> item) => ((ICollection<KeyValuePair<string, string>>)this._variables).Contains(item);
void ICollection<KeyValuePair<string, string>>.CopyTo(KeyValuePair<string, string>[] array, int arrayIndex) => ((ICollection<KeyValuePair<string, string>>)this._variables).CopyTo(array, arrayIndex);
bool ICollection<KeyValuePair<string, string>>.Remove(KeyValuePair<string, string> item) => ((ICollection<KeyValuePair<string, string>>)this._variables).Remove(item);
ICollection<string> IDictionary<string, string>.Keys => ((IDictionary<string, string>)this._variables).Keys;
ICollection<string> IDictionary<string, string>.Values => ((IDictionary<string, string>)this._variables).Values;
int ICollection<KeyValuePair<string, string>>.Count => ((ICollection<KeyValuePair<string, string>>)this._variables).Count;
bool ICollection<KeyValuePair<string, string>>.IsReadOnly => ((ICollection<KeyValuePair<string, string>>)this._variables).IsReadOnly;
string IDictionary<string, string>.this[string key]
{
get => ((IDictionary<string, string>)this._variables)[key];
set => ((IDictionary<string, string>)this._variables)[key] = value;
}

internal const string MainKey = "INPUT";

[DebuggerBrowsable(DebuggerBrowsableState.Never)]
internal string DebuggerDisplay =>
this.TryGetValue(MainKey, out string? input) && !string.IsNullOrEmpty(input)
? $"Variables = {this._variables.Count}, Input = {input}"
: $"Variables = {this._variables.Count}";
? $"Variables = {this.Count}, Input = {input}"
: $"Variables = {this.Count}";

#region private ================================================================================

/// <summary>
/// Important: names are case insensitive
/// </summary>
private readonly ConcurrentDictionary<string, string> _variables = new(StringComparer.OrdinalIgnoreCase);

private sealed class TypeProxy
{
private readonly ContextVariables _variables;

public TypeProxy(ContextVariables variables) => this._variables = variables;

[DebuggerBrowsable(DebuggerBrowsableState.RootHidden)]
public KeyValuePair<string, string>[] Items => this._variables._variables.ToArray();
public KeyValuePair<string, string>[] Items => this._variables.ToArray();
}

#endregion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ public void ReadFromJsonSucceeds()

// Assert
Assert.Equal("b", result!["a"]);
Assert.Equal(string.Empty, result!["INPUT"]);
Assert.Throws<KeyNotFoundException>(() => result!["INPUT"]);
Assert.Equal(string.Empty, result!.Input);
}

[Fact]
Expand Down Expand Up @@ -60,10 +61,6 @@ public void ReadFromJsonSucceedsWithInput()
// input value
// params key/value
[Theory]
[InlineData(null, new[] { "a", "b" }, new[]
{
/*lang=json,strict*/ @"{""Key"":""INPUT"",""Value"":""""}", /*lang=json,strict*/ @"{""Key"":""a"",""Value"":""b""}"
})]
[InlineData("", new[] { "a", "b" }, new[]
{
/*lang=json,strict*/ @"{""Key"":""INPUT"",""Value"":""""}", /*lang=json,strict*/ @"{""Key"":""a"",""Value"":""b""}"
Expand Down Expand Up @@ -157,7 +154,8 @@ public void ReadFromJsonReturnsDefaultWithEmpty()

// Assert
Assert.NotNull(result);
Assert.Equal(string.Empty, result!["INPUT"]);
Assert.Throws<KeyNotFoundException>(() => result!["INPUT"]);
Assert.Equal(string.Empty, result!.Input);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,14 +248,13 @@ public void UpdateOriginalDoesNotAffectClonedSucceeds()
string anyContent = Guid.NewGuid().ToString();
string someOtherMainContent = Guid.NewGuid().ToString();
string someOtherContent = Guid.NewGuid().ToString();
ContextVariables target = new();
ContextVariables original = new(mainContent);

ContextVariables original = new(mainContent);
original.Set(anyName, anyContent);

// Act
// Clone original into target
target.Update(original);
ContextVariables target = original.Clone();
// Update original
original.Update(someOtherMainContent);
original.Set(anyName, someOtherContent);
Expand Down
2 changes: 1 addition & 1 deletion dotnet/src/SemanticKernel/Planning/Plan.cs
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ private ContextVariables GetNextStepVariables(ContextVariables variables, Plan s
var input = string.Empty;
if (!string.IsNullOrEmpty(step.Parameters.Input))
{
input = this.ExpandFromVariables(variables, step.Parameters.Input);
input = this.ExpandFromVariables(variables, step.Parameters.Input!);
}
else if (!string.IsNullOrEmpty(variables.Input))
{
Expand Down
5 changes: 3 additions & 2 deletions dotnet/src/SemanticKernel/SkillDefinition/NativeFunction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -481,9 +481,10 @@ Task<SKContext> function(ITextCompletion? text, AIRequestSettings? requestSettin
}

// 4. Otherwise, fail.
throw new SKException($"Missing value for parameter '{name}'");
throw new SKException($"Missing value for parameter '{name}'",
new ArgumentException("Missing value function parameter", name));

object? Process(string value)
object ? Process(string value)
{
if (type == typeof(string))
{
Expand Down