openai / openai-dotnet

The official .NET library for the OpenAI API
https://www.nuget.org/packages/OpenAI
MIT License
707 stars 60 forks source link

GetAssistantAsync does not return a useful value #62

Open dspear opened 2 weeks ago

dspear commented 2 weeks ago

AssistantClient.GetAssistantAsync only returns a ClientResult, not a ClientResult<Assistant>. Also, it should take a CancellationToken optional parameter, rather than just a RequestOptions optional parameter.

KrzysztofCwalina commented 1 week ago

I see the following overload that returns ClirntResult<Assitant> and accepts a cancellation token: public virtual AsyncPageableCollection<Assistant> GetAssistantsAsync(ListOrder? resultOrder = null, CancellationToken cancellationToken = default);

dspear commented 1 week ago

Yes, there is that, but that lists all of the Assistants you have. There's another function, GetAssistant, that takes a string ID for the assistant, and only returns info about that one Assistant. That's the one that doesn't return the typed result or take a CancellationToken.

trrwilson commented 1 week ago

This method is present, though currently still lacking the optional CancellationToken parameter:

https://github.com/openai/openai-dotnet/blob/aca190004662e5818fa21f22e09866398050ce86/src/Generated/AssistantClient.cs#L35

E.g. this code compiles:

AssistantClient client = new();
Assistant assistant = client.GetAssistant("existing-assistant-id");

...but will not yet compile if providing a CancellationToken.

@KrzysztofCwalina -- this was one of a few methods that didn't need any customization from the generated code, and thus it missed the pass over the methods under /src/Custom (since it only appeared in /src/Generated. I'll do a refresh to ensure we catch these remaining methods.

KrzysztofCwalina commented 1 week ago

fixed

dspear commented 6 days ago

@KrzysztofCwalina I checked this in the Beta.6 release, and this function is unchanged. It still does not take a CancellationToken parameter.

trrwilson commented 6 days ago

Thanks, @dspear -- yes, #70 is still pending and this should remain open until that's merged.