.Net: Change VolatileMemoryStore.CreateCollectionAsync to not throw when collection already exists

[dehoward]

Motivation and Context

today the VolatileMemoryStore will throw when creating a collection if it already exists. other stores (e.g. Postgres, DuckDB) follow a different pattern and return gracefully. throwing an error can create a problem for parallel writes to the memory store, which is being added in ChatCopilot here.

Description

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations
• All unit tests pass, and I have added new tests where possible
• I didn't break anyone 😄

[dsgrieve]

I think this is a good change but it is contrary to the contract of CreateCollectionAsync in IMemoryStore. If you're going to do this, then I think you need to change the contract of CreateCollectionAsync and make this change to all implementations of IMemoryStore.

[dehoward]

I think this is a good change but it is contrary to the contract of CreateCollectionAsync in IMemoryStore. If you're going to do this, then I think you need to change the contract of CreateCollectionAsync and make this change to all implementations of IMemoryStore.

@dsgrieve contrary in what way? the contract of IMemoryStore.CreateCollectionAsync doesn't specify what the behavior should be. DuckDB, Postgres, and Redis already implement this while Pinecone, Redis, and Chroma don't, so there doesn't seem to be a clear pattern here.

with that said, I think I could update this change to check to see if the collection already exists before throwing the error, which is more similar to what we're doing in other places.

[dsgrieve]

I think this is a good change but it is contrary to the contract of CreateCollectionAsync in IMemoryStore. If you're going to do this, then I think you need to change the contract of CreateCollectionAsync and make this change to all implementations of IMemoryStore.

@dsgrieve contrary in what way? the contract of IMemoryStore.CreateCollectionAsync doesn't specify what the behavior should be. DuckDB, Postgres, and Redis already implement this while Pinecone, Redis, and Chroma don't, so there doesn't seem to be a clear pattern here.

with that said, I think I could update this change to check to see if the collection already exists before throwing the error, which is more similar to what we're doing in other places.

At the time I ported IMemoryStore.cs to Java, I recall the code throwing an exception if the collection already existed. Now, it seems that the .NET implementation has drifted quite far from the Java implementation and I have some catching up to do.

Sorry for the noise.

[dmytrostruk]

DuckDB, Postgres, and Redis already implement this while Pinecone, Redis, and Chroma don't, so there doesn't seem to be a clear pattern here

@dehoward @dsgrieve I think it is expected behavior because different DBs have different API. It depends how consumers will use IMemoryStore interface. For example, SemanticTextMemory has a check if collection exists before its creation:

semantic-kernel/dotnet/src/SemanticKernel/Memory/SemanticTextMemory.cs

Lines 42 to 45 in 8552943

	if (!(await this._storage.DoesCollectionExistAsync(collection, cancellationToken).ConfigureAwait(false)))
	{
	await this._storage.CreateCollectionAsync(collection, cancellationToken).ConfigureAwait(false);
	}

The only difference would be when you use concrete DB directly instead of using it through Kernel.

In case of VolatileMemoryStore, since it's ConcurrentDictionary, I wouldn't throw an exception, just to make CreateCollection operation idempotent.

[shawncal]

I think this is a good change but it is contrary to the contract of CreateCollectionAsync in IMemoryStore. If you're going to do this, then I think you need to change the contract of CreateCollectionAsync and make this change to all implementations of IMemoryStore.

@dsgrieve contrary in what way? the contract of IMemoryStore.CreateCollectionAsync doesn't specify what the behavior should be. DuckDB, Postgres, and Redis already implement this while Pinecone, Redis, and Chroma don't, so there doesn't seem to be a clear pattern here.
with that said, I think I could update this change to check to see if the collection already exists before throwing the error, which is more similar to what we're doing in other places.

At the time I ported IMemoryStore.cs to Java, I recall the code throwing an exception if the collection already existed. Now, it seems that the .NET implementation has drifted quite far from the Java implementation and I have some catching up to do.

Sorry for the noise.

Not noise. This is a good consideration, and was also my instinct too. I'm not sure this is what that function should do, but whatever it does, we should document it and standardize it everywhere (all connectors, all languages).

We may want to change method to (or add a new one) that's named more clearly, like CreateCollectionIfNotExists or
GetCollection(bool createIfNotExists)

[shawncal]

Member

My 2c: these functions are not really meant for the C# developer; they're for the AI to call. Throwing exceptions will just inevitably break AI-driven scenarios, so we should make SKFunctions as resilient as possible. When in doubt, defer to the AI's likely intent of calling the function, and make it do the logical thing, not relying on exceptions as control logic. In this case, the intent is clearly to create the collection -- throwing because it already exists it a technicality that makes sense from a strictly-code Collections library, but cannot possibly be productive for the AI that just wants to make something happen.

I agree with the removal of the exception, and I would also like to see the Memory Skill docs updated describe this expected behavior, assuming we all agree. Then we can propagate that behavior and documentation to all the connectors and languages.