-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
SequentialPlanner - Allow custom manual discovery and loading. #1727
Merged
lemillermicrosoft
merged 7 commits into
microsoft:main
from
lemillermicrosoft:626_sequential_config
Jun 27, 2023
Merged
SequentialPlanner - Allow custom manual discovery and loading. #1727
lemillermicrosoft
merged 7 commits into
microsoft:main
from
lemillermicrosoft:626_sequential_config
Jun 27, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
.NET
Issue or Pull requests regarding .NET code
kernel
Issues or pull requests impacting the core kernel
labels
Jun 27, 2023
5 tasks
5 tasks
dmytrostruk
previously approved these changes
Jun 27, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of small formatting/convention comments, in general LGTM!
dotnet/src/Extensions/Planning.SequentialPlanner/SKContextSequentialPlannerExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Extensions/Planning.SequentialPlanner/SequentialPlanParser.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Extensions/Planning.SequentialPlanner/SequentialPlanner.cs
Outdated
Show resolved
Hide resolved
This commit refactors the SequentialPlanParser class to simplify the logic and avoid catching and rethrowing exceptions. It also adds a GetFunction option to the SequentialPlannerConfig class, which allows the caller to provide a custom function to get a function by name. This can be useful for scenarios where the available functions are not statically defined or need to be resolved dynamically. Additionally, this commit changes the stop sequence for the plan parsing to be "<!-- END -->" instead of just "<!--", to avoid ambiguity with other comments. The commit also refactors the plan parsing logic to use a function delegate instead of a SKContext parameter. This allows for more flexibility and customization in how the skill functions are retrieved. The function delegate can be provided by the SequentialPlannerConfig or by a default implementation that uses the SKContext. The commit also updates the unit tests and the planner extensions to use the new function delegate parameter. The skprompt.txt file was updated to clarify some points about the function syntax and the available functions. The examples were also reorganized and simplified.
This commit changes the SequentialPlanParser to ignore text and comment nodes when parsing a plan from XML. These nodes are not valid steps and should not be added to the plan. This also updates the unit tests to reflect the expected behavior. Additionally, this commit adds some comments to the skprompt.txt file to clarify the expected format and validity of the plans. Squashed commit of the following: commit eeae9b0 Author: Lee Miller <lemiller@microsoft.com> Date: Thu Jun 22 15:26:51 2023 -0700 🐛 Fix sequential plan parser to ignore non-function tags The sequential plan parser was incorrectly adding non-function tags as steps in the plan, resulting in extra and invalid steps. This commit fixes the parser to skip any tags that are not <function> and only add the function steps to the plan. This also updates the unit tests to reflect the expected behavior and output of the parser. commit 0e2e9c1 Author: Lee Miller <lemiller@microsoft.com> Date: Thu Jun 22 15:23:14 2023 -0700 🐛 Fix plan parsing to ignore text and comments The plan parser was incorrectly adding text and comment nodes as steps in the plan, which could cause errors or unexpected behavior. This commit fixes the parser to skip over these nodes and only add function nodes as steps. It also adds some TODO comments for possible future enhancements to use text or comments as reasoning or desired functions for a plan. commit 5bb555c Author: Lee Miller <lemiller@microsoft.com> Date: Thu Jun 22 14:44:29 2023 -0700 🛠️ Fix stop sequence for sequential planner The stop sequence for the sequential planner was missing the "END" part, which could cause problems when parsing the XML plan. This commit fixes the stop sequence to include the "END" comment. It also simplifies the instructions for creating a plan, removing some redundant or outdated information. It adds an example of how to call a function with multiple inputs, and how to use XML comments in the input values.
The parameter order in the ToPlanFromXml method was inconsistent with the rest of the codebase, and could cause confusion for callers. This commit swaps the order of the allowMissingFunctions and GetFunction parameters, and updates the XML documentation accordingly. No changes were made to the method logic or functionality.
…r relevancy threshold This commit updates the SequentialPlanner to use the AzureChatCompletionService instead of the AzureTextCompletionService, which is more suitable for generating natural language responses. It also lowers the relevancy threshold for the planner to 0.5, which may improve the quality of the plans. Additionally, this commit updates the skprompt.txt file to include the available functions in the prompt, and to clarify the steps for creating a plan and calling a function. It also adds some examples of valid and invalid function calls. Finally, this commit removes a redundant EmailSkillFake import in the SequentialPlannerTests class, and adds a fake EmailSkill to simulate the use of a custom skill in the planning process. No functional changes were made to the tests or the planner.
- Use ternary operator to simplify GetAvailableFunctionsAsync logic - Rename GetFunction to GetSkillFunction for clarity and consistency - Update references to GetFunction in SequentialPlanParser and SequentialPlanner - Rename GetFunction property in SequentialPlannerConfig to GetSkillFunction
This commit renames the GetFunction method in the SequentialPlanParser class to GetSkillFunction, to better reflect its purpose and avoid confusion with other methods that also deal with functions. The method is used to create a delegate that takes a skill name and returns a corresponding function object. All the references and tests that use this method are also updated accordingly. This change improves the readability and clarity of the code.
lemillermicrosoft
force-pushed
the
626_sequential_config
branch
from
June 27, 2023 17:01
aa3f87b
to
7a847f6
Compare
teresaqhoang
requested changes
Jun 27, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, lgtm! Requesting some small changes for dev clarity
dotnet/src/Extensions/Planning.SequentialPlanner/SequentialPlanParser.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Extensions/Planning.SequentialPlanner/SequentialPlanParser.cs
Show resolved
Hide resolved
dotnet/src/Extensions/Planning.SequentialPlanner/SKContextSequentialPlannerExtensions.cs
Show resolved
Hide resolved
This commit adds the ability to specify custom function providers for the sequential planner, which can be used to filter or augment the available functions for planning. The default behavior is to use the SKContext function provider, which returns all the functions from the context skills. The custom function providers can be set in the SequentialPlannerConfig class, using the GetAvailableFunctionsAsync and GetSkillFunction properties. These are optional callbacks that take the planner config and the semantic query as parameters, and return the desired functions. The commit also fixes a bug in the SequentialPlanParser, where it would not handle the case where the skill name is empty. This could happen when using a custom function provider that returns functions without a skill name. The parser now tries to find the function by name only if the skill name is empty.
teresaqhoang
approved these changes
Jun 27, 2023
shawncal
pushed a commit
to shawncal/semantic-kernel
that referenced
this pull request
Jul 6, 2023
…soft#1727) ### Motivation and Context This change is part of a need to enable planners that can reference a large list of potential plugins that aren't loaded in a kernel instance. Additionally, consumers of planners need to be able to load plugins on demand when needed by a plan. The change includes some additional improvements to the sequential plan parser and prompt. Part of microsoft#1728 ### Description This pull request refactors and improves the sequential plan parser and config, simplifying the code, adding new features, and fixing some issues. The main changes are: - Use the Azure Chat Completion Service instead of the Azure Text Completion Service for the sequential planner, and lower the relevancy threshold for creating a plan. This allows for more flexibility and creativity in generating plans, and improves the performance and accuracy of the planner. - Refactor the plan parsing logic to simplify the code and improve readability. Remove unnecessary try-catch blocks, logging statements, and string splitting logic. Reorder the parameters of the ToPlanFromXml method to match the order of the XML attributes. Ignore text and comment nodes in the plan XML, as they are not valid steps and could cause confusion or errors. Remove the logic that adds empty nodes as steps, as they are not valid functions and could cause errors or confusion. - Add two optional functions to the SequentialPlannerConfig class that allow the planner to use custom logic for getting available functions and finding functions by name. These functions can be useful for scenarios where the planner needs to access functions from different sources or with different criteria, such as filtering by skill category or function type. The default behavior of the planner remains unchanged if these functions are not specified. - Simplify and standardize the syntax for calling functions in the plan XML, using fully qualified function names and consistent attribute names for inputs, outputs, and parameters. Update the skprompt.txt file to reflect the new syntax and add more instructions and examples for creating a plan. Update the sequential plan parser tests to use the new syntax and function names, and add a helper method to get the function from the kernel context. - Fix some minor typos and errors in the code and the skprompt.txt file. Details: - dotnet/src/IntegrationTests/Planning/SequentialPlanner/SequentialPlannerTests.cs - Changed the KernelSyntaxExamples and SequentialPlannerTests to use the Azure Chat Completion Service instead of the Azure Text Completion Service, by replacing the deployment name, endpoint, and key environment variables. - Removed the EmailSkillFake import from the InitializeKernel method, as it is not used in any test case and is already imported in the TestHelpers class. - Updated the tests to reflect the changes in the plan parser and remove the redundant or invalid steps. For example, remove the text and empty nodes from the plan XML, and update the expected number and description of the steps. - dotnet/src/Extensions/Planning.SequentialPlanner/SequentialPlanParser.cs - Simplified the ToPlanFromXml method by removing unnecessary try-catch blocks, logging statements, and string splitting logic. - Reordered the parameters of the ToPlanFromXml method to match the order of the XML attributes, as this makes the code more consistent and easier to read. - Ignored text and comment nodes in the plan XML, as they are not valid steps and could cause confusion or errors. Added TODO comments for possible future enhancements using text or comments as reasoning or desired functions. - Removed the logic that adds empty nodes as steps, as they are not valid functions and could cause errors or confusion. Added TODO comments for possible future enhancements using empty nodes as desired functions. - Added a GetFunction parameter to the ToPlanFromXml method that takes a delegate for finding a skill function by skill name and function name. This delegate can be specified in the SequentialPlannerConfig class or default to the existing logic of using the SKContext. - Added a new static method GetFunction to the SequentialPlanParser class that returns the default delegate for getting the ISKFunction instance, which calls context.Skills.TryGetFunction. - Fixed a minor typo in the GetFunctionName method, changing "functionName" to "functionNameAttribute". This makes the code more accurate and clear. - dotnet/src/Extensions/Planning.SequentialPlanner/SequentialPlannerConfig.cs - Added a new property GetAvailableFunctionsAsync to the SequentialPlannerConfig class that takes a delegate of type Func<SequentialPlannerConfig, string, Task<IOrderedEnumerable<FunctionView>>>. This delegate can be used to provide a custom logic for retrieving available functions from a semantic query, instead of using the default logic that calls context.GetAvailableFunctionsAsync. The default value of this property is null, which means the default logic will be used. - Modified the SKContextSequentialPlannerExtensions class to use the GetAvailableFunctionsAsync delegate if it is not null, otherwise fall back to the default logic. - dotnet/src/Extensions/Planning.SequentialPlanner/skprompt.txt - Simplified and standardized the syntax for calling functions from <function.{FunctionName} ... /> to <function.{FullyQualifiedFunctionName} ... />, where {FullyQualifiedFunctionName} is the name of the function with its namespace or skill prefix, such as _GLOBAL_FUNCTIONS_.NovelOutline or AuthorAbility.Summarize. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [x] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [x] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with `dotnet format` - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
This change is part of a need to enable planners that can reference a large list of potential plugins that aren't loaded in a kernel instance. Additionally, consumers of planners need to be able to load plugins on demand when needed by a plan. The change includes some additional improvements to the sequential plan parser and prompt.
Part of #1728
Description
This pull request refactors and improves the sequential plan parser and config, simplifying the code, adding new features, and fixing some issues. The main changes are:
Details:
Contribution Checklist
dotnet format