Skip to content

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Oct 11, 2024

Closes #5499

Microsoft Reviewers: Open in CodeFlow

@stephentoub stephentoub requested a review from a team as a code owner October 11, 2024 20:21
@stephentoub stephentoub added the area-ai Microsoft.Extensions.AI libraries label Oct 11, 2024
@stephentoub stephentoub enabled auto-merge (squash) October 12, 2024 11:26
/// It is expected that all implementations of <see cref="IChatClient"/> support being used by multiple requests concurrently.
/// </para>
/// <para>
/// However, implementations of <see cref="IChatClient"/> may mutate the arguments supplied to <see cref="CompleteAsync"/> and
Copy link
Member

Choose a reason for hiding this comment

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

"May" seems a bit weak given that mutating the list is the standard mechanism for updating the chat history.

Suggested change
/// However, implementations of <see cref="IChatClient"/> may mutate the arguments supplied to <see cref="CompleteAsync"/> and
/// However, implementations of <see cref="IChatClient"/> do mutate the arguments supplied to <see cref="CompleteAsync"/> and

Copy link
Member Author

@stephentoub stephentoub Oct 12, 2024

Choose a reason for hiding this comment

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

To me that suggests that all implementations do, which isn't the case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
/// However, implementations of <see cref="IChatClient"/> may mutate the arguments supplied to <see cref="CompleteAsync"/> and
/// However, implementations of <see cref="IChatClient"/> are permitted to mutate the arguments supplied to <see cref="CompleteAsync"/> and

@stephentoub stephentoub merged commit 412305b into dotnet:main Oct 12, 2024
6 checks passed
@stephentoub
Copy link
Member Author

Ah, forgot I'd enabled auto merge :)

@stephentoub stephentoub deleted the threadsafetycomments branch October 28, 2024 21:16
@github-actions github-actions bot locked and limited conversation to collaborators Nov 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ai Microsoft.Extensions.AI libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review all IChatClient implementations for thread-safety and suitability as singleton

2 participants