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

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 error handling during vectorizer setup; embedding and retry behavior are unchanged aside from clearer failures, with a broad fallback still converting unexpected errors to ValueError.
Overview
When vectorizer initialization’s dimension probe fails, callers now get actionable
ValueErrormessages (provider, model/deployment, and what to fix) instead of a generic “Error setting embedding model dimensions” string.Across OpenAI, Azure OpenAI, Cohere, Mistral, VoyageAI, Bedrock, Vertex AI,
_set_model_dims()now unwrapsRetryErrorand chained causes from retried_embed()failures, then maps known SDK errors (auth, unknown model/deployment, connectivity, etc.) to tailored text. Hugging Face additionally turns model load (OSError/RuntimeErrorin_initialize_client) and probe failures into clearer messages.Adds
tests/unit/test_vectorizer_dim_errors.pywith end-to-end and cause-dispatch tests so messages and the unwrap logic stay correct.Reviewed by Cursor Bugbot for commit bc8205d. Bugbot is set up for automated code reviews on this repo. Configure here.