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.

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

Planner Constructor normalization #2847

Closed
Tracked by #2519
hario90 opened this issue Sep 15, 2023 · 0 comments · Fixed by #2849
Closed
Tracked by #2519

Planner Constructor normalization #2847

hario90 opened this issue Sep 15, 2023 · 0 comments · Fixed by #2849
Assignees
Labels
Milestone

Comments

@hario90
Copy link
Contributor

hario90 commented Sep 15, 2023

No description provided.

@hario90 hario90 self-assigned this Sep 15, 2023
@lemillermicrosoft lemillermicrosoft added this to the v1 milestone Sep 19, 2023
github-merge-queue bot pushed a commit that referenced this issue Sep 21, 2023
### Motivation and Context

Fixes #2847,  #2519

Contains breaking changes.

This addresses inconsistencies in planner construction. The planners
sometimes took a prompt override through a delegate in the config and
sometimes as a constructor argument. Additionally, the ActionPlanner
used to take a loggerFactory as a constructor argument while the other
planners did not.

The goal of these changes is to make working with planners more
predictable.

### Description

* Added `GetPromptTemplate` delegate function to the planner base
config. All planners will use this to get the prompt override.
* Removed logger factory argument from ActionPlanner constructor. Used
the Kernel's logger factory to create a new logger within each planner
constructor.
* Renamed private `Config` private instance field to `_config` 

### Contribution Checklist

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

- [x] The code builds clean without any errors or warnings
- [x] 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: Shawn Callegari <36091529+shawncal@users.noreply.github.com>
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this issue Nov 1, 2023
### Motivation and Context

Fixes microsoft#2847,  microsoft#2519

Contains breaking changes.

This addresses inconsistencies in planner construction. The planners
sometimes took a prompt override through a delegate in the config and
sometimes as a constructor argument. Additionally, the ActionPlanner
used to take a loggerFactory as a constructor argument while the other
planners did not.

The goal of these changes is to make working with planners more
predictable.

### Description

* Added `GetPromptTemplate` delegate function to the planner base
config. All planners will use this to get the prompt override.
* Removed logger factory argument from ActionPlanner constructor. Used
the Kernel's logger factory to create a new logger within each planner
constructor.
* Renamed private `Config` private instance field to `_config` 

### Contribution Checklist

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

- [x] The code builds clean without any errors or warnings
- [x] 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: 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
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants