Conditional skills (If / IfElse) feature by rogerbarreto · Pull Request #175 · microsoft/semantic-kernel

Roger Barreto (rogerbarreto) · 2023-03-27T20:54:12Z

Motivation and Context

Currently, the core planner doesn’t support conditional behavior.
This design offers an initial “If” statement capability skill that allows planner control flow execution conditioned to check for equals/contains/greaterthan against context variables.

This change will allow Plan to run correctly for prompts like: Summarize an outline about AI, if the summarization contains any reference to "AGI" then Generate a summary outline for AGI. After that generate a list of synopsis for a novel using the resulting summarization.

Description

This change adds a conditional structure to the plan as a way to be able to use a standard conditional structure for branching that can be recursive as follows.

<plan>
  ... plan steps
  <if>
     <conditiongroup>
         <condition variable="variable1" equals|contains|greaterthan="value" />
     </conditiongroup>
     <then>
         ... plan steps
     </then>
     <else> (Optional)
        ... plan steps
        <if /> (nested ifs)
     </else>
  </if>
  ... plan steps
</plan>

When resolving the conditional group this skill returns the content in Then or Else branches including potential nested ifs.

This structure was build in a way that it can allow future nested complex conditions and checks like:

<conditiongroup>
  <condition variable="…" pattern|email|numeric|custom="…" />
  <and|or|xor|not …/>
  <conditiongroup> 
      (recursive and nested conditions...)
  </conditiongroup>
</conditiongroup>

The nested scenarios above were not yet fully tested and are not officially supported by this PR.

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

…in place

…al variables

…into features/planner-conditionalskill-ifelse

… Conditional Planner example

Lee Miller (lemillermicrosoft) · 2023-03-27T21:14:34Z

 </plan><!-- END -->

-[AVAILABLE FUNCTIONS]
+[EXAMPLE FUNCTIONS]


Do you find this improves things to help differentiate the pattern?

Yes, I was getting very frequently WriterSkill.Summarization instead of the Everything, this was happening with Skills that bumped in the examples given and for some reason the LLM was understanding AVAILABLE examples as indeed available...

Something for us to consider when we start allowing users to supply their own examples.

Even changing that to this pattern I see less frequently it using the wrong function, potentially we should use abstract examples that would not conflict with actual skills.

Lee Miller (lemillermicrosoft) · 2023-03-27T21:15:14Z

 7. To save an 'output' from a <function>, to pass into a future <function>, use <function.{FunctionName} ... setContextVariable: ""$<UNIQUE_VARIABLE_KEY>""/>
 8. To save an 'output' from a <function>, to return as part of a plan result, use <function.{FunctionName} ... appendToResult: ""RESULT__$<UNIQUE_RESULT_KEY>""/>
-9. Append an ""END"" XML comment at the end of the plan.
+9. Available conditions: greaterthan, equals, contains, startswith


I'd expect more instructions about these reserved words and others.

This can potentially increase but since we don't have any specific implementation the LLM is currently guessing what to do in the conditions. I'm adding this for the scope of this change but we can limit those later on

How does it know about 'if' or 'conditiongroup'? Only from examples? That will not work when we allow users to supply their own examples.

Lee Miller (lemillermicrosoft) · 2023-03-27T21:52:39Z

+        Console.WriteLine("Original plan:");
+        Console.WriteLine(originalPlan.Variables.ToPlan().PlanString);
+
+        await ExecutePlanAsync(kernel, planner, originalPlan, 5);


Is 5 the required steps here for completion of the plan? What is the input to the first Summarize? Can you show it with two different inputs to show it goes down each branch?

The number of steps can be greater, currently the plan will resolve the if (then/else) inner steps and add them as part of the list, so if in the overall branching we end up with more than 5 steps this will abort at the given step. For branching examples I can move this number up a bit.

Lee Miller (lemillermicrosoft) · 2023-03-27T21:53:44Z


+                if (processFunctions && o2.Name.StartsWith(ConditionIfTag, StringComparison.InvariantCultureIgnoreCase))
+                {
+                    if (!context.IsFunctionRegistered("Conditional", "If", out var ifCheckFunction))


What if they register with a different name?

Lee Miller (lemillermicrosoft) · 2023-03-27T21:54:11Z

        // Load native skill into the kernel skill collection, sharing its functions with prompt templates
        planner = kernel.ImportSkill(new PlannerSkill(kernel), "planning");
-
+        kernel.ImportSkill(new ConditionalSkill(kernel), "conditional");


I'm wondering if this is how we'll want to do things going forward.

Additionally, I'm still torn on whether we need a special syntax in the prompt for these skills or not -- maybe it could just be like any other. Did you try that?

PlannerSkill has a dependency on ConditionalSkill but this is not the ideal way moving forward, the PR proved the concept that the IF behavior worked.

Will think more on changes that don't create this problem of skillname referencing/dependency potentially adding ConditionalSkill as a Planner adittional capability instead of a standalone Skill.

Lee Miller (lemillermicrosoft)

Leaving more comments.

Dmytro Struk (dmytrostruk) · 2023-03-28T11:27:23Z

+public class ConditionalSkill
+{
+    #region Prompts
+    internal const string IfStructureCheckPrompt =


Maybe out of scope of this PR, but I'm wondering, can we store core skill prompts in separate txt files and read from them? The same applies for Planner and other skills. In this way we can keep our C# classes nice and clean.

Sure, I think this could be a improvement to tackle

Right now we are using SemanticFunctionConstants for these definitions.

Dmytro Struk (dmytrostruk) · 2023-03-28T11:30:04Z

+            maxTokens: 100,
+            temperature: 0,
+            topP: 0.5
+            );


nit: unnecessary new line here

Dmytro Struk (dmytrostruk) · 2023-03-28T11:30:54Z

+using Microsoft.SemanticKernel.SkillDefinition;
+
+namespace Microsoft.SemanticKernel.CoreSkills;
+public class ConditionalSkill


Need an empty line between namespace and class declaration. Also would be nice to add XML comments. Same applies for ConditionException.cs

Dmytro Struk (dmytrostruk) · 2023-03-28T11:35:47Z

+        foreach (var v in existingVariables)
+        {
+            // Numeric don't add quotes
+            if (Regex.Match(v.Value, "^[0-9.,]+$").Success)


nit: a little simplification, feel free to skip

var value = Regex.Match(v.Value, "^[0-9.,]+$").Success ? v.Value : JsonSerializer.Serialize(v.Value); conditionalVariables.AppendLine($"{v.Key} = {value}");

Shawn Callegari (shawncal) · 2023-03-29T05:21:41Z

+[END EXAMPLE FUNCTIONS]
+
+<goal>Summarize the input, if the input length is greater than 10 then Create an outline for a children's book with 3 chapters about a group of kids in a club and then summarize it otherwise translate to japanese and email it to Martin.</goal>
+<plan>


Yowza, this is going to be a token eating machine! We need to drive for simplicity in this format, or it'll burn too many tokens on prompt and response, and the AI is far more likely to get things wrong.

Asks:
at minimum, let's remove . Compound conditions can be constructed by nesting one inside another, or with specialized native skills. This is more complex than required right now.

<if> <conditiongroup> <condition variable=""$SUMMARIZED_INPUT"" greaterthan=""10"" /> </conditiongroup>

...becomes...
<if variable=""$SUMMARIZED_INPUT"" greaterthan=""10"">

I also think we can get rid of the <then> tags, as these are token eaters and kind of redundant (the content in the <if...> tags is implicitly then).

As for else, I think this could be another conditional stacked below with the inverse condition. I wonder if the AI would do that on its own:

<if variable=""$SUMMARIZED_INPUT"" greaterthan=""10""> <do stuff> </if> <if variable=""$SUMMARIZED_INPUT"" lessthanequals=""10""> <do other stuff> </if>

Shawn Callegari (shawncal) · 2023-03-29T05:27:57Z

 7. To save an 'output' from a <function>, to pass into a future <function>, use <function.{FunctionName} ... setContextVariable: ""$<UNIQUE_VARIABLE_KEY>""/>
 8. To save an 'output' from a <function>, to return as part of a plan result, use <function.{FunctionName} ... appendToResult: ""RESULT__$<UNIQUE_RESULT_KEY>""/>
-9. Append an ""END"" XML comment at the end of the plan.
+9. Available conditions: greaterthan, equals, contains, startswith


I think the most important conditions will be "isSet" (i.e. not null or empty), "isUnset".

Shawn Callegari (shawncal) · 2023-03-29T05:33:09Z

+    <conditiongroup>
+        <condition variable=""z"" greaterthan=""10"" />
+    </conditiongroup>
+    </not>


Yeah, way too complex. These compound conditions would be difficult to maintain. Please cut the conditiongroup for this first version -- keep it simple.

### Motivation and Context  I would like to provide a production-grade PaaS solution to store the vector database. It provides PostgreSQL database support for memory. We can now use Azure Database for PostgreSQL or CosmosDB for PostgreSQL. Fixes microsoft#166 ### Description  I added configuration support for PostgreSQL. in the kernel initialization. Furthermore I updated the deployment script to support this configuration. > Note: In the deployment scripts, I've added the SqlAdminPassword parameter, which must be supplied during provisioning. I intentionally named it without the PostgreSQL name, as I also worked on implementing SQL Server Memory store in the SK repository. This parameter could be used by both databases during deployment. ### Contribution Checklist  - [x] The code builds clean without any errors or warnings - [x] The PR follows the [Contribution Guidelines](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/copilot-chat/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible > Not found test project on the API part ;( - [x] I didn't break anyone 😄 --------- Co-authored-by: Gil LaHaye <[email protected]>

Roger Barreto (rogerbarreto) added 11 commits March 24, 2023 11:38

Added component classes, parsing logic

e28c8c2

Added conditional skill and components, getting the first unit tests …

8385f01

…in place

Adding unit tests changing functions accordingly

b3c1007

Improved unit tests, Changed logic to only sent to LLM used condition…

2143cb1

…al variables

Merge branch 'main' of https://github.com/RogerBarreto/semantic-kernel …

3487cfc

…into features/planner-conditionalskill-ifelse

Changes to the Planner to Use the conditional skill impl. Added a new…

657cd91

… Conditional Planner example

Changes to the Planner to Use the conditional skill impl. Added a new…

c10527f

… Conditional Planner example

Fix merge conflict

f5d967c

Updated UnitTest regarding the latest changes

d57bad7

Removing unneeded POM conditional classes

99e83e0

Removing unused code

3b71a44

Roger Barreto (rogerbarreto) requested a review from Lee Miller (lemillermicrosoft) March 27, 2023 20:58

Lee Miller (lemillermicrosoft) reviewed Mar 27, 2023

View reviewed changes

Comment thread dotnet/src/SemanticKernel/Planning/FunctionFlowRunner.cs Resolved

Lee Miller (lemillermicrosoft) reviewed Mar 27, 2023

View reviewed changes

Comment thread dotnet/src/SemanticKernel/CoreSkills/SemanticFunctionConstants.cs Resolved

Lee Miller (lemillermicrosoft) reviewed Mar 27, 2023

View reviewed changes

Roger Barreto (rogerbarreto) added 4 commits March 27, 2023 22:17

Small changes

d9d6a3a

Adding back launchSettings

1d7d354

Ignoring back launchsettings

741d384

Applying suggestions

9f2f964

Roger Barreto (rogerbarreto) requested review from Devis Lucato (dluc) and Shawn Callegari (shawncal) March 27, 2023 21:44

Lee Miller (lemillermicrosoft) reviewed Mar 27, 2023

View reviewed changes

Adrian Bonar (adrianwyatt) assigned Lee Miller (lemillermicrosoft) Mar 27, 2023

Dmytro Struk (dmytrostruk) reviewed Mar 28, 2023

View reviewed changes

Fix bugs in conditional skill

863fa84

Roger Barreto (rogerbarreto) added the enhancement label Mar 28, 2023

Roger Barreto (rogerbarreto) added PR: in progress Under development and/or addressing feedback .NET Issue or Pull requests regarding .NET code labels Mar 28, 2023

Added comments to the ConditionalSkill.cs

7ef1ad3

Shawn Callegari (shawncal) suggested changes Mar 29, 2023

View reviewed changes

Roger Barreto (rogerbarreto) closed this by deleting the head repository Mar 29, 2023

Conversation

Roger Barreto (rogerbarreto) commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Description

Contribution Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lee Miller (lemillermicrosoft) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Roger Barreto (rogerbarreto) commented Mar 27, 2023 •

edited

Loading