Skip to content

Commit

Permalink
.Net: Making ContextVariables into a proper Dictionary. (#2852)
Browse files Browse the repository at this point in the history
### Description
Simplifying ContextVariables by making it into an actual
Dictionary<string, string> instead of a reimplemenation.

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [X] The code builds clean without any errors or warnings
- [X] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [X] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄
- BREAKING CHANGE - use normal Dictionary methods instead. Behavior
change: retrieving missing values via indexer [key] will throw if
argument is missing, instead of returning string.Empty.

---------

Co-authored-by: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com>
  • Loading branch information
shawncal and RogerBarreto committed Sep 20, 2023
1 parent 1425d0d commit e221256
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 134 deletions.
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 = "",
[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);
}
}
126 changes: 12 additions & 114 deletions dotnet/src/SemanticKernel.Abstractions/Orchestration/ContextVariables.cs
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>
{
/// <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)
{
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;

/// <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);
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

0 comments on commit e221256

Please sign in to comment.