semantic-kernel icon indicating copy to clipboard operation
semantic-kernel copied to clipboard

Published 20 hours ago • verified publisher icon microsoft

Replace kernel builder with constructor

Open johanste opened this issue 2 years ago • 2 comments

Motivation and Context

Attempt to make the code slightly more pythonic (work in progress).

Description

The builder pattern (including the chained configuration) felt a bit out of place for me as a python developer. This change removes the kernel builder and instead use keyword-only parameters in the constructor of the kernel.

Contribution Checklist

  • [ ] The code builds clean without any errors or warnings
  • [ ] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
  • [ ] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
  • [ ] All unit tests pass, and I have added new tests where possible
  • [ ] I didn't break anyone :smile:

johanste avatar Mar 22 '23 03:03 johanste

I want to chime in here and say, after some discussion, we are really looking to head toward making the Python version more idiomatic. (For context, I did a direct port from C# initially to kick-start things and now we'd love to make it more natural/pythonic.)

I fully support removing the builder-style Kernel creation. The C# port brings over many layers of indirection that really don't feel necessary for Python.

jjhenkel avatar Mar 22 '23 17:03 jjhenkel

There are several non-idiomatic aspects of this initial alpha, and that's intentional, to get the lib functional asap. There's going to be a second stage (very soon!) where we focus on code quality. We're preparing and prioritizing a list of items to tackle, and we'll do that strategically. This one specific change is also a particular one that I would leave it last, since it touches also the "design", so we could be spending days debating :-) There's also several design updates coming from the C# branch which is moving fast with other ideas, so we have plenty of work ahead.

dluc avatar Mar 24 '23 01:03 dluc

@jjhenkel @AdityaGudimella ok to move forward with this, there's a couple of conflicts to fix. Please let me know how you prefer to proceed

dluc avatar Apr 02 '23 23:04 dluc

@johanste would you like to update fixing the conflicts so we would merge?

dluc avatar Apr 05 '23 21:04 dluc

I fixed the conflicts, but I am not comfortable with how much (or rather little) testing I have done. Unfortunately, I'm going offline for two weeks starting... now... If anyone wants to take over this, it would be great; else I will pick it up again when I'm back.

johanste avatar Apr 06 '23 22:04 johanste

@mkarle @awharrison-28 could you review and test please?

dluc avatar Apr 07 '23 06:04 dluc

Reviewing and testing

awharrison-28 avatar Apr 07 '23 19:04 awharrison-28

FYI: Python PRs on hold while we stage to merge /python into main. See https://github.com/microsoft/semantic-kernel/pull/423

shawncal avatar Apr 12 '23 19:04 shawncal

@mkarle @dluc @johanste I took over development of the PR.

I updated documentation, fixed unit tests, and verified functionality of the notebooks on my local copy.

awharrison-28 avatar Apr 13 '23 18:04 awharrison-28

@johanste we've merged the python branch into main, and GitHub doesn't allow to point this PR to main because of some rebase steps (I tried and GH automatically closed the PR, without an option to reopen it).

Could you send the PR again?

dluc avatar Apr 14 '23 00:04 dluc