fix(rerank): validate credentials for explicitly configured providers - #4443
Open
Terminator666666 wants to merge 2 commits into
Open
fix(rerank): validate credentials for explicitly configured providers#4443Terminator666666 wants to merge 2 commits into
Terminator666666 wants to merge 2 commits into
Conversation
RerankConfig checked required fields for openai and litellm only. An explicit `provider: cohere` without api_key, or `provider: vikingdb` without ak/sk, was accepted at load time. is_available() then returned False and HierarchicalRetriever fell back to plain vector search, logging a single info-level line saying rerank was not configured. The new checks run against the effective provider, matching the existing openai and litellm branches. Auto-detection is unaffected, since detecting cohere already requires api_key and detecting vikingdb already requires ak and sk. An empty RerankConfig() still resolves to no provider and stays valid. Drops test_default_provider_is_vikingdb, which asserted a default that auto-detection replaced and had been failing on main. Rewrites test_unknown_provider_raises_value_error to actually cover an unknown provider and adds coverage for the two providers that were missing validation.
The rerank section described credential inference but not the fields each provider requires when provider is set explicitly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
RerankConfigvalidated required fields foropenaiandlitellmand skippedcohereandvikingdb. An explicitprovider: coherewithoutapi_key, orprovider: vikingdbwithoutak/sk, loaded without complaint.is_available()then returned False,HierarchicalRetrievertook its no-rerank branch, and the only trace was one info line reading "Rerank not configured".
Retrieval kept serving requests on plain vector search alone.
This PR adds the two missing checks so all four providers fail the same way.
provider: openai, noapi_baseprovider: litellm, nomodelprovider: cohere, noapi_keyprovider: vikingdb, noak/skprovidersetHuman Involvement
Related Issue
Fixes #4440
Type of Change
Changes Made
cohere(api_key) andvikingdb(ak,sk) inRerankConfig.validate_provider_fields(), next to the existingopenaiandlitellmbranches.test_unknown_provider_raises_value_error. It was named for an unknown provider butasserted the missing cohere check instead. Both cases now have their own test.
test_default_provider_is_vikingdb. It asserted a default that auto-detection replaced andhad been failing on main. The no-credentials case is already covered by
test_vikingdb_not_available_without_credentials.reference.
Testing
Reproduced before fixing. Revert the source change and the new test fails; put it back and the file
passes.
The two failures that disappear are the ones this PR addresses. The other 22 are pre-existing on main
and unrelated to rerank.
Environment: Linux, Python 3.11.16, main at 5e0754f.
Checklist
Screenshots (if applicable)
N/A
Additional Notes
Scope of the break: configs that name a provider and leave out its credentials. Those already got no
rerank at runtime, so nothing that works today stops working. Only the timing of the feedback changes.
Today the operator sees worse recall and no explanation. After this, the server refuses to start and
says which field is missing.
A warning instead of an error would avoid the break, but it puts the message in the same log stream as
the info line that already fails to get noticed. The two validated providers set the precedent here,
and matching them keeps one rule instead of four.
The new checks read the effective provider rather than the raw field, same as the branches above them,
so auto-detection is out of reach: inferring cohere already requires
api_key, inferring vikingdbalready requires
akandsk.RerankConfig()with no fields resolves to no provider and staysvalid.
cc @zhoujh01 @t0saki for the retrieval side.