-
Notifications
You must be signed in to change notification settings - Fork 221
Reorganize samples #242
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
Reorganize samples #242
Conversation
tracyboehrer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If we are going to keep
multi-turn-promptit needs to indicate "compat" since it is anActivityHandlerbased agent. We need to be clearer thatActivityHandlerbased agents are not the recommended path. DotNet will have some too, but more related to a "tutorial" on migration from BF SDK. implement-semantic-kernelis not consistent withintegrate-langchain. Also, too generic. There could be other SK or langChain samples. How aboutsemantickernel-weatheragentandlangchain-weatheragent.- That way when we add others they are grouped.
RetrievalBotis inconsistent: Case and "bot". But... it's more to that in the project and probably out of scope for this PR.- We will need to add
launchSettings.jsonto.gitignore(preferrably) or remove those files from the PR. - I assume the samples in the old locations will be removed in the same PR.
|
I wonder if the "compat" samples should be in a different folder? i.e., Not samples. They exist to demonstrate migration, and that folder can have a README with guidance. Not hard to imagine these will go away some day. In BF-land, when BF SDK v4 came out, there was a |
|
@tracyboeher
These names were available for discussion earlier in Agents SDK Sample Plan , but we can revisit them. I know nothing about semantic kernel or langchain. But felt that the goal of these samples wasn't about how to create a weather agent. I don't think these should have names involving 'weather agent' unless that is the goal of the sample. My understanding is that the weather agent is just an implementation detail to demonstrate something. But, I could be wrong. Only after reading the README could I come to understand that the goal of these samples is to demonstrate this capability that has different names for each language. That is the problem I'm trying to address. Are these known as 'orchestrators'? |
You are correct. "WeatherAgent" is just used to demonstrate. "SemanticKernel" is what we are showing. But... that likely won't be the only SK thing we demonstrate. What then? Furthermore, "streaming" is what we wanted to show, and we called it |
I expect that this existing sample is showing the basics of semantic kernel. Other samples will provide more complex/advanced capabilities. At that time, I think we could call them If you are recommending that we use |
I'm opposed to "implement", or "integrate". If I were to give a generic name I would say something like "sematickernel-multiturn". Do note you can't just change the folder name for DotNet projects. The csproj and then namespace would have to change as well. Some minor changes to README's because usually the name is used in there too. |
Not technically required, but expected. The WeatherAgent project builds just fine within the quickstart folder. |
…to sample-reorg
…to sample-reorg
There is only one JavaScript sample in the |
Leave it as what this PR does for now. The broader issue is we didn't have a goal of providing compat samples, except in the context of migration. i.e., All samples should be AgentApplication-based otherwise. This will have to be a follow up discussion on the engineering team about what we want to do around this. |
|
@rodrigobr-msft These changes are:
Make sense? |
|
@JimDaly NodeJS should also have the quickstart project. It can be referenced from the doc for convenience. Is there a reason it's not currently in the nodejs tree? |
@tracyboehrer |
We can use |
This PR implements the changes proposed in Agents SDK Sample Plan doc
Companion PR makes changes to paths in the docs: https://github.com/MicrosoftDocs/businessapps-copilot-docs-pr/pull/979
This also adds the Python samples.