-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor!: make provider_id mandatory in vector DB registration to avoid ambiguity BREAKING CHANGE #3147
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
base: main
Are you sure you want to change the base?
refactor!: make provider_id mandatory in vector DB registration to avoid ambiguity BREAKING CHANGE #3147
Conversation
121a858 to
820ec43
Compare
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.
@HabebNawatha see unit test failures, thanks
| ) | ||
| else: | ||
| raise ValueError("No provider available. Please configure a vector_io provider.") | ||
| provider_vector_db_id = provider_vector_db_id or vector_db_id |
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.
What is provider_vector_db_id and why does it take precedence here?
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.
The fallback provider_vector_db_id = provider_vector_db_id or vector_db_id is no longer needed
since the merged logic now uses provider_vector_db_id or vector_store_id downstream. Updated.
|
@franciscojavierarceo this is a breaking change? do you have a preferred strategy for rolling this out? |
| provider_vector_db_id: str | None = None, | ||
| vector_db_name: str | None = None, | ||
| ) -> VectorDB: | ||
| if provider_id is None: |
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.
typically i role out a log first to note that the change is breaking but i suppose it's too late for that in this change.
I commented on the PR but yeah typically i stagger releases by introduce logging first and then the break in the next release. This one is pretty easy though so just a call out in the release notes is probably fine. |
|
@HabebNawatha @franciscojavierarceo still relevant? Thanks! |
What does this PR do?
This PR improves vector database registration by making provider_id a mandatory field, aligning with best practices across the codebase and ensuring consistent, explicit behavior.
Previously, if multiple vector_io providers were registered and no provider_id was provided, the system would arbitrarily pick the first provider or raise a less-clear error. This change eliminates ambiguity and ensures users always specify which provider to use.
Closes #2834
Test Plan