feat: Add MVI data methods #506

nand4011 · 2023-10-26T21:16:39Z

Add the upsert, delete, and search MVI methods to the vector client.

Add an internal MVI data client and grpc manager.

Update the protos to get the latest MVI changes.

nand4011 · 2023-10-26T21:20:58Z

src/Momento.Sdk/Messages/Vector/MetadataValue.cs

This is the biggest point I'm not sure about. We need a clean type-safe way of expressing the different types of metadata in the upsert and search methods. Doing this lets them be defined in a clean way, e.g. here, but parsing it feels a bit clunky since you need to switch on the different types like I'm doing here. I'm open to any suggestions to clean it up.

do you have examples of what user code will look like?

at a glance this seems reasonable and what I would have probably started with, but if you have example user code that would help.

The linked test code is what the user code would look like for calling search or upsert. Specifically, creating metadata is easy for the different types:

var metadata = new Dictionary<string, MetadataValue> { { "string_key", "string_value" }, { "long_key", 123 }, { "double_key", 3.14 }, { "bool_key", true }, { "list_key", new List<string> { "a", "b", "c" } }, { "empty_list_key", new List<string>() } };

For search hits that have metadata that could be any type you would need to do something like this:

foreach (var hit in successResponse.Hits) { foreach (var metadataPair in hit.Metadata) { var field = metadataPair.Key; switch (metadataPair.Value) { case StringValue stringValue: // do stuff break; case LongValue longValue: // do stuff break; case DoubleValue doubleValue: // do stuff break; case BoolValue boolValue: // do stuff break; case StringListValue stringListValue: // do stuff break; } } }

nand4011 · 2023-10-26T21:22:02Z

src/Momento.Sdk/Requests/Vector/MetadataFields.cs

This uses a similar trick to MetadataValue to allow a method to take either a List, or an ALL sentinel value. The downside is it can't take an IEnumerable. I'm also open to suggestions here.

same comment here, looks reasonable to me at first glance but would love to see example code

nand4011 · 2023-10-26T21:23:26Z

src/Momento.Sdk/PreviewVectorIndexClient.cs

 {
    private readonly VectorIndexControlClient controlClient;
-
+    private readonly VectorIndexDataClient dataClient;


There is no data client round robin like in the cache client currently, nor are there eager connections or retries. I assumed we could add those in a future pr if there is demand.

cprice404 · 2023-10-26T21:34:31Z

src/Momento.Sdk/Requests/Vector/MetadataFields.cs

+    /// </summary>
+    /// <param name="fields">The fields to look up.</param>
+    /// <returns></returns>
+    public static implicit operator MetadataFields(List<string> fields) => new List(fields);


neat, I didn't know you could do that.

You can't put an enumerable as the argument though?

Nope, it has to be a specific implementation. The best we can do is add conversions for all implementations that make sense.

i see.

i actually think list is probably fine, especially for MVP. was just curious because I hadn't seen this feature before.

cprice404

i didn't review super closely, and definitely want michael to weigh in. but i skimmed and replied to your comments, and it looks reasonable to me from what I saw!

malandis

Since .NET has namespacing, I'd prefer the request and response names to not be prefixed with Vector. Feels redundant. In python we have elided it, but not in typescript since we don't have a nice option for namespacing.

@cprice404 thoughts?

src/Momento.Sdk/IPreviewVectorIndexClient.cs

malandis · 2023-10-27T17:34:16Z

src/Momento.Sdk/Internal/VectorIndexDataClient.cs

+            var metadataRequest = metadataFields switch
+            {
+                MetadataFields.AllFields => new _MetadataRequest { All = new _MetadataRequest.Types.All() },
+                MetadataFields.List list => new _MetadataRequest
+                {
+                    Some = new _MetadataRequest.Types.Some { Fields = { list.Fields } }
+                },
+                _ => throw new InvalidArgumentException($"Unknown metadata fields type {metadataFields.GetType()}")
+            };


I didn't know it C# had switch expressions. That's cool!

src/Momento.Sdk/Internal/VectorIndexDataClient.cs

malandis · 2023-10-27T17:50:00Z

src/Momento.Sdk/Requests/Vector/VectorIndexItem.cs

+/// <summary>
+/// A item in a vector index. Contains an ID, the vector, and any associated metadata.
+/// </summary>
+public class VectorIndexItem


Could use a record class here

I don't think we can since we the metadata dictionary would be compared by reference. I tried making MetadataValue a record for a while but I couldn't overwrite the equals method in ListStringValue to make it compare correctly. Looks like I overlooked adding an equals and hashcode method here. I can add that in.

src/Momento.Sdk/Responses/Vector/SearchHit.cs

cprice404 · 2023-10-27T18:23:54Z

Since .NET has namespacing, I'd prefer the request and response names to not be prefixed with Vector. Feels redundant. In python we have elided it, but not in typescript since we don't have a nice option for namespacing.

@cprice404 thoughts?

In a greenfield I definitely agree.

The inconsistency bugs me, so if we do it, I think we should be planning to make a consistent version of the existing ones. Which I think can be done, by basically renaming the CacheFoo response to Cache.Foo, and then making a type alias (possibly deprecated) using the old name so we don't break backward compat.

We had the same convo about Python and I think we decided to do this, but I don't think we've actually come back and updated the Cache/Topic responses yet.

I'm good with making this change for vectors now as long as we at least capture a ticket for updating the Cache/Topic ones and put it on the board.

malandis

Looks good; a few minor comments

src/Momento.Sdk/IPreviewVectorIndexClient.cs

malandis · 2023-10-31T20:22:21Z

tests/Integration/Momento.Sdk.Tests/VectorIndexDataTest.cs

+        var indexName = $"index-{Utils.NewGuidString()}";
+
+        var createResponse = await vectorIndexClient.CreateIndexAsync(indexName, 2, SimilarityMetric.InnerProduct);
+        Assert.True(createResponse is CreateIndexResponse.Success, $"Unexpected response: {createResponse}");
+
+        try


This repeated code and cleanup can be refactored to a disposable class, then you can use a using block; see https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/using

malandis · 2023-11-01T21:22:44Z

LGTM. I will 👍 once the conflicts are resolved.

Add the upsert, delete, and search MVI methods to the vector client. Add an internal MVI data client and grpc manager. Update the protos to get the latest MVI changes.

malandis

😎

nand4011 commented Oct 26, 2023

View reviewed changes

nand4011 requested review from malandis and a team October 26, 2023 21:23

cprice404 reviewed Oct 26, 2023

View reviewed changes

malandis reviewed Oct 27, 2023

View reviewed changes

Base automatically changed from initial-mvi to main October 30, 2023 22:10

nand4011 force-pushed the mvi-data-methods branch from 5aa0bad to 078b735 Compare October 30, 2023 22:33

nand4011 requested a review from malandis October 30, 2023 23:03

malandis reviewed Oct 31, 2023

View reviewed changes

nand4011 added 4 commits November 2, 2023 14:36

feat: Add MVI data methods

5ee4ed4

Add the upsert, delete, and search MVI methods to the vector client. Add an internal MVI data client and grpc manager. Update the protos to get the latest MVI changes.

Address PR comments

0512bf5

Remove 'vector' from request/response classes

8976618

Fix doc errors

0dd4fca

nand4011 force-pushed the mvi-data-methods branch from 705021c to 0dd4fca Compare November 2, 2023 21:55

nand4011 requested a review from malandis November 2, 2023 21:55

malandis approved these changes Nov 3, 2023

View reviewed changes

nand4011 merged commit 5ca7411 into main Nov 3, 2023
7 checks passed

nand4011 deleted the mvi-data-methods branch November 3, 2023 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add MVI data methods #506

feat: Add MVI data methods #506

nand4011 commented Oct 26, 2023

nand4011 Oct 26, 2023

cprice404 Oct 26, 2023

cprice404 Oct 26, 2023

nand4011 Oct 26, 2023 •

edited

Loading

nand4011 Oct 26, 2023

cprice404 Oct 26, 2023

nand4011 Oct 26, 2023

cprice404 Oct 26, 2023

nand4011 Oct 26, 2023 •

edited

Loading

cprice404 Oct 26, 2023

cprice404 left a comment

malandis left a comment

malandis Oct 27, 2023

malandis Oct 27, 2023

nand4011 Oct 27, 2023

cprice404 commented Oct 27, 2023

malandis left a comment

malandis Oct 31, 2023

malandis commented Nov 1, 2023

malandis left a comment

feat: Add MVI data methods #506

feat: Add MVI data methods #506

Conversation

nand4011 commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nand4011 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nand4011 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cprice404 left a comment

Choose a reason for hiding this comment

malandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cprice404 commented Oct 27, 2023

malandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malandis commented Nov 1, 2023

malandis left a comment

Choose a reason for hiding this comment

nand4011 Oct 26, 2023 •

edited

Loading

nand4011 Oct 26, 2023 •

edited

Loading