Skip to content

Commit

Permalink
.Net: Obsolete the useless SetDefaultFunctionCollection (microsoft#3183)
Browse files Browse the repository at this point in the history
### Description

`ISKFunction` exposes a `SetDefaultFunctionCollection`. On all
implementations it's either a nop or just delegates to the same
interface method on a wrapped object. The one exception to that is
`SemanticFunction`, which stores the argument into a field _that then is
never used_.

### 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 😄

Co-authored-by: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com>
  • Loading branch information
2 people authored and SOE-YoungS committed Oct 31, 2023
1 parent 69f0e7f commit fbec362
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 54 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,6 @@ private static ISKFunction CreateSemanticFunction(
kernel.LoggerFactory
);

// Connect the function to the current kernel function collection, in case the function
// is invoked manually without a context and without a way to find other functions.
func.SetDefaultFunctionCollection(kernel.Functions);

func.SetAIConfiguration(promptTemplateConfig.GetDefaultRequestSettings());

// Note: the service is instantiated using the kernel configuration state when the function is invoked
Expand Down
17 changes: 7 additions & 10 deletions dotnet/src/Functions/Functions.Semantic/SemanticFunction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,6 @@ public async Task<FunctionResult> InvokeAsync(
return await this.RunPromptAsync(this._aiService?.Value, requestSettings ?? this.RequestSettings, context, cancellationToken).ConfigureAwait(false);
}

/// <inheritdoc/>
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions)
{
this._functionCollection = functions;
return this;
}

/// <inheritdoc/>
public ISKFunction SetAIService(Func<ITextCompletion> serviceFactory)
{
Expand Down Expand Up @@ -169,7 +162,6 @@ internal SemanticFunction(
private static readonly JsonSerializerOptions s_toStringStandardSerialization = new();
private static readonly JsonSerializerOptions s_toStringIndentedSerialization = new() { WriteIndented = true };
private readonly ILogger _logger;
private IReadOnlyFunctionCollection? _functionCollection;
private Lazy<ITextCompletion>? _aiService;
private readonly Lazy<FunctionView> _view;
public IPromptTemplate _promptTemplate { get; }
Expand Down Expand Up @@ -244,9 +236,14 @@ private async Task<FunctionResult> RunPromptAsync(
public bool IsSemantic => true;

/// <inheritdoc/>
[Obsolete("Methods, properties and classes which include Skill in the name have been renamed. Use ISKFunction.SetDefaultFunctionCollection instead. This will be removed in a future release.")]
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this;

/// <inheritdoc/>
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this.SetDefaultFunctionCollection(skills);
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions) => this;

#endregion
}
20 changes: 11 additions & 9 deletions dotnet/src/SemanticKernel.Abstractions/Functions/ISKFunction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,6 @@ Task<FunctionResult> InvokeAsync(
AIRequestSettings? requestSettings = null,
CancellationToken cancellationToken = default);

/// <summary>
/// Set the default function collection to use when the function is invoked
/// without a context or with a context that doesn't have a collection.
/// </summary>
/// <param name="functions">Kernel's function collection</param>
/// <returns>Self instance</returns>
ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions);

/// <summary>
/// Set the AI service used by the semantic function, passing a factory method.
/// The factory allows to lazily instantiate the client and to properly handle its disposal.
Expand Down Expand Up @@ -94,10 +86,20 @@ Task<FunctionResult> InvokeAsync(
/// </summary>
/// <param name="skills">Kernel's function collection</param>
/// <returns>Self instance</returns>
[Obsolete("Methods, properties and classes which include Skill in the name have been renamed. Use ISKFunction.SetDefaultFunctionCollection instead. This will be removed in a future release.")]
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills);

/// <summary>
/// Set the default function collection to use when the function is invoked
/// without a context or with a context that doesn't have a collection.
/// </summary>
/// <param name="functions">Kernel's function collection</param>
/// <returns>Self instance</returns>
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions);

/// <summary>
/// Whether the function is defined using a prompt template.
/// IMPORTANT: native functions might use semantic functions internally,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,6 @@ public ISKFunction SetAIConfiguration(AIRequestSettings? requestSettings) =>
public ISKFunction SetAIService(Func<ITextCompletion> serviceFactory) =>
this._function.SetAIService(serviceFactory);

/// <inheritdoc/>
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions) =>
this._function.SetDefaultFunctionCollection(functions);

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

private readonly ISKFunction _function;
Expand Down Expand Up @@ -189,9 +185,14 @@ private async Task<FunctionResult> InvokeWithInstrumentationAsync(Func<Task<Func
public bool IsSemantic => this._function.IsSemantic;

/// <inheritdoc/>
[Obsolete("Methods, properties and classes which include Skill in the name have been renamed. Use ISKFunction.SetDefaultFunctionCollection instead. This will be removed in a future release.")]
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this;

/// <inheritdoc/>
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this._function.SetDefaultFunctionCollection(skills);
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions) => this;

#endregion
}
17 changes: 7 additions & 10 deletions dotnet/src/SemanticKernel.Core/Functions/NativeFunction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,6 @@ public async Task<FunctionResult> InvokeAsync(
}
}

/// <inheritdoc/>
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions)
{
// No-op for native functions; do not throw, as both Plan and PromptFunctions use this,
// and we don't have a way to distinguish between a native function and a Plan.
return this;
}

/// <inheritdoc/>
public ISKFunction SetAIService(Func<ITextCompletion> serviceFactory)
{
Expand Down Expand Up @@ -892,9 +884,14 @@ private static string SanitizeMetadataName(string methodName) =>
public bool IsSemantic => true;

/// <inheritdoc/>
[Obsolete("Methods, properties and classes which include Skill in the name have been renamed. Use ISKFunction.SetDefaultFunctionCollection instead. This will be removed in a future release.")]
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this;

/// <inheritdoc/>
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this.SetDefaultFunctionCollection(skills);
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions) => this;

#endregion
}
1 change: 0 additions & 1 deletion dotnet/src/SemanticKernel.Core/Kernel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ public ISKFunction RegisterCustomFunction(ISKFunction customFunction)
{
Verify.NotNull(customFunction);

customFunction.SetDefaultFunctionCollection(this.Functions);
this._functionCollection.AddFunction(customFunction);

return customFunction;
Expand Down
13 changes: 7 additions & 6 deletions dotnet/src/SemanticKernel.Core/Planning/InstrumentedPlan.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,6 @@ public ISKFunction SetAIConfiguration(AIRequestSettings? requestSettings) =>
public ISKFunction SetAIService(Func<ITextCompletion> serviceFactory) =>
this._plan.SetAIService(serviceFactory);

/// <inheritdoc/>
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions) =>
this._plan.SetDefaultFunctionCollection(functions);

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

private readonly IPlan _plan;
Expand Down Expand Up @@ -170,9 +166,14 @@ private async Task<FunctionResult> InvokeWithInstrumentationAsync(Func<Task<Func
public bool IsSemantic => this._plan.IsSemantic;

/// <inheritdoc/>
[Obsolete("Methods, properties and classes which include Skill in the name have been renamed. Use ISKFunction.SetDefaultFunctionCollection instead. This will be removed in a future release.")]
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this;

/// <inheritdoc/>
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this._plan.SetDefaultFunctionCollection(skills);
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions) => this;

#endregion
}
15 changes: 7 additions & 8 deletions dotnet/src/SemanticKernel.Core/Planning/Plan.cs
Original file line number Diff line number Diff line change
Expand Up @@ -359,12 +359,6 @@ public async Task<FunctionResult> InvokeAsync(
return result;
}

/// <inheritdoc/>
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions)
{
return this.Function is not null ? this.Function.SetDefaultFunctionCollection(functions) : this;
}

/// <inheritdoc/>
public ISKFunction SetAIService(Func<ITextCompletion> serviceFactory)
{
Expand Down Expand Up @@ -656,9 +650,14 @@ private string DebuggerDisplay
public bool IsSemantic { get; private set; }

/// <inheritdoc/>
[Obsolete("Methods, properties and classes which include Skill in the name have been renamed. Use ISKFunction.SetDefaultFunctionCollection instead. This will be removed in a future release.")]
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this;

/// <inheritdoc/>
[Obsolete("This method is a nop and will be removed in a future release.")]
[EditorBrowsable(EditorBrowsableState.Never)]
public ISKFunction SetDefaultSkillCollection(IReadOnlyFunctionCollection skills) => this.SetDefaultFunctionCollection(skills);
public ISKFunction SetDefaultFunctionCollection(IReadOnlyFunctionCollection functions) => this;

#endregion
}

0 comments on commit fbec362

Please sign in to comment.