-
Couldn't load subscription status.
- Fork 618
.NET: Add DevUI package for .NET #1603
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
base: main
Are you sure you want to change the base?
Conversation
b48577a to
d7ea3a4
Compare
d7ea3a4 to
334da63
Compare
334da63 to
bb830c1
Compare
bb830c1 to
ef4b7c8
Compare
5234a16 to
c45b945
Compare
...et/tests/Microsoft.Agents.AI.Workflows.UnitTests/Sample/07_GroupChat_Workflow_HostAsAgent.cs
Show resolved
Hide resolved
51646c1 to
6bb8eb8
Compare
|
I think this is a good implementation. When will this PR be merged? |
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.
Quite a big PR, so could miss something. I pulled the branch and played a bit with it. It works amazingly!
Suggesting to include the devui for our sample app as well (included changes here, take them if you like please #1668). I've also fancy-named the URLs in AgentHost to be able to reach devui easily from aspire dashboard

Question: when I launched devui via your sample + my sample I got git changes on index.js + python package-lock (see pic). Should we gitignore it / why not? I guess that's because I ran it first time (does not appear now when i build), just curious on what's the best practice here

|
Regarding @DeagleGross's question:
@victordibia, what is your opinion: should we add these to .gitignore or should we require a developer manually re-builds the devui assets when there is a change? EDIT: I modified vite config to hopefully make the build deterministic. So far, it's worked for me. Let's see how it goes. |
6bb8eb8 to
4e0cff3
Compare
8b48992 to
dfea997
Compare
| // Register sample workflows | ||
| var assistantBuilder = builder.AddAIAgent("workflow-assistant", "You are a helpful assistant in a workflow."); | ||
| var reviewerBuilder = builder.AddAIAgent("workflow-reviewer", "You are a reviewer. Review and critique the previous response."); | ||
| builder.AddSequentialWorkflow( |
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.
Are we duplicating patterns onto the builder?
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.
I don't quite understand - these aren't new methods I'm adding, just a sample
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.
I was not aware @DeagleGross added these in #1359. Question still stands, just not for this PR
| builder.AddAIAgent("my-agent", "You are a helpful assistant."); | ||
|
|
||
| // Add DevUI services | ||
| builder.AddDevUI(); |
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.
With this showing up in public surface area, do we need a more formal name / agent-specific name than "DevUI"?
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.
Yes, probably. Should that be in this PR?
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.
Don't block on it
| namespace Microsoft.Agents.AI.Hosting.OpenAI.Conversations; | ||
|
|
||
| /// <summary> | ||
| /// Minimal API endpoints for OpenAI Conversations API. | ||
| /// </summary> | ||
| public static class ConversationsHttpApi | ||
| { | ||
| /// <summary> | ||
| /// Maps an OpenAI Conversations API to the specified <see cref="IEndpointRouteBuilder"/>. | ||
| /// </summary> | ||
| public static IEndpointConventionBuilder MapConversations(this IEndpointRouteBuilder endpoints) |
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.
It's unexpected that this is in the DevUI package. Does it need to be exposed publicly or could it just be done implicitly as part of MapDevUI? And if it must be public, can it be moved to the same package as the other OpenAI stuff?
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.
I think its specific to DevUI. We should not move it in this form to OpenAI package IMO. But we probably should hide it, I agree
| JsonSerializerOptions options = new(ConversationsJsonContext.Default.Options); | ||
|
|
||
| // Chain with AIContent types from Microsoft.Extensions.AI | ||
| options.TypeInfoResolverChain.Add(AIJsonUtilities.DefaultOptions.TypeInfoResolver!); |
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.
Do we need any of the AIAgent-related types? If yes, this should probably chain with AgentAbstractionsJsonUtilities.DefaultOptions instead (and it in turn chains in AIJsonUtilities.DefaultOptions).
| internal sealed partial class ConversationsJsonContext : JsonSerializerContext | ||
| { | ||
| } |
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.
Nit: you can just use a ; after JsonSerializerContext rather than empty braces
| { | ||
| writer.WriteStartObject(); | ||
|
|
||
| // Convert TextContent to OpenAI format |
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.
There are routines in M.E.AI.OpenAI for converting M.E.AI.ChatMessages into Responses ResponseItems and Chat Completion ChatMessages. Not sure that'll help you, but wanted to call it out.
| /// </summary> | ||
| public static IHostApplicationBuilder AddDevUI(this IHostApplicationBuilder builder) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(builder); |
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.
It's so refreshing reading code that only targets .NET Core and can use such APIs :)
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.
We should do this everywhere :)
| { | ||
| var encoding = acceptEncoding[i]; | ||
|
|
||
| if (encoding.Quality is not 0 && |
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.
Why does this need to check for Quality != 0?
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.
A value of 0 means not accepted
| // Add condition name if this is a conditional edge | ||
| if (directEdge.HasCondition) | ||
| { | ||
| edge["condition_name"] = "condition"; |
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.
Is "condition" the right value here? I'd have expected it to be something more customized to the condition, like the condition itself, a description of it, etc.
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.
This is just there to make the shape right for DevUI. I copied this from Victor's DevUI proof of concept. This should be the name of the condition (assuming we could fetch that, but it's not accessible in this part of the model) or something generic if we cannot get a good name for the condition. Python code uses or in that event.
| { | ||
| await foreach (var agent in agentCatalog.GetAgentsAsync(cancellationToken).ConfigureAwait(false)) | ||
| { | ||
| if (agent.GetType().Name == "WorkflowHostAgent") |
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.
This seems pretty fragile and that there's a decent chance the internal type will get renamed, no one will know to update this, and it'll just silently be skipped
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.
It will break DevUI if/when it does. It's a hack for sure.
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.
I recommend we add an API that allows the app to pass in agents that should be excluded from the devUI. In the prototype Luis had built up, he hid "internal" agents from the devui where a workflow wrapped around those agents, so that only the workflow agent would be shown and the others would not be. He did that by implementing a filter in his corresponding method for discovering the registered agents.
dfea997 to
f98bfa4
Compare
|
|
||
| 3. Add DevUI services and map the endpoint: | ||
| ```csharp | ||
| builder.AddDevUI(); |
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.
It felt odd to me to need to AddDevUI regardless of environment, but I understand we don't have the environment until calling Build().
| "environmentVariables": { | ||
| "ASPNETCORE_ENVIRONMENT": "Development" | ||
| }, | ||
| "applicationUrl": "https://localhost:57966;http://localhost:57967" |
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.
Could we add /devui/ to the applicationUrl to have it launch the devui automatically?
| context.Response.StatusCode = StatusCodes.Status404NotFound; | ||
| } | ||
|
|
||
| private async Task<bool> TryServeResourceAsync(HttpContext context, string resourcePath) |
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.
We'll want to make sure to cover this behavior thoroughly in the threat model to make sure this can't ever serve unexpected resources (without relying on applications gating the devui on Development environment).
| // Map required endpoints | ||
| app.MapEntities(); | ||
| app.MapOpenAIResponses(); | ||
| app.MapConversations(); |
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.
When I see 'Entities', I think of my business model entities (ala EF). Augmenting the comment would add context to clarify that 'Entities' here are the agents/workflows.
| // Map required endpoints | |
| app.MapEntities(); | |
| app.MapOpenAIResponses(); | |
| app.MapConversations(); | |
| // Map required endpoints for agents and workflows | |
| app.MapEntities(); | |
| app.MapOpenAIResponses(); | |
| app.MapConversations(); |
| { | ||
| await foreach (var agent in agentCatalog.GetAgentsAsync(cancellationToken).ConfigureAwait(false)) | ||
| { | ||
| if (agent.GetType().Name == "WorkflowHostAgent") |
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.
I recommend we add an API that allows the app to pass in agents that should be excluded from the devUI. In the prototype Luis had built up, he hid "internal" agents from the devui where a workflow wrapped around those agents, so that only the workflow agent would be shown and the others would not be. He did that by implementing a filter in his corresponding method for discovering the registered agents.
Motivation and Context
This adds a DevUI NuGet package for .NET and implements just enough API surface to support it. Some of what is implemented here (eg Conversations API) should be replaced by first-class implementations rather than what I've implemented in the DevUI project. HOWEVER, for this PR, I am happy with this hacked together implementation for now to promote forward progress.
I also fixed just enough of the Responses API implementation to make it work, but that will be replaced by #1550.To try it out, run the DevUI_Step01_BasicUsage sample.
Contribution Checklist