-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat!: Migrate Vector DB IDs to Vector Store IDs (breaking change) #3253
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
feat!: Migrate Vector DB IDs to Vector Store IDs (breaking change) #3253
Conversation
426b7cc to
a7a7af6
Compare
varshaprasad96
left a comment
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.
/lgtm
The changes look good. However, in longer term it would be good to rip Vector DB and support only vector stores directly. To be able to use vector dbs now, we are breaking the ability to pass vector_db_id to subsequent calls in vector_io which was possible before and this can be confusing.
2000krysztof
left a comment
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.
/lgtm
So this PR makes the That means that users would have to do something like follows in their application code: res = client.vector_dbs.register(
vector_db_id='my-vector-db-id',
embedding_model='ollama/all-minilm:l6-v2',
embedding_dimension=384,
)
vector_db_id = res.identifierAnd then the rest of their code would behave, including An alternative implementation would be to just delete the |
Signed-off-by: Francisco Javier Arceo <[email protected]>
dafa626 to
432ec7d
Compare
|
|
||
| vector_store_id = vector_store.id | ||
| actual_provider_vector_db_id = provider_vector_db_id or vector_store_id | ||
| logger.warning( |
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.
I think this is a reasonable way to highlight this to users.
|
@leseb mind taking a look at this one? |
|
|
||
| provider = self.impls_by_provider_id[provider_id] | ||
| logger.warning( | ||
| "VectorDB is being deprecated in future releases in favor of VectorStore. Please migrate your usage accordingly." |
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.
I don't understand, if it's deprecated (or want to deprecate soon), why convert the call to vector store under the hood? Can't we just fail the call and tell users to use VectorStore?
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.
I wanted to provide a lower friction migration.
The path would be:
- Create Vector Stores (this PR)
- Create Files and Vector Store Files for user (would be next PR)
Doing this would make the migration easier for users IMO. Under the hood, Vector Stores uses most of the VectorDB and VectorIO logic.
Given we have so many Red Hat users of RAG, I thought this was an easier path for them.
For some, they may just rip off the bandaid and that'd definitely be preferred but I'm happy to try and reduce the friction as much as possible.
leseb
left a comment
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.
|
I'm good with that. I'm happy to also include the other items I mentioned in this PR then too if @mattf and @ashwinb are comfortable with this. To be clear, I am in favor of deprecating the APIs and I am only doing this because (1) I know many Red Hat+ use cases rely on VectorIO and VectorDB APIs and (2) even though the Stack is still early in its adoption lifecycle, I want to treat users with care as we deprecate APIs and I believe this work will provide that. |
r3v5
left a comment
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.
Great work, @franciscojavierarceo ! lgtm
leseb
left a comment
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 hasn’t received much attention, and I’d like to move it forward. We’re on the right track, and precautions have been taken to minimize any impact on end users. Thank you!
This is needed after this breaking change in llama-stack: llamastack/llama-stack#3253 Signed-off-by: Jorge Garcia Oncins <[email protected]>
* fix: Obtain vector_db_id when registering the vectordb This is needed after this breaking change in llama-stack: llamastack/llama-stack#3253 Signed-off-by: Jorge Garcia Oncins <[email protected]> * fix: Add back uuid to vector_db identifier Signed-off-by: Jorge Garcia Oncins <[email protected]> * feat: set llama-stack-storage-size to 2Gi Signed-off-by: Jorge Garcia Oncins <[email protected]> --------- Signed-off-by: Jorge Garcia Oncins <[email protected]>
* fix: Obtain vector_db_id when registering the vectordb This is needed after this breaking change in llama-stack: llamastack/llama-stack#3253 Signed-off-by: Jorge Garcia Oncins <[email protected]> * fix: Add back uuid to vector_db identifier Signed-off-by: Jorge Garcia Oncins <[email protected]> * feat: set llama-stack-storage-size to 2Gi Signed-off-by: Jorge Garcia Oncins <[email protected]> --------- Signed-off-by: Jorge Garcia Oncins <[email protected]>
* fix: Obtain vector_db_id when registering the vectordb This is needed after this breaking change in llama-stack: llamastack/llama-stack#3253 * fix: Add back uuid to vector_db identifier * feat: set llama-stack-storage-size to 2Gi --------- Signed-off-by: Jorge Garcia Oncins <[email protected]> Co-authored-by: Jorge <[email protected]>
…o#662) * fix: Obtain vector_db_id when registering the vectordb This is needed after this breaking change in llama-stack: llamastack/llama-stack#3253 Signed-off-by: Jorge Garcia Oncins <[email protected]> * fix: Add back uuid to vector_db identifier Signed-off-by: Jorge Garcia Oncins <[email protected]> * feat: set llama-stack-storage-size to 2Gi Signed-off-by: Jorge Garcia Oncins <[email protected]> --------- Signed-off-by: Jorge Garcia Oncins <[email protected]>
What does this PR do?
This change migrates the VectorDB id generation to Vector Stores.
This is a breaking change for some users that may have application code using the
vector_db_idparameter in the request of the VectorDB protocol instead of theVectorDB.identifierin the response.By default we will now create a Vector Store every time we register a VectorDB. The caveat with this approach is that this maps the
vector_db_id→vector_store.name. This is a reasonable tradeoff to transition users towards OpenAI Vector Stores.As an added benefit, registering VectorDBs will result in them appearing in the VectorStores admin UI.
Why?
This PR makes the
POSTAPI call to/v1/vector-dbsswap thevector_db_idparameter in the request body into the VectorStore's name field and sets thevector_db_idto the generated vector store id (e.g.,vs_038247dd-4bbb-4dbb-a6be-d5ecfd46cfdb).That means that users would have to do something like follows in their application code:
And then the rest of their code would behave, including
VectorIO's insert protocol usingvector_db_idin the request.An alternative implementation would be to just delete the
vector_db_idparameter inVectorDBbut the end result would still require users having to writevector_db_id = res.identifiersinceVectorStores.create()generates the ID for you.So this approach felt the easiest way to migrate users towards VectorStores (subsequent PRs will be added to trigger
files.create()andvector_stores.files.create()).Test Plan
Unit tests and integration tests have been added.