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

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

Open
Clive321A opened this issue Oct 2, 2024 · 3 comments
Open

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

Clive321A opened this issue Oct 2, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@Clive321A
Copy link

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
@Clive321A Clive321A added the bug Something isn't working label Oct 2, 2024
@trrwilson
Copy link
Collaborator

trrwilson commented Oct 3, 2024

@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
Copy link
Author

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
Copy link

gmantri commented Oct 15, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants