Skip to content

Python: sync C# for Pre/Post RunAsync Handlers - #3063

Merged
Roger Barreto (rogerbarreto) merged 24 commits into
microsoft:mainfrom
joowon-dm-snu:python-add-handlers-part1
Nov 13, 2023
Merged

Python: sync C# for Pre/Post RunAsync Handlers#3063
Roger Barreto (rogerbarreto) merged 24 commits into
microsoft:mainfrom
joowon-dm-snu:python-add-handlers-part1

Conversation

@joowon-dm-snu

@joowon-dm-snu Joowon (joowon-dm-snu) commented Oct 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation and Context

#3038

Description

Contribution Checklist

@shawncal Shawn Callegari (shawncal) added the python Pull requests for the Python Semantic Kernel label Oct 3, 2023
@joowon-dm-snu
Joowon (joowon-dm-snu) marked this pull request as ready for review October 4, 2023 17:26
@joowon-dm-snu
Joowon (joowon-dm-snu) requested a review from a team as a code owner October 4, 2023 17:26
@alexchaomander

Alex Chao (alexchaomander) commented Oct 15, 2023

Copy link
Copy Markdown
Contributor

Great to see this Joowon (@joowon-dm-snu)! Thank you for helping to sync this important feature to Python!

@rogerbarreto

Roger Barreto (rogerbarreto) commented Oct 19, 2023

Copy link
Copy Markdown
Member

Joowon (@joowon-dm-snu) Thanks for the contribution, all of the implementation seems very close to C# codebase, I missed the cancelation capability, are you planning in adding that?

image

@joowon-dm-snu

Joowon (joowon-dm-snu) commented Oct 19, 2023

Copy link
Copy Markdown
Contributor Author

Roger Barreto (@rogerbarreto) No plan for cancellation token for now as semantic-kernel python doesn't have that one :)

image

@rogerbarreto

Roger Barreto (rogerbarreto) commented Oct 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Joowon (@joowon-dm-snu) I see, is it possible to instead of having a cancellation token use a flag where the handler could cancel/abort the pipeline?

@joowon-dm-snu

Joowon (joowon-dm-snu) commented Oct 24, 2023

Copy link
Copy Markdown
Contributor Author

Roger Barreto (@rogerbarreto) I hope this commit is your intended implementation :)

@joowon-dm-snu Joowon (joowon-dm-snu) mentioned this pull request Oct 26, 2023
Closed
@rogerbarreto Roger Barreto (rogerbarreto) added the PR: ready to merge PR has been approved by all reviewers, and is ready to merge. label Nov 7, 2023
dmytrostruk
Dmytro Struk (dmytrostruk) approved these changes Nov 11, 2023
View reviewed changes

@dmytrostruk Dmytro Struk (dmytrostruk) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Joowon (@joowon-dm-snu) Thank you for contribution! I added a couple of small comments, but in general looks good to me!

Comment thread python/semantic_kernel/events/__init__.py Resolved
Comment thread python/semantic_kernel/kernel.py Resolved
@rogerbarreto
Roger Barreto (rogerbarreto) added this pull request to the merge queue Nov 13, 2023
Merged via the queue into microsoft:main with commit 194f240 Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmytrostruk Dmytro Struk (dmytrostruk) dmytrostruk approved these changes
@rogerbarreto Roger Barreto (rogerbarreto) rogerbarreto approved these changes

Assignees

@rogerbarreto Roger Barreto (rogerbarreto)

@joowon-dm-snu Joowon (joowon-dm-snu)

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

@joowon-dm-snu @alexchaomander @rogerbarreto @dmytrostruk @shawncal