Skip to content

Conversation

joowon-dm-snu
Copy link
Contributor

Motivation and Context

implement stepwise planner #2221

Description

Contribution Checklist

@shawncal shawncal added the python Pull requests for the Python Semantic Kernel label Jul 30, 2023
@joowon-dm-snu joowon-dm-snu marked this pull request as ready for review August 3, 2023 19:55
@joowon-dm-snu joowon-dm-snu requested a review from a team as a code owner August 3, 2023 19:55
@joowon-dm-snu
Copy link
Contributor Author

i think bc61d2e this commit should be handled more globally in other PR. #2301

Copy link
Contributor

@awharrison-28 awharrison-28 left a comment

Choose a reason for hiding this comment

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

Hi @joowon-dm-snu,

Thanks for porting stepwise planner to python SK. And thank for you identifying the bug in plan.py. I am going to cherry pick the plan.py change out to a separate PR and add a new test for it as it's needed downstream for something else.

github-merge-queue bot pushed a commit that referenced this pull request Aug 10, 2023
### Motivation and Context

Initially called out and fixed by @joowon-dm-snu in
#2223

This PR closes
[#2094](#2094)

There was a bug in the Plan Object Model that propagates the name of the
step parameter to the next step. It should be propagating the
parameter's value


### Description
In addition to making the code change to fix the bug, also added a unit
test that covers the changed code.

Below image shows the before and after of the test with the fix. 


![image](https://github.com/microsoft/semantic-kernel/assets/54643756/7217f42a-a2ac-4491-a709-bea9be7328e3)

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] 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
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

Co-authored-by: Abby Harrison <[email protected]>
Copy link
Member

@lemillermicrosoft lemillermicrosoft left a comment

Choose a reason for hiding this comment

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

It'd be worth doing the webskill improvement IMO but would understand if better for a different PR. And we can iterate on any changes we do in dotnet (after we create a good validation suite).

Rearranged the order of chat completion parameters in both
`ChatRequestSettings` and `OpenAIChatCompletion` classes for better
readability. Updated test cases to reflect these changes and added a
new test case for stop sequences.
@lemillermicrosoft
Copy link
Member

FYI @joowon-dm-snu I pushed a commit to fix the UTs.

@lemillermicrosoft lemillermicrosoft added this pull request to the merge queue Aug 23, 2023
Merged via the queue into microsoft:main with commit 05afd29 Aug 23, 2023
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
…#2388)

### Motivation and Context

Initially called out and fixed by @joowon-dm-snu in
microsoft#2223

This PR closes
[microsoft#2094](microsoft#2094)

There was a bug in the Plan Object Model that propagates the name of the
step parameter to the next step. It should be propagating the
parameter's value


### Description
In addition to making the code change to fix the bug, also added a unit
test that covers the changed code.

Below image shows the before and after of the test with the fix. 


![image](https://github.com/microsoft/semantic-kernel/assets/54643756/7217f42a-a2ac-4491-a709-bea9be7328e3)

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] 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
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

Co-authored-by: Abby Harrison <[email protected]>
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
### 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.
-->
implement stepwise planner microsoft#2221

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] 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
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Abby Harrison <[email protected]>
Co-authored-by: Lee Miller <[email protected]>
Co-authored-by: Abby Harrison <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Pull requests for the Python Semantic Kernel

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants