openai / openai-dotnet

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

GetVectorStoreAsync is now internal rather than public in V2.0,0 #240

Open Clive321A opened 1 month ago

Clive321A commented 1 month ago

Service

OpenAI

Describe the bug

Should this method have been public rather than internal in V2.0.0?

    /// <summary>
    /// Gets an instance representing an existing <see cref="VectorStore"/> based on its ID.
    /// </summary>
    /// <param name="vectorStoreId"> The ID of the vector store to retrieve. </param>
    /// <param name="cancellationToken"> A token that can be used to cancel this method call. </param>
    /// <returns> A representation of an existing <see cref="VectorStore"/>. </returns>
    internal virtual async Task<ClientResult<VectorStore>> GetVectorStoreAsync(string vectorStoreId, CancellationToken cancellationToken = default)
    {
        Argument.AssertNotNullOrEmpty(vectorStoreId, nameof(vectorStoreId));

        ClientResult result
            = await GetVectorStoreAsync(vectorStoreId, cancellationToken.ToRequestOptions()).ConfigureAwait(false);
        return ClientResult.FromValue(
            VectorStore.FromResponse(result.GetRawResponse()), result.GetRawResponse());
    }

Steps to reproduce

There is no GetVectorStoreAsync available, only GetVectorStore, Im wondering if this was intentional or not?

Im updating from 2.0.0-beta.7 to 2.0.0

Code snippets

No response

OS

winOS

.NET version

.net 8

Library version

2.0.0

trrwilson commented 1 month ago

@Clive321A:

Amended edit: after checking back with the team, I missed that this was one of the intended moves with the introduction of operation abstractions for vector stores -- VectorStore is now present as the Value on CreateVectorStoreOperation.

If it's possible, could you share more about the overall task you're accomplishing with fetching the Vector Store? In isolation, the new operation pattern is clearly not an ideal 1:1 replacement for simply fetching the store by ID -- you'd rehydrate the CreateVectorStoreOperation from a token and then retrieve its Value -- but it may be considerably more intuitive in the context of the overall application flow (like using a vector store that was just created).

~@Clive321A, thank you for the report! This change in visibility was made in error and we'll correct it in the next round of release.~

Clive321A commented 1 month ago

Thanks for the reply, Just having another look through, I think with the latest version I could probably refactor to mostly not use it, I had just focused on updating.

1) After creating Vector Store, checking its status while its not complete (Can probably refactor this to not use it) 2) Looping through every vector store and removing file attachments.(Can probably refactor this to not use it) 3) Adding a file to an existing VectorStore where I have the VectorStore ID stored locally (This one I think I need to use it) as im retrieving the VectorStore, then removing all files attached to it, then adding a new file. 4) Logging the status of a VectorStore, this was used when I was getting random vector store issues, so im retireving a VectorStore from a locally stored ID, then adding the VectorStore status to the log.

I do think it is worth retaining as Public rather than internal.

gmantri commented 1 month ago

@Clive321A:

Amended edit: after checking back with the team, I missed that this was one of the intended moves with the introduction of operation abstractions for vector stores -- VectorStore is now present as the Value on CreateVectorStoreOperation.

If it's possible, could you share more about the overall task you're accomplishing with fetching the Vector Store? In isolation, the new operation pattern is clearly not an ideal 1:1 replacement for simply fetching the store by ID -- you'd rehydrate the CreateVectorStoreOperation from a token and then retrieve its Value -- but it may be considerably more intuitive in the context of the overall application flow (like using a vector store that was just created).

~@Clive321A, thank you for the report! This change in visibility was made in error and we'll correct it in the next round of release.~

So we have GetVectorStoreAsync as internal however GetVectorStore (Sync version) as public. Why this anomaly?


    /// <summary>
    /// Gets an instance representing an existing <see cref="VectorStore"/> based on its ID.
    /// </summary>
    /// <param name="vectorStoreId"> The ID of the vector store to retrieve. </param>
    /// <param name="cancellationToken"> A token that can be used to cancel this method call. </param>
    /// <returns> A representation of an existing <see cref="VectorStore"/>. </returns>
    internal virtual async Task<ClientResult<VectorStore>> GetVectorStoreAsync(string vectorStoreId, CancellationToken cancellationToken = default)
    {
        Argument.AssertNotNullOrEmpty(vectorStoreId, nameof(vectorStoreId));

        ClientResult result
            = await GetVectorStoreAsync(vectorStoreId, cancellationToken.ToRequestOptions()).ConfigureAwait(false);
        return ClientResult.FromValue(
            VectorStore.FromResponse(result.GetRawResponse()), result.GetRawResponse());
    }

    /// <summary>
    /// Gets an instance representing an existing <see cref="VectorStore"/> based on its ID.
    /// </summary>
    /// <param name="vectorStoreId"> The ID of the vector store to retrieve. </param>
    /// <param name="cancellationToken"> A token that can be used to cancel this method call. </param>
    /// <returns> A representation of an existing <see cref="VectorStore"/>. </returns>
    public virtual ClientResult<VectorStore> GetVectorStore(string vectorStoreId, CancellationToken cancellationToken = default)
    {
        Argument.AssertNotNullOrEmpty(vectorStoreId, nameof(vectorStoreId));

        ClientResult result = GetVectorStore(vectorStoreId, cancellationToken.ToRequestOptions());
        return ClientResult.FromValue(VectorStore.FromResponse(result.GetRawResponse()), result.GetRawResponse());
    }

In our case, we would want to store the Id of the vector store in our database and then check if the vector store exists before performing any operations on that. This is where GetVectorStoreAsync method will be very useful for us.