Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Python: Filter only json content from basic planner execution input #1496

Merged
merged 3 commits into from
Jun 15, 2023
Merged

Python: Filter only json content from basic planner execution input #1496

merged 3 commits into from
Jun 15, 2023

Conversation

awharrison-28
Copy link
Contributor

Motivation and Context

Closes #1469

There are scenarios where the basic_planner prompt results in a valid plan surrounded by additional text. This renders the plan unable to be json deserialized. This PR addresses this scenario. Note that it does NOT address malformed json.

Description

  • added a regex in basic_planner.execute_plan_async that grabs only json blocks. See screenshot below of it ignoring additional, non-json text.
  • Added dependency on regex pip package (re package does not support regex recursion)

image

Contribution Checklist

@awharrison-28 awharrison-28 added the bug Something isn't working label Jun 14, 2023
@github-actions github-actions bot added the python Pull requests for the Python Semantic Kernel label Jun 14, 2023
@shawncal shawncal merged commit 7b3dc2c into microsoft:main Jun 15, 2023
26 checks passed
dluc
dluc reviewed Jun 15, 2023
View reviewed changes
python/semantic_kernel/planning/basic_planner.py
generated_plan = json.loads(plan.generated_plan.result)

# Filter out good JSON from the result in case additional text is present
json_regex = r"\{(?:[^{}]|(?R))*\}"
Copy link
Collaborator

@dluc dluc Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of notes:

  • is the regex correct? I'm getting parse errors (using online tools), e.g. at ?R (the code works with python though)
  • could you add a comment explaining the regex?
  • maybe add unit tests?
  • is the regex safe (redos)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a breakdown of the regex pattern {(?:[^{}]|(?R))*}:

  • \{: This part of the pattern matches the opening curly brace {.
  • (?:: This part is the beginning of a non-capturing group. Non-capturing groups allow you to apply quantifiers to part of your regex, without capturing the matched text.
  • [^{}]: This part of the pattern is a character class that matches any character except the curly braces { and }.
  • |: This is the alternation (OR) operator, which means the pattern can match either the left side (any character except curly braces) or the right side (the next part of the pattern).
  • (?R): This part of the pattern is a regex recursion. It refers to the entire regular expression pattern, allowing the pattern to be applied recursively. This is useful for matching nested structures like JSON.
  • )*: This part of the pattern is a quantifier that means "match zero or more of the preceding pattern." In this case, the preceding pattern is the non-capturing group that contains the character class and the recursion.
  • \}: This part of the pattern matches the closing curly brace }.

Overall, the regex pattern can be described as follows:
Match an opening curly brace.
Match any number of characters that are not curly braces or recursively apply the entire regex pattern (to match nested JSON objects).
Match a closing curly brace.

@lemillermicrosoft lemillermicrosoft mentioned this pull request Jun 27, 2023
Closed
@evchaki evchaki added this to the Sprint 33 milestone Jun 30, 2023
shawncal added a commit to shawncal/semantic-kernel that referenced this pull request Jul 6, 2023
@awharrison-28 @shawncal
Python: Filter only json content from basic planner execution input (m…
e607929 
…icrosoft#1496)

### Motivation and Context
Closes microsoft#1469 

There are scenarios where the basic_planner prompt results in a valid
plan surrounded by additional text. This renders the plan unable to be
json deserialized. This PR addresses this scenario. Note that it does
NOT address malformed json.

### Description
- added a regex in basic_planner.execute_plan_async that grabs only json
blocks. See screenshot below of it ignoring additional, non-json text.
- Added dependency on `regex` pip package (`re` package does not support
regex recursion)

Co-authored-by: Shawn Callegari <36091529+shawncal@users.noreply.github.com>
@Sailesh-2209 Sailesh-2209 mentioned this pull request Jul 11, 2023
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jul 31, 2023
@Sailesh-2209 @awharrison-28 @shawncal
Python: Action Planner implementation (#1948)
0d37271 
### 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 PR is a first draft for implementation of ActionPlanner in Python.
I try to follow the C# implementation as much as possible.
Fixes: #1917

### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
- I have implemented ActionPlanner in the
`python/semantic_kernel/planning/action_planner` directory. The
implementation is in a file called `action_planner.py` and the prompt
used by the planner (a replica of the prompt used in C#) is found in
`skprompt.txt` in the same directory.
- The functions `good_examples`, `edge_case_examples` and
`list_of_functions` are used by the prompt to populate examples and list
of available functions in the kernel.
- After creating the plan, I parse the JSON generated by the planner by
using regex matching to extract out pure JSON as implemented in #1496
- Based on whether or not the planner was able to identify a function, I
create a Plan instance and return it.

### Here is some example code for using the planner
```python
import logging
import semantic_kernel as sk
from semantic_kernel.planning import ActionPlanner
from semantic_kernel.connectors.ai.open_ai import (
	OpenAIChatCompletion,
	OpenAITextCompletion
)
from semantic_kernel.core_skills import (
	MathSkill,
	FileIOSkill,
	TimeSkill,
	TextSkill
)

async def main():
	kernel = sk.Kernel()
	api_key, org_id = sk.openai_settings_from_dot_env()

	kernel.add_text_completion_service("dv", OpenAITextCompletion("text-davinci-003", api_key, org_id))
	kernel.import_skill(MathSkill(), "math")
	kernel.import_skill(FileIOSkill(), "fileIO")
	kernel.import_skill(TimeSkill(), "time")
	kernel.import_skill(TextSkill(), "text")

	planner = ActionPlanner(kernel)
	ask = "What is the sum of 110 and 990?"
	plan = await planner.create_plan_async(goal=ask)
	result = await plan.invoke_async()
	print(result.result)

if __name__ == "__main__":
	import asyncio
	asyncio.run(main())
```

### Queries
- In the C# implementation, when populating the function parameters in
the plan object, the `plan.parameters` variable is updated with the
function parameters. When I did the same in python, these parameters
weren't being passed onto the function in `invoke_async` method of the
plan instance. The parameters are only being passed when I update them
in the `plan.state` variable. This code is in `line 121` of
`action_planner.py`. What is the right way to do this?
- Some skills / plugins require one of the inputs to be passed as the
`input` variable and not in the context. How should this be handled in
the planner?

![image](https://github.com/microsoft/semantic-kernel/assets/69807105/967ea4b7-914e-46ac-89fb-1eb81c4ea9b0)
**OR**

![image](https://github.com/microsoft/semantic-kernel/assets/69807105/13a21556-8028-4f54-b40a-e4ebd1c5e654)

### Issues that I faced
- The `text-davinci-002` text-completion model regularly output
incorrectly formatted JSON with commas missing. For example
  ```json
  {""plan"":{
""rationale"": ""The list contains a function that allows for writing to
files, so the plan is to use that function to write the desired content
to the file 'output.txt'."",
  ""function"": ""fileIO.writeAsync"",
  ""parameters"": {
  ""input"": null,
  ""content"": ""A brief history of computer science""
  ""path"": ""output.txt""
  }}}
  ```


Pending Items
- [x] Create unit tests
- [x] Send the function parameters to the planner in list of available
functions with/without input parameter.

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

---------

Co-authored-by: Abby Harrison <abby.harrison@microsoft.com>
Co-authored-by: Abby Harrison <54643756+awharrison-28@users.noreply.github.com>
Co-authored-by: Shawn Callegari <36091529+shawncal@users.noreply.github.com>
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
@Sailesh-2209 @awharrison-28
@shawncal @SOE-YoungS
Python: Action Planner implementation (microsoft#1948)
450b253 
### 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 PR is a first draft for implementation of ActionPlanner in Python.
I try to follow the C# implementation as much as possible.
Fixes: microsoft#1917

### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
- I have implemented ActionPlanner in the
`python/semantic_kernel/planning/action_planner` directory. The
implementation is in a file called `action_planner.py` and the prompt
used by the planner (a replica of the prompt used in C#) is found in
`skprompt.txt` in the same directory.
- The functions `good_examples`, `edge_case_examples` and
`list_of_functions` are used by the prompt to populate examples and list
of available functions in the kernel.
- After creating the plan, I parse the JSON generated by the planner by
using regex matching to extract out pure JSON as implemented in microsoft#1496
- Based on whether or not the planner was able to identify a function, I
create a Plan instance and return it.

### Here is some example code for using the planner
```python
import logging
import semantic_kernel as sk
from semantic_kernel.planning import ActionPlanner
from semantic_kernel.connectors.ai.open_ai import (
	OpenAIChatCompletion,
	OpenAITextCompletion
)
from semantic_kernel.core_skills import (
	MathSkill,
	FileIOSkill,
	TimeSkill,
	TextSkill
)

async def main():
	kernel = sk.Kernel()
	api_key, org_id = sk.openai_settings_from_dot_env()

	kernel.add_text_completion_service("dv", OpenAITextCompletion("text-davinci-003", api_key, org_id))
	kernel.import_skill(MathSkill(), "math")
	kernel.import_skill(FileIOSkill(), "fileIO")
	kernel.import_skill(TimeSkill(), "time")
	kernel.import_skill(TextSkill(), "text")

	planner = ActionPlanner(kernel)
	ask = "What is the sum of 110 and 990?"
	plan = await planner.create_plan_async(goal=ask)
	result = await plan.invoke_async()
	print(result.result)

if __name__ == "__main__":
	import asyncio
	asyncio.run(main())
```

### Queries
- In the C# implementation, when populating the function parameters in
the plan object, the `plan.parameters` variable is updated with the
function parameters. When I did the same in python, these parameters
weren't being passed onto the function in `invoke_async` method of the
plan instance. The parameters are only being passed when I update them
in the `plan.state` variable. This code is in `line 121` of
`action_planner.py`. What is the right way to do this?
- Some skills / plugins require one of the inputs to be passed as the
`input` variable and not in the context. How should this be handled in
the planner?

![image](https://github.com/microsoft/semantic-kernel/assets/69807105/967ea4b7-914e-46ac-89fb-1eb81c4ea9b0)
**OR**

![image](https://github.com/microsoft/semantic-kernel/assets/69807105/13a21556-8028-4f54-b40a-e4ebd1c5e654)

### Issues that I faced
- The `text-davinci-002` text-completion model regularly output
incorrectly formatted JSON with commas missing. For example
  ```json
  {""plan"":{
""rationale"": ""The list contains a function that allows for writing to
files, so the plan is to use that function to write the desired content
to the file 'output.txt'."",
  ""function"": ""fileIO.writeAsync"",
  ""parameters"": {
  ""input"": null,
  ""content"": ""A brief history of computer science""
  ""path"": ""output.txt""
  }}}
  ```


Pending Items
- [x] Create unit tests
- [x] Send the function parameters to the planner in list of available
functions with/without input parameter.

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

---------

Co-authored-by: Abby Harrison <abby.harrison@microsoft.com>
Co-authored-by: Abby Harrison <54643756+awharrison-28@users.noreply.github.com>
Co-authored-by: Shawn Callegari <36091529+shawncal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dluc dluc dluc left review comments

@shawncal shawncal shawncal approved these changes

@mkarle mkarle Awaiting requested review from mkarle

@alexchaomander alexchaomander Awaiting requested review from alexchaomander

Assignees
No one assigned
Labels
bug Something isn't working python Pull requests for the Python Semantic Kernel
Projects
None yet
Milestone
Sprint 33
Development

Successfully merging this pull request may close these issues.

Python: basic planner generated plan sometimes contains generated text that is not json de-serializable.
4 participants
@awharrison-28 @dluc @shawncal @evchaki