-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: Phase 1 of the declarative agent schema for review #10260
base: feature-declarative-agents
Are you sure you want to change the base?
.Net: Phase 1 of the declarative agent schema for review #10260
Conversation
c624bbd
to
e800600
Compare
Hi @markwallace-microsoft, Maybe support:
Unsure this helps, pretty much high level. |
Thanks for the feedback @joslat
|
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.
Copilot reviewed 79 out of 79 changed files in this pull request and generated 3 comments.
d961a0a
to
431c6ee
Compare
/// <summary> | ||
/// Tool definition type for code interpreter. | ||
/// </summary> | ||
public const string CodeInterpreter = "code_interpreter"; |
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.
What is the plan for 5 other tools introduced for Azure AI Agents?
(imho) code_interpreter
and file_search
are implementation specific and perhaps don't belong in the abstraction?
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.
My thinking is to include things that are cross service in the abstraction and others in the specific package, so the openai, bing etc. that Azure AI supports would go into that package.
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.
These aren't always available for every agent service (e.g. ChatCompletion). At best, it still strikes me as an over-generalization that doesn't have a lot of impact/value compared to the alternative. On the down side, it could create both implementation and usage confusion.
/// Determines if the agent definition has a code interpreter tool. | ||
/// </summary> | ||
/// <param name="agentDefinition">Agent definition</param> | ||
public static bool IsEnableCodeInterpreter(this AgentDefinition agentDefinition) |
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.
In alignment with previous comment, these might more propertly be defined on the agent specific extensions in the respective packages/projects.
if (agentDefinition.Type?.Equals(OpenAIAssistantAgentType, System.StringComparison.Ordinal) ?? false) | ||
{ | ||
var clientProvider = kernel.GetOpenAIClientProvider(agentDefinition); | ||
AssistantClient client = clientProvider.Client.GetAssistantClient(); |
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.
The client-provider pattern is being deprecated in favor of direct client usage.
KernelAgent? kernelAgent = null; | ||
if (agentDefinition.Type?.Equals(AzureAIAgentType, System.StringComparison.Ordinal) ?? false) | ||
{ | ||
var clientProvider = kernel.GetAzureAIClientProvider(agentDefinition); |
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.
The client-provider pattern is being deprecated in favor of direct client usage.
}; | ||
} | ||
|
||
return Task.FromResult<KernelAgent?>(kernelAgent).Result; |
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 return kernelAgent;
problematic?
}; | ||
} | ||
|
||
return Task.FromResult<KernelAgent?>(kernelAgent).Result; |
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 return kernelAgent;
problematic?
Adding support for |
/// <param name="agentDefinition">Definition of the agent to create.</param> | ||
/// <param name="cancellationToken">Optional cancellation token.</param> | ||
/// <return>The created <see cref="KernelAgent"/>, if null the agent type is not supported.</return> | ||
public abstract Task<KernelAgent?> CreateAsync(Kernel kernel, AgentDefinition agentDefinition, CancellationToken cancellationToken = default); |
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 the factory cannot produce the agent, perhaps it should throw instead of returning null
? (change return type to Task<KernelAgent>
?)
Verify.NotNull(agentDefinition.Model.Id); | ||
|
||
KernelAgent? kernelAgent = null; | ||
if (agentDefinition.Type?.Equals(OpenAIAssistantAgentType, System.StringComparison.Ordinal) ?? false) |
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.
Couldn't type validation be performed as a protected
method in KernelAgentFactory
since Types
is defined in the base abstraction? Then each factory won't have to implement custom validation. They can just call into the base method.
Be nice to see some comprehensive samples on this. |
Any thoughts on reconciling the support for the |
Is this going to be released as "GA" or have an experimental tag defined? |
Verify.NotNull(agentDefinition); | ||
|
||
// Use the agent configuration as the first option | ||
var configuration = agentDefinition?.Model?.Configuration; |
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'd expect that developers will not want to (or be allowed to) store secrets such as ApiKey
in the agent manifest.
Supporting the ability to resolve environment variables (at a minimum) or resolve other configurations settings may be prudent to consider.
#security
{ | ||
var hasEndpoint = configuration.TryGetValue(Endpoint, out var endpoint) && endpoint is not null; | ||
var hasApiKey = configuration.TryGetValue(ApiKey, out var apiKey) && apiKey is not null; | ||
if (hasApiKey && hasEndpoint) |
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 logic may be overly brittle. Isn't there the case where a non-azure endpoint can be targeted that requires an endpoint (such as a model gateway)?
It might be prudent to require explicit knowledge on whether Azure or OpenAI is intended.
Motivation and Context
Related #10224
Includes following
Description
Contribution Checklist