-
Notifications
You must be signed in to change notification settings - Fork 2
feat:Refactor REST DTOs and enhance vectorizer configuration #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Secrets | View in Orca |
|
|
||
| public static readonly Vectorizer None = new("none", new { }); | ||
| // TODO Maybe have a global config value for default named vector | ||
| public static NamedVectorConfig None(string name = "default") => New(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add a default for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs the VectorIndexConfig field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add a default for now
I thought this was the intention all along -- having the client name the only vector a "default" one.
src/Weaviate.Client/Extensions.cs
Outdated
| else | ||
| { | ||
| // TODO Throw Exception for unexpected scenario? | ||
| vc.Vectorizer = new NoneConfig(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: @dirkkul once mentioned that we want users to select a vectorizer explicitly, perhaps we should throw something here then?
Or make this requirement more apparent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we just don't send anything if the user doesn't supply a named vector. But I am open to making it a required argument
|
I notice there's a lot of generic serialization code (type switches and alike) -- is that done to avoid relying on a 3rd-party lib? |
00cec65 to
e434eeb
Compare
Yes and no. Depends on where you're looking. If you look into the REST code, that could be auto-generated, and have a lot of serialization handling. The .NET framework has many serialization options, but I think that before I introduce a 3rd party library I need to fully/better understand all of the intricacies of what Weaviate expects. There's also a LOT of 3rd party libs that do a lot of magic, which I'd rather look into once I know 100% what I need to avoid surprises along the way. My preference in this case is to use a 3rd party lib to eventually get rid of my own code. |
…ectorization configurations - Introduced VectorConfigList for managing multiple VectorConfig instances. - Implemented Vector class with a Builder pattern for creating vector configurations. - Added various vectorizer configurations including Img2VecNeural, Multi2Vec, Text2Vec, and more. - Defined properties for each vectorizer configuration to support multimedia and text vectorization. - Included implicit conversion operators for seamless integration between VectorConfig and VectorConfigList.
- Fix capitalization of properties built from Linq Expressions
e4f83db to
0327136
Compare
| moveAway: new Move(0.5f, concepts: concepts), | ||
| fields: ["value"], | ||
| metadata: new MetadataQuery(Vectors: new HashSet<string>(["default"])) | ||
| // metadata: new MetadataQuery("default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented out code?
| new VectorConfig { Vectorizer = Vectorizer.None, VectorIndexType = "hnsw" } | ||
| }, | ||
| }, | ||
| vectorConfig: Vector.Name(Vector.DefaultVectorName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this produce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as Vector.Name(Vector.DefaultVectorName).With(new VectorizerConfig.None()).With(new VectorIndexConfig.HNSW())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oki, I need to think a bit about this. Hope I'll have time to play around with it but let's go with this for now :)
Update REST DTOs to use object types instead of dynamic, fix a bug in the collection client, and introduce new vectorizer and named vector configurations for improved functionality.
resolves #23