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

.Net: adds support for strict mode with OpenAI #9924

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

baywet
Copy link
Member

@baywet baywet commented Dec 10, 2024

fixes #9786
closes #9741

eiriktsarpalis and others added 23 commits December 10, 2024 07:39
Signed-off-by: Vincent Biret <[email protected]>
Signed-off-by: Vincent Biret <[email protected]>
@baywet baywet requested a review from a team as a code owner December 10, 2024 12:40
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Dec 10, 2024
@github-actions github-actions bot changed the title adds support for strict mode with OpenAI .Net: adds support for strict mode with OpenAI Dec 10, 2024
serverUrlOverride: new Uri("https://graph.microsoft.com/v1.0"));
serverUrlOverride: new Uri("https://graph.microsoft.com/v1.0"),
enableDynamicOperationPayload: false,
enablePayloadNamespacing: true);
Copy link
Member

Choose a reason for hiding this comment

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

Namespacing is only used when dynamic payload construction is enabled. So, enabling it won't make a difference since payload construction is disabled. Same in a few other samples.

@@ -178,7 +178,7 @@ public static async IAsyncEnumerable<ChatMessageContent> GetMessagesAsync(Assist

FunctionCallsProcessor functionProcessor = new(logger);
// This matches current behavior. Will be configurable upon integrating with `FunctionChoice` (#6795/#5200)
FunctionChoiceBehaviorOptions functionOptions = new() { AllowConcurrentInvocation = true, AllowParallelCalls = true };
FunctionChoiceBehaviorOptions functionOptions = new() { AllowConcurrentInvocation = true, AllowParallelCalls = true, AllowStrictSchemaAdherence = true };
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have the strict mode configurable? To keep it off by default and allow it to be enabled if necessary. This way, we can avoid unnecessary behavioral changes.

@@ -33,7 +33,7 @@ public string? ArgumentName
/// <summary>
/// The parameter type - string, integer, number, boolean, array and object.
/// </summary>
internal string Type { get; }
public string Type { get; }
Copy link
Member

Choose a reason for hiding this comment

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

The type and array item type parameters together with constructor were intentionally kept internal because their types will change during the migration to Microsoft.OpenApi V2. Consider adding a freezable setter to the Schema property, like the one for the ArgumentName property, so that a modified schema can be assigned.

{
CreatePayloadArtificialParameter(operation),
CreateContentTypeArtificialParameter(operation)
}.Select(p => parameterFilter(new(operation, p))).OfType<RestApiParameter>().ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}.Select(p => parameterFilter(new(operation, p))).OfType<RestApiParameter>().ToList();
}.Where(p => parameterFilter(new(operation, p)) is not null).ToList();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel.core kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net: Extend function calling model to support strict mode
6 participants