Java: Enable plan validation in the SequentialPlanner #2855
Comments
github-actions
bot
changed the title
|
@johnoliver - we have this feature in .net, can you take a look and see if we have this in Java to stop making up plugins that are not there. - @alliscode can provide more details here also. |
|
We will take a look at implementing this our side, @alliscode is this the correct code to be looking at: |
|
Is this something you'd be open to contributions on if it becomes a high priority for us before it becomes a high priority for you? |
|
@lnavarette Yes we are absolutely open to contributions, feel free to message in the discord if you want to discuss it, I believe the validation model c# side is at: https://github.com/microsoft/semantic-kernel/blob/main/dotnet/src/Planners/Planners.Core/Sequential/SequentialPlanParser.cs#L171 so can replicate that, and if there is any other validation you think we can do let us know. |
4 tasks
github-merge-queue bot
pushed a commit
that referenced
this issue
Nov 8, 2023
…er (#3384) ### Motivation and Context <!-- Thank you for your contribution to the semantic-kernel repo! Please help reviewers and future users, providing the following information: 1. Why is this change required? 2. What problem does it solve? 3. What scenario does it contribute to? 4. If it fixes an open issue, please link to the issue here. --> This is to resolve #2855 -- when using the java `SequentialPlanner`, the planner will return a plan with missing skills. This enables a configuration setting to allow/disallow the missing functions. ### Description <!-- Describe your changes, the overall approach, the underlying design. These notes will help understanding how your code works. Thanks! --> This PR adds * The configuration setting to allow/disallow missing functions. I set the default to false (throw on a missing function), technically a breaking change from what was previously implemented. Given this branch is still prerelease, I decided that's the better default configuration, and the breaking change should be okay. * The logic in the plan parser; to throw when `allowMissingFunctions=true` and the plan has a missing function. * Unit tests for the SequentialPlanner that bring its tests to parity with the other Planner unit tests and test the desired behavior for this setting when it is on or off. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [ ] 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: Linda Navarette <[email protected]>
|
This issue is stale because it has been open for 90 days with no activity. |
github-actions
bot
added
the
stale
|
Closing as we are not planning on implementing planners right now. Instead we will be implementing tool calling for the V1 release line. |
johnoliver
pushed a commit
to johnoliver/semantic-kernel
that referenced
this issue
Jun 5, 2024
…er (microsoft#3384) ### Motivation and Context <!-- Thank you for your contribution to the semantic-kernel repo! Please help reviewers and future users, providing the following information: 1. Why is this change required? 2. What problem does it solve? 3. What scenario does it contribute to? 4. If it fixes an open issue, please link to the issue here. --> This is to resolve microsoft#2855 -- when using the java `SequentialPlanner`, the planner will return a plan with missing skills. This enables a configuration setting to allow/disallow the missing functions. ### Description <!-- Describe your changes, the overall approach, the underlying design. These notes will help understanding how your code works. Thanks! --> This PR adds * The configuration setting to allow/disallow missing functions. I set the default to false (throw on a missing function), technically a breaking change from what was previously implemented. Given this branch is still prerelease, I decided that's the better default configuration, and the breaking change should be okay. * The logic in the plan parser; to throw when `allowMissingFunctions=true` and the plan has a missing function. * Unit tests for the SequentialPlanner that bring its tests to parity with the other Planner unit tests and test the desired behavior for this setting when it is on or off. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [ ] 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: Linda Navarette <[email protected]>
johnoliver
pushed a commit
to johnoliver/semantic-kernel
that referenced
this issue
Jun 5, 2024
…er (microsoft#3384) ### Motivation and Context <!-- Thank you for your contribution to the semantic-kernel repo! Please help reviewers and future users, providing the following information: 1. Why is this change required? 2. What problem does it solve? 3. What scenario does it contribute to? 4. If it fixes an open issue, please link to the issue here. --> This is to resolve microsoft#2855 -- when using the java `SequentialPlanner`, the planner will return a plan with missing skills. This enables a configuration setting to allow/disallow the missing functions. ### Description <!-- Describe your changes, the overall approach, the underlying design. These notes will help understanding how your code works. Thanks! --> This PR adds * The configuration setting to allow/disallow missing functions. I set the default to false (throw on a missing function), technically a breaking change from what was previously implemented. Given this branch is still prerelease, I decided that's the better default configuration, and the breaking change should be okay. * The logic in the plan parser; to throw when `allowMissingFunctions=true` and the plan has a missing function. * Unit tests for the SequentialPlanner that bring its tests to parity with the other Planner unit tests and test the desired behavior for this setting when it is on or off. ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [ ] 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: Linda Navarette <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We've been using the experimental java branch with the SequentialPlanner. Sometimes, the LLM will return a plan that uses some example skills we don't implement in our project.
For example, the plan might look like
where we have implemented
CustomSkill.MyCustomSemanticFunctionbut notEmailer.EmailToIt happens infrequently (we have the temperature set to 0 and see it <5% of the time in our tests), so I tried to implement logic in our project to inspect the resulting
Planto check if any of the steps weren't available to the kernel, but the steps on the plan are not accessible.I assume this will be a common problem; I could see a few different ways to approach it:
Planitself for a user to check if it's executable and retry creation if it is notThe text was updated successfully, but these errors were encountered: