Skip to content

[Python] Add conversation summary skill - #278

Merged
Devis Lucato (dluc) merged 7 commits into
microsoft:python-previewfrom
kitnhl:conversation-summary-skil-py
Apr 10, 2023
Merged

[Python] Add conversation summary skill#278
Devis Lucato (dluc) merged 7 commits into
microsoft:python-previewfrom
kitnhl:conversation-summary-skil-py

Conversation

@kitnhl

@kitnhl Kit (kitnhl) commented Apr 3, 2023 •
edited
Loading

Copy link
Copy Markdown

Motivation and Context

Porting from C#

Description

The semantic_text_partitioner class and SKFunctionBase.aggregate_partitioned_results_async(...) method still need to be implemented for the skill to be operational, but for the sake of modularity and PR granularity, I will leave these implementations outside the scope of this particular PR.

Contribution Checklist

@kitnhl Kit (kitnhl) changed the title [DRAFT] [Python] Add conversation summary skill [Python] Add conversation summary skill Apr 3, 2023
@adrianwyatt Adrian Bonar (adrianwyatt) added the python Pull requests for the Python Semantic Kernel label Apr 3, 2023
@jjhenkel

Jordan Henkel (jjhenkel) commented Apr 4, 2023

Copy link
Copy Markdown

Hey thanks for working on this! I know it's in a draft state, but one bit of early feedback: maybe in the Python version we can put those constants w/ the skill? (This will also help reduce conflicts as we have others working on similar skills that used to, in the C# version, store their prompts in a "constants" file.)

Open to thoughts/feedback!

@jjhenkel

Jordan Henkel (jjhenkel) commented Apr 4, 2023

Copy link
Copy Markdown

Oh and as we're evolving things, soon the sk.extensions.create_*(kernel, ...) is getting cleaned up to be just kernel.create_(...) (so long as you use Kernel as the type and not KernelBase)

@kitnhl
Kit (kitnhl) marked this pull request as ready for review April 5, 2023 03:23
@kitnhl

Kit (kitnhl) commented Apr 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

Hey thanks for working on this! I know it's in a draft state, but one bit of early feedback: maybe in the Python version we can put those constants w/ the skill? (This will also help reduce conflicts as we have others working on similar skills that used to, in the C# version, store their prompts in a "constants" file.)

Open to thoughts/feedback!

Hi Jordan! Happy to help! 😀 Some of the prompts are really long and I feel like it's less readable if it's just part of the code file, so I still have them in a constants file for now. Please let me know if you feel strongly the other way and I'll change it no problem.

@jjhenkel

Jordan Henkel (jjhenkel) commented Apr 5, 2023

Copy link
Copy Markdown

That sounds good for now Kit (@kitnhl)! I think we may eventually try and load them from file or something to clean it up, but that can come later. I'll take a look at this PR soon, looks like we may be blocked by some of the partitioning features being missing (I know someone is actively working on that though).

mkarle
Mark Karle (mkarle) suggested changes Apr 6, 2023
View reviewed changes

@mkarle Mark Karle (mkarle) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some typos. The rest can be done when unblocked by the partitioner.

Comment thread python/semantic_kernel/core_skills/conversation_summary_skill.py Resolved
Comment thread python/semantic_kernel/core_skills/conversation_summary_skill.py Resolved
Comment thread python/semantic_kernel/core_skills/conversation_summary_skill.py Resolved
@mkarle Mark Karle (mkarle) added the PR: ready to merge PR has been approved by all reviewers, and is ready to merge. label Apr 10, 2023
@dluc
Devis Lucato (dluc) merged commit 0585476 into microsoft:python-preview Apr 10, 2023
golden-aries pushed a commit to golden-aries/semantic-kernel that referenced this pull request Oct 10, 2023
@alliscode Ben Thomas
Updating broken links and links to wrong repository. (microsoft#278)
d0d92bb 
### Description

Fixing broken links and links that point to the wrong repository.

### Contribution Checklist

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

- [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
- [x] I didn't break anyone 😄

---------

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

Reviewers

1 more reviewer
@mkarle Mark Karle (mkarle) mkarle approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mkarle Mark Karle (mkarle)

Labels

PR: ready to merge PR has been approved by all reviewers, and is ready to merge. python Pull requests for the Python Semantic Kernel

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kitnhl @jjhenkel @mkarle @dluc @adrianwyatt