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

Copilot chat: ChatController #986

Merged

Conversation

TaoChenOSU
Copy link
Contributor

@TaoChenOSU TaoChenOSU commented May 14, 2023

Motivation and Context

We would like developers to be able to deploy SKaaS that is not tightly coupled to Copilot Chat.

Description

  1. Group Chat related components to their own folders.
  2. Strip out anything related to the current Chat features out of the SemanticKernelController.
  3. Create a ChatController specifically to invoke the ChatSkill.
  4. Refactor the WebApp to use the new ChatController. Make the service invocation pattern more consistent.
  5. Remove the SKBotAudienceMember and refactor ChatStatus. Note that this component does not currently take any effect. But this component will be useful moving forward with the multi user support.
  6. Disable implicit using.

Contribution Checklist

@TaoChenOSU TaoChenOSU added PR: in progress Under development and/or addressing feedback samples labels May 14, 2023
glahaye
glahaye previously approved these changes May 15, 2023
@TaoChenOSU TaoChenOSU force-pushed the users/taochen/copilot_chat_controller branch from f9b840f to ae04c25 Compare May 15, 2023 16:40
@TaoChenOSU TaoChenOSU requested a review from glahaye May 15, 2023 16:42
@TaoChenOSU TaoChenOSU added PR: ready for review All feedback addressed, ready for reviews and removed PR: in progress Under development and/or addressing feedback labels May 15, 2023
glahaye
glahaye previously approved these changes May 15, 2023
@TaoChenOSU TaoChenOSU added PR: in progress Under development and/or addressing feedback and removed PR: ready for review All feedback addressed, ready for reviews labels May 15, 2023
Copy link
Member

@Vivihung Vivihung left a comment

Choose a reason for hiding this comment

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

Thanks for the refactory. I've left some comments

@TaoChenOSU TaoChenOSU force-pushed the users/taochen/copilot_chat_controller branch 2 times, most recently from 6f17760 to ac0c20b Compare May 15, 2023 21:46
@TaoChenOSU TaoChenOSU added PR: ready for review All feedback addressed, ready for reviews and removed PR: in progress Under development and/or addressing feedback labels May 15, 2023
@TaoChenOSU TaoChenOSU requested a review from glahaye May 15, 2023 22:55
glahaye
glahaye previously approved these changes May 16, 2023
Vivihung
Vivihung previously approved these changes May 16, 2023
Copy link
Member

@Vivihung Vivihung left a comment

Choose a reason for hiding this comment

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

I worried the new project structure will confuse developers who are not familiar with the codebase. It's not straightforward to have two api project of the same app. And it's not common to include Controllers in a lib. I will let these big changes in but will work with Adrian offline to see if there's any mitigation.

Given said that, it's a great effort to separate components apart. Appreciate the huge refactory here.

@craigomatic
Copy link
Contributor

I worried the new project structure will confuse developers who are not familiar with the codebase. It's not straightforward to have two api project of the same app. And it's not common to include Controllers in a lib. I will let these big changes in but will work with Adrian offline to see if there's any mitigation.

Given said that, it's a great effort to separate components apart. Appreciate the huge refactory here.

I agree with Vivian, it's going to feel strange to have controllers in a project that does not appear to be runnable (I don't see a Program.cs)

Another pattern to consider:

  • Everything under your new copilotchat folder becomes 1 or more services (ChatService, DocumentService, etc) and anything aspnet specific disappears (controllers, etc)
  • Your services know nothing about the underlying hosting mechanism - it could be a console app, windows forms, aspnet, etc
  • They do the work you have in the controllers currently
  • We ship another aspnet app that uses the services so there remains a way for devs to deploy copilot chat

And if the objective is to allowing devs to deploy SKaaS with or without copilot chat, does this also imply that we should move the webapi project out of the copilot-chat-app folder?

@TaoChenOSU TaoChenOSU force-pushed the users/taochen/copilot_chat_controller branch from 82a2ed9 to 0cfbc0a Compare May 16, 2023 22:38
dotnet/SK-dotnet.sln Outdated Show resolved Hide resolved
@adrianwyatt adrianwyatt enabled auto-merge (squash) May 16, 2023 23:51
@adrianwyatt adrianwyatt merged commit 0d80c3e into microsoft:main May 16, 2023
19 checks passed
shawncal pushed a commit to johnoliver/semantic-kernel that referenced this pull request May 19, 2023
### Motivation and Context
We would like developers to be able to deploy SKaaS that is not tightly
coupled to Copilot Chat. This is the first step in creating more obvious boundaries between the WebApi service and CopilotChat specific components.

### Description
1. Group Chat related components to their own folders.
2. Strip out anything related to the current Chat features out of the
SemanticKernelController.
3. Create a ChatController specifically to invoke the ChatSkill.
4. Refactor the WebApp to use the new ChatController. Make the service
invocation pattern more consistent.
5. Remove the SKBotAudienceMember and refactor ChatStatus. Note that
this component does not currently take any effect. But this component
will be useful moving forward with the multi user support.
6. Disable implicit using.
golden-aries pushed a commit to golden-aries/semantic-kernel that referenced this pull request Oct 10, 2023
### Motivation and Context
We would like developers to be able to deploy SKaaS that is not tightly
coupled to Copilot Chat. This is the first step in creating more obvious boundaries between the WebApi service and CopilotChat specific components.

### Description
1. Group Chat related components to their own folders.
2. Strip out anything related to the current Chat features out of the
SemanticKernelController.
3. Create a ChatController specifically to invoke the ChatSkill.
4. Refactor the WebApp to use the new ChatController. Make the service
invocation pattern more consistent.
5. Remove the SKBotAudienceMember and refactor ChatStatus. Note that
this component does not currently take any effect. But this component
will be useful moving forward with the multi user support.
6. Disable implicit using.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready for review All feedback addressed, ready for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants