-
Notifications
You must be signed in to change notification settings - Fork 545
Audit protocol types #892
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?
Audit protocol types #892
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -411,7 +411,7 @@ public async Task<JsonRpcResponse> SendRequestAsync(JsonRpcRequest request, Canc | |
// Set request ID | ||
if (request.Id.Id is null) | ||
{ | ||
request = request.WithId(new RequestId(Interlocked.Increment(ref _lastRequestId))); | ||
request.Id = new RequestId(Interlocked.Increment(ref _lastRequestId)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now mutates the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order to avoid potential regressions in the scope of this change, I would suggest preserving the original cloning semantics even if done manually. |
||
} | ||
|
||
_propagator.InjectActivityContext(activity, request); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ public sealed class Argument | |
/// Gets or sets the name of the argument being completed. | ||
/// </summary> | ||
[JsonPropertyName("name")] | ||
public string Name { get; set; } = string.Empty; | ||
public required string Name { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW introducing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. And the same is true for other changes made in this PR (e.g., |
||
|
||
/// <summary> | ||
/// Gets or sets the current partial text value for which completion suggestions are requested. | ||
|
@@ -25,5 +25,5 @@ public sealed class Argument | |
/// options should be generated. | ||
/// </remarks> | ||
[JsonPropertyName("value")] | ||
public string Value { get; set; } = string.Empty; | ||
} | ||
public required string Value { get; set; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ public sealed class CreateMessageRequestParams : RequestParams | |
/// The client may ignore this request. | ||
/// </remarks> | ||
[JsonPropertyName("includeContext")] | ||
public ContextInclusion? IncludeContext { get; init; } | ||
public ContextInclusion? IncludeContext { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the maximum number of tokens to generate in the LLM response, as requested by the server. | ||
|
@@ -29,13 +29,13 @@ public sealed class CreateMessageRequestParams : RequestParams | |
/// response length and computation time. The client may choose to sample fewer tokens than requested. | ||
/// </remarks> | ||
[JsonPropertyName("maxTokens")] | ||
public int? MaxTokens { get; init; } | ||
public required int MaxTokens { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This property was an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use the schema as our source of truth when defining these model types. |
||
|
||
/// <summary> | ||
/// Gets or sets the messages requested by the server to be included in the prompt. | ||
/// </summary> | ||
[JsonPropertyName("messages")] | ||
public required IReadOnlyList<SamplingMessage> Messages { get; init; } | ||
public IList<SamplingMessage> Messages { get; set; } = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this changed back to optional? Presumably not required by the schema? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking here was that this property is still effectively "required" in that it's not declared as allowing However, your comment made me realize that removing Curious about your thoughts - do you think it's fine to have a default initial value as a substitute for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or maybe we should consider enabling |
||
|
||
/// <summary> | ||
/// Gets or sets optional metadata to pass through to the LLM provider. | ||
|
@@ -46,7 +46,7 @@ public sealed class CreateMessageRequestParams : RequestParams | |
/// that are specific to certain AI models or providers. | ||
/// </remarks> | ||
[JsonPropertyName("metadata")] | ||
public JsonElement? Metadata { get; init; } | ||
public JsonElement? Metadata { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the server's preferences for which model to select. | ||
|
@@ -66,7 +66,7 @@ public sealed class CreateMessageRequestParams : RequestParams | |
/// </para> | ||
/// </remarks> | ||
[JsonPropertyName("modelPreferences")] | ||
public ModelPreferences? ModelPreferences { get; init; } | ||
public ModelPreferences? ModelPreferences { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets optional sequences of characters that signal the LLM to stop generating text when encountered. | ||
|
@@ -84,7 +84,7 @@ public sealed class CreateMessageRequestParams : RequestParams | |
/// </para> | ||
/// </remarks> | ||
[JsonPropertyName("stopSequences")] | ||
public IReadOnlyList<string>? StopSequences { get; init; } | ||
public IList<string>? StopSequences { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets an optional system prompt the server wants to use for sampling. | ||
|
@@ -93,11 +93,11 @@ public sealed class CreateMessageRequestParams : RequestParams | |
/// The client may modify or omit this prompt. | ||
/// </remarks> | ||
[JsonPropertyName("systemPrompt")] | ||
public string? SystemPrompt { get; init; } | ||
public string? SystemPrompt { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the temperature to use for sampling, as requested by the server. | ||
/// </summary> | ||
[JsonPropertyName("temperature")] | ||
public float? Temperature { get; init; } | ||
public float? Temperature { get; set; } | ||
} |
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 was implicitly
string.Empty
before, but now it's explicit. This PR doesn't introduce a behavioral change here, but we should decide whether the empty string should really be used here, as it's not actually a valid URI. It would also be strange to usedataContent.Uri
, because then the data is specified in two places (once inBlob
and once inUri
).