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

.Net: Removing internal HttpRetry Abstraction and Implementation #2281

Conversation

RogerBarreto
Copy link
Member

@RogerBarreto RogerBarreto commented Aug 2, 2023

Motivation and Context

⚠️ This is a breaking change.

Reduce the responsibility surface of Semantic Kernel regarding Resiliency implementations, keeping only the abstractions that can be delegated and handling by well know resiliency libraries and custom implementations.

Resolves #2271
Closes #2271

Description

This change removes and deprecates the usage of Retry implementations in our Resiliency namespace with NullHttpHandlerFactory implementations.

Changed our examples using Polly Handlers to handle most common retrial scenarios.

Contribution Checklist

@RogerBarreto RogerBarreto added PR: ready for review All feedback addressed, ready for reviews PR: breaking change Pull requests that introduce breaking changes labels Aug 2, 2023
@RogerBarreto RogerBarreto self-assigned this Aug 2, 2023
@RogerBarreto RogerBarreto requested a review from a team as a code owner August 2, 2023 13:52
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Aug 2, 2023
@shawncal shawncal changed the title Removing internal HttpRetry Abstraction and Implementation .Net: Removing internal HttpRetry Abstraction and Implementation Aug 2, 2023
@shawncal shawncal added the PR: paused PR temporarily parked label Aug 3, 2023
@shawncal
Copy link
Member

shawncal commented Aug 3, 2023

Pausing this PR due to its impact, until after next nuget release. Please continue to review and apply changes -- just don't merge!

@shawncal
Copy link
Member

shawncal commented Aug 4, 2023

@SergeyMenshykh any thoughts or concerns here? Possible conflicts with error handling plans?

@SergeyMenshykh
Copy link
Member

@SergeyMenshykh any thoughts or concerns here? Possible conflicts with error handling plans?

I welcome the idea of minimizing SK SDK API surface and removing all infra code that is not related to SK SDK domain. I don't see and conflicts with error handling.

@lemillermicrosoft
Copy link
Member

lemillermicrosoft commented Aug 4, 2023

So now there will be no retry handling by default? That seems like a big change that will have negative impact in developer and user experience.

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.

Sorry for the random comments.

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.

Very concerned about this removal without a replacement that doesn't require the user to copy code from our KernelSyntaxExamples project. This feels like it should be a core concept of SK and helping users leverage LLMs.

At minimum, there should be an ADR to make sure the various SDK flavors are aligned on expectations going forward.

Approving though to remove my blocking review.

https://platform.openai.com/docs/guides/rate-limits/retrying-with-exponential-backoff

https://api.python.langchain.com/en/latest/_modules/langchain/llms/openai.html#completion_with_retry

@lemillermicrosoft lemillermicrosoft removed the PR: ready for review All feedback addressed, ready for reviews label Aug 21, 2023
@RogerBarreto
Copy link
Member Author

Closing this in favor of PR #2656

@RogerBarreto RogerBarreto deleted the features/remove-internal-retry-implementation branch December 5, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code PR: breaking change Pull requests that introduce breaking changes PR: paused PR temporarily parked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obsolete the public "WithRetry" and internal Retry logic implementation
4 participants