fix: raise provider-specific errors from vectorizer _set_model_dims() - #680
fix: raise provider-specific errors from vectorizer _set_model_dims()#680Aryan-Pardeshi wants to merge 5 commits into
Conversation
Each vectorizer probes its provider with a throwaway embedding call to learn the model dimensionality. On failure every one of the eight raised the same generic message under a 'TODO get more specific' comment, which told the caller nothing about which provider rejected them or what to change. Catch the exception classes each SDK actually raises and report the provider, the model, and the concrete next step. The broad 'except Exception' stays as a final clause so an unanticipated error still surfaces as a ValueError rather than escaping raw.
…atch actually fires Two layers were hiding the real SDK exception from _set_model_dims(): 1. _embed()/_embed_many() already catch the SDK's own exception and re-raise a generic ValueError, so catching the provider exception type directly in _set_model_dims() (as this PR originally did) never triggers -- Cursor Bugbot caught this on review. 2. _embed()/_embed_many() are @retry-decorated with retry_if_not_exception_type(TypeError), which does not exempt that ValueError -- so a permanent failure like bad credentials or an unknown model is retried 6 times with exponential backoff before tenacity gives up and raises RetryError, wrapping the ValueError, which itself wraps the SDK exception. _set_model_dims() now unwraps RetryError.last_attempt.exception() first, then unwraps __cause__/__context__ on what's left, before dispatching on the provider's real exception type. Also fixes HuggingFace separately: a bad model name raises OSError from SentenceTransformer() in _initialize_client(), before _set_model_dims() runs at all -- that OSError is now caught where it actually happens. Tests now drive the real _embed()/_initialize_client() code for OpenAI, Bedrock and HuggingFace (patching only the network client / model load, not _embed itself), with time.sleep patched so retry backoff doesn't stall the suite. The remaining five providers get a cause-dispatch test built the same way _embed really builds its wrapper: raised while handling the SDK exception, so __context__ is set by real Python chaining rather than fabricated. 11 passed in test_vectorizer_dim_errors.py (16s). Full unit suite: 1303 passed, 11 skipped -- no regressions from the previous 1299.
|
Good catch, and it went deeper than the first fix. There was a second layer underneath that too:
Full unit suite: 1303 passed, no regressions. |
…r/RetryError _embed_many() re-raises voyageai.error.InvalidRequestError as TypeError specifically so retry_if_not_exception_type(TypeError) skips retrying it -- a bad model id can never succeed regardless of attempt count. That means it reaches _set_model_dims() as a bare, unwrapped TypeError, never as ValueError or RetryError, so the unrecognized-model branch never fired. Caught by Cursor Bugbot on the second review pass. Verified: reverting the except tuple back to (ValueError, RetryError) makes the new test fail with the generic fallback message instead of the InvalidRequestError guidance.
|
Good catch again — pushed. Added |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 098e5e9. Configure here.
|
Both real, pushed a fix for each. High: right, cohere.UnauthorizedError/NotFoundError only exist in cohere 5.0+, but the package still declares cohere>=4.44 and the embed path still handles 4.x list responses. Those were referenced unconditionally at import/isinstance time, so any 4.x install would crash on construction. Now using getattr(cohere, "UnauthorizedError", None) etc., and the ApiError import is wrapped in try/except ImportError -- falls through to the generic message on 4.x instead of crashing. Low: fixed the copy-paste wording, HF's OSError handler now says "failed while determining its dimensions" like the RuntimeError one, since it fires after SentenceTransformer already loaded. Full suite: 1304 passed, 11 skipped, no regressions. |
vishal-bala
left a comment
There was a problem hiding this comment.
Hi, thanks for contributing to RedisVL! I appreciate the idea you're going for here, but the current implementation adds a lot of bloated logic and comments for limited lift to an end user. If the goal is to expose the underlying specific error more concretely, I think we can do that by simply chaining the exception into the ValueError
except (...) as ke:
raise ValueError("...") from keThat should make it sufficiently visible in the traceback and actionable by others catching this exception, while also maintaining the standard of code quality we expect for this project.
Per review on redis#680: the RetryError-unwrap-and-dispatch-by-SDK-type logic added a lot of bloated branching for limited lift. Collapse each provider's _set_model_dims() to one except Exception clause that wraps in a ValueError and chains the original exception with `from e`, which already surfaces the real cause in the traceback without the manual unwrapping.

Fixes #485
Every vectorizer probes its provider with a throwaway embedding call to learn the model's dimensionality. When that probe failed, all eight raised the same message under a
# fall back (TODO get more specific)comment:That tells the caller nothing about which provider rejected them or what to change, which is exactly the troubleshooting cost the issue describes.
Each
_set_model_dims()now catches the exception classes its SDK actually raises and reports the provider, the model, and a concrete next step:text/openai.py,text/azureopenai.py—AuthenticationError,PermissionDeniedError,NotFoundError,APIConnectionError. The Azure messages talk about deployments rather than models, since Azure addresses models by deployment name and a wrong deployment is the common failure.text/cohere.py—UnauthorizedError,NotFoundError, thencohere.core.api_error.ApiError.text/mistral.py—mistralai.models.SDKError.voyageai.py—AuthenticationError,InvalidRequestError,APIConnectionErrorfromvoyageai.error.bedrock.py—ClientError, split on the error code so credential problems and unknown model ids give different advice, thenBotoCoreError.vertexai.py—PermissionDenied,Unauthenticated,NotFound, thenGoogleAPICallError.text/huggingface.py—OSErrorfor a model that will not load,RuntimeErrorfor the CUDA OOM/device-mismatch case.Every class name was verified by importing the installed SDK rather than taken from documentation, since a catch for a class that does not exist is dead code.
Two deliberate choices:
The broad
except Exceptionstays as a final clause in all eight. Narrowing to provider classes alone would let anything unanticipated escape raw instead of surfacing as aValueError, which would be a regression rather than a fix. The generic clause now also names the provider and model.Imports are local to the method, matching how these modules already treat their optional SDK dependencies. The SDK is guaranteed importable at that point because
__init__initialises the client before calling_set_model_dims().tests/unit/test_vectorizer_dim_errors.pydrives OpenAI, Azure OpenAI, Bedrock and HuggingFace through their real__init__with the network client stubbed, so it exercises the same path a user hits with a bad key. It also pins the generic fallback: an unexpectedZeroDivisionErrormust still arrive as aValueError.Verified 7 failed against unmodified
main, 7 passed with the change. Full unit suite: 1299 passed, 11 skipped.isort --profile blackandblack --target-version py311report no changes.Note
Low Risk
Changes are limited to init-time error wrapping in embedding vectorizers; successful embed paths are unchanged and failures still surface as ValueError.
Overview
When vectorizers fail their startup dimension probe, failures are no longer reported as a generic
Error setting embedding model dimensions: …message. OpenAI, Azure OpenAI, Cohere, Mistral, Bedrock, Vertex AI, and VoyageAI now raise aValueErrorthat names the provider and model/deployment, with the underlying SDK or retry error preserved viaraise … from e. The separateKeyError/IndexErrorbranch for “unexpected API response” on that path is removed in favor of the single wrapped handler.Hugging Face gets the largest behavioral change:
SentenceTransformerload failures (OSError,RuntimeErrorsuch as CUDA OOM) are caught in_initialize_clientwith guidance on model path/download anddevice='cpu', and similar handling is added in_set_model_dimswhen the probe encode step fails.tests/unit/test_vectorizer_dim_errors.pylocks in model names in messages, exception chaining, HF load errors, VoyageAI’sTypeErrorpath from invalid models, and the generic fallback for unexpected errors.Reviewed by Cursor Bugbot for commit 5a1ebea. Bugbot is set up for automated code reviews on this repo. Configure here.