fix: bail out of get_provider('local') when sentence-transformers is missing#484
Open
mvanhorn wants to merge 1 commit into
Open
Conversation
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.
Summary
embed_graph_toolreturned{ok: true, embeddings: 0}on workspaces withoutsentence-transformersbecauseget_provider("local")only caughtImportErrorfrom theLocalEmbeddingProviderconstructor. The check existed (_check_available()) but wasn't consulted before the constructor ran, so the silent fallback path swallowed the unavailability and returned a no-op provider.Add an early
_check_available()short-circuit at the top ofget_providerso the call returnsNoneand surfaces "local provider unavailable" to the caller instead of producing a silent no-op.Why this matters
#448 (filed today by @nicobailon) reports embed runs that "succeed" with zero embeddings produced. The root cause is exactly the missing precondition:
_check_availableexists for this purpose but only gets called byEmbeddingStore. Hoisting it intoget_providermakes both paths consistent.Tests: 4 new tests cover both
localand embedding-store-with-local paths when_check_availablereturns False, plus 2 existing tests patched to assert the new precondition path still admits successful constructions.uv run pytest tests/test_embeddings.py -k "TestGetProviderModel or test_local_unavailable or test_embedding_store_unavailable"passes locally.closes #448
AI was used for assistance.