Repository navigation
.Net: Global exceptions vs storage specific exceptions #10883
Description
Important exceptions like “the collection doesn’t exists” should be agnostic of the storage type, to avoid the need to catch several exception types.
For instance, this code would be preferable and wouldn’t need changes when swapping storage type:
Try { … }
catch (Microsoft.Extensions.VectorData.CollectionNotFoundException e)
{ … }as opposed to:
Try { … }
catch (Exception e) when (e is Qdrant.CollectionNotFoundException or Postgres.CollectionNotFoundException or AzureAiSearch.CollectionNotFoundException or Milvus.CollectionNotFoundException or ...)
{ … }
Activity
[-]Global exceptions vs storage specific exceptions[/-][+].Net: Global exceptions vs storage specific exceptions[/+]westey-m commentedon Mar 11, 2025
All connectors are expected to return standard exceptions. Any deviation is a bug.
See the following link for the specification on this:
https://learn.microsoft.com/en-us/semantic-kernel/concepts/vector-store-connectors/how-to/build-your-own-connector?pivots=programming-language-csharp#11-standard-exceptions
Adam Sitnik (@adamsitnik), one that would be good to cover in our specification tests as well to confirm.
markwallace-microsoft commentedon Mar 11, 2025
This is the current design so the remaining work to be done here is to verify this via integration tests for all connectors.
adamsitnik commentedon Mar 11, 2025
We should definitely always throw
VectorStoreOperationException, but throwing more specific exception may be tricky. Example: when the collection does not exist and we try to remove it, Redis client throwsRedisServerException, but it does not set an error code of any kind (at least to my knowledge), just a specific exception message (Unknown Index name). I am afraid that it can change over time (or simply get translated at some point), so relying on it to map it toCollectionNotFoundExceptioncould be very fragile.dluc commentedon Mar 12, 2025
I think we need to define a few important exceptions such as
Not Found,Invalid Request,ConflictandAccess Denied.Otherwise, wrapping every storage exception in a generic
OperationExceptionwould still require each app to inspect the error with custom logic (e.g. custom string matching) for each storage type. Everyone would have to do this, making the code less portable since string matching would be specific to individual engines. Copy&paste expectations would fail, etc.If this abstraction wants to be useful, it should align the behavior for important scenarios. This is the same concern I see with different key types (Guid, string, composite keys, long, ulong, etc) which lead to code duplication and lengthy
switchstatements to handle each storage engine.As for breaking changes, the risk is always present, but it can be mitigated with integration tests for each storage engine. The core team can focus on strong test coverage for Azure storage options, while the broader OSS community can support with integration tests for third party connectors.
.Net MEVD: More Conformance tests (#10944)
adamsitnik commentedon Mar 18, 2025
#10958 has made some good progress, but we need to have a complete conformance test suite (#10194) to be 100% sure we don't throw any storage specific exceptions
roji commentedon Mar 31, 2025
Devis Lucato (@dluc) as in #10944, in many cases the different databases simply generally don't provide us with the information to do this. It's unfortnuate, but that's how it is. From all my time working on EF, this kind of request occasionally comes up, but in general users don't actually need to distinguish between e.g.
Access DefniedandNot Found- it simply isn't a distinction that actually requires two different code paths; the operation generally fails, the exception shown or logged, and that's it.roji commentedon Apr 15, 2025
Discussed offline - we don't currently intend to try introduce specific exceptions such as "not found" or "access denied", for the reasons detailed above; this is something we can also do later if needed without it introducing a breaking change (the new specific exceptions would simply extend VectorStoreOperationException.
The separate verification that we're indeed wrapping with VectorStoreOperationException everywhere is tracked by #11512, so I'll close this for now.
.Net MEVD: More Conformance tests (microsoft#10944)