fix: five fixes from the python-parity audit (two source-breaking) - #368
Open
g-despot wants to merge 15 commits into
Open
fix: five fixes from the python-parity audit (two source-breaking)#368g-despot wants to merge 15 commits into
g-despot wants to merge 15 commits into
Conversation
Multi2VecGoogleGemini declared [Vectorizer("multi2vec-google-gemini")]. No
such module exists: modules/multi2vec-google/module.go names the module
"multi2vec-google" with the single alias "multi2vec-palm", and grepping the
server for "multi2vec-google-gemini" returns nothing. A live 1.39.0 rejects
it outright:
422 {"error":[{"message":"target vector \"default\": vectorizer: no
module with name \"multi2vec-google-gemini\" present"}]}
So the factory could not create a collection at all.
Gemini is reached through the one Google multimodal module by pointing
apiEndpoint at generativelanguage.googleapis.com — what python does with a
single _Multi2VecGoogleConfig and a switched endpoint, and what this client
already does on the text side in Text2VecGoogleGemini. Multi2VecGoogle grows
that ApiEndpoint property and VectorizerFactory.Multi2VecGoogleGemini stays
as the ergonomic entry point, now returning a Multi2VecGoogle.
ProjectId and Location drop `required` and become nullable, matching
python's Optional[str]. The Gemini API is scoped to neither, so both are
null and the REST serializer omits them. This also unbreaks readback: a
required member absent from the server's JSON makes System.Text.Json throw,
so a Gemini config could not have round-tripped even once creation worked.
Folds in the related gap: the Gemini path had neither Dimensions nor
VectorizeCollectionName, both of which Multi2VecGoogle has and python passes.
The Vertex overloads gain an apiEndpoint parameter, which the repo's own
WEAVIATE002 analyzer requires once the property exists, and which mirrors
Text2VecGoogle.
Source-breaking for anyone binding the record type, so Multi2VecGoogleGemini
survives as an [Obsolete] shim over Multi2VecGoogle following the
Multi2VecPalm/Text2VecPalm precedent. It deliberately carries no [Vectorizer]
attribute: VectorizerRegistry keys types by identifier and last write wins,
so a second type claiming "multi2vec-palm" would make deserialization of
every Google multimodal config depend on reflection order.
Tests: the two existing Gemini unit cases now pin the module name, the
endpoint and the absent Vertex fields; the string-array case carries the new
dimensions/vectorizeCollectionName and the weighted case stays without them
so omit-when-unset stays covered. New integration coverage creates both a
Gemini and a Vertex collection against a real server and asserts the round
trip; a RequireModule gate skips when the module is absent, and CI now
enables multi2vec-google so it runs there.
The generated DTO has carried Incremental_base_backup_id since the 1.37 spec (Rest/Dto/Models.g.cs) and the server reads it (entities/models/backup_create_request.go:51), but nothing in the client could set it: BackupCreateRequest had no such member and BuildBackupCreateRequest never populated one. Asking for an incremental backup was impossible; python has had it since 4.20.2 (weaviate/backup/executor.py). The version gate is on the field rather than on the operation. A [RequiresWeaviateVersion] attribute on Create would refuse every backup on a pre-1.37 server, which is a regression for plain backups, so this follows CollectionsClient.EnsureTextAnalyzerFeaturesSupported instead: check only when the caller actually supplied a base id, and throw the same WeaviateVersionMismatchException the rest of the client's gates throw. Python gates identically (executor.py:92-96). The base id is passed through verbatim rather than lowercased. Python lowers it, but this client does not lower request.Id either, and applying the rule to one id and not the other would be the surprising behaviour. Tests cover all four quadrants: the key reaches the wire when set, is absent when unset, a pre-1.37 server rejects an incremental request with the right RequiredVersion/ActualVersion, and a plain backup on that same old server still succeeds.
ToModelListItem read the list payload and then threw two fields away. Size is the outright bug: the property exists on Backup, the create-status path fills it, and Anonymous3 carries it — but the list mapper never assigned it, so every listed backup reported Size == null whatever the server said. The incremental base id had nowhere to go, so Backup gains it. Python added the same field to BackupListReturn in 4.23.0. Anonymous3 is an nswag anonymous-schema name, so it was re-checked rather than assumed: Rest/Backup.cs decodes the list response as List<Dto.Anonymous3>, and that record's shape (id/classes/status/startedAt/completedAt/size/incremental_base_backup_id) matches the list item in the spec. The create-status response carries incremental_base_backup_id too, and the Backup model is shared between both paths, so that mapper sets it as well — otherwise the field would have been silently null on the status path, which is the same defect this commit removes from the list path.
The client had zero references to generative-deepseek. Adds the collection config (GenerativeConfig.Deepseek + GenerativeConfigFactory.Deepseek + the serialization arm) and the runtime provider (Providers.Deepseek + GenerativeProviderFactory.Deepseek + the Search.Builders mapping), following the Databricks precedent throughout. Wire keys are taken from the server, not from the C# property names: modules/generative-deepseek/config/class_settings.go spells the base url "baseURL", and the camelCase policy happens to produce exactly that, so no [JsonPropertyName] is needed. That is load-bearing rather than lucky — a baseUrl/baseURL slip is silent, since the server ignores an unknown key and quietly uses its own endpoint — so the unit test pins the literal string, and was confirmed to fail when the casing is forced the other way. Includes a vendored proto sync. src/Weaviate.Client/gRPC/proto/v1/ generative.proto was one feature behind upstream: GenerativeDeepseek was missing from the GenerativeProvider oneof (field 16) along with the message itself and GenerativeDeepseekMetadata, so the runtime provider could not be expressed at all. The four hunks are copied verbatim from Weaviate v1.39.0's grpc/proto/v1/generative.proto; the vendored file now differs from upstream only by the deliberate `option csharp_namespace` line, and no other drift was found in it. The metadata message is unused today — the client does not read GenerativeMetadata anywhere — but is included so the file stays a faithful copy and the next sync is a clean diff. Note for the integration test: on a class using named vectors, Weaviate 1.39.0 rejects any fractional float in this module's config, e.g. temperature 0.7 comes back as "Wrong temperature configuration, values are between 0.0 and 2.0". The client sends ordinary JSON numbers and the same body is accepted when the class uses a class-level vectorizer, so this is a server defect: the value arrives as a json.Number, getNumberValue's non-integer path returns the caller's defaultValue, and this module passes a -100 sentinel there. generative-openai fails the same way; generative-cohere does not. The integration test therefore uses integral floats so it exercises all eight keys against a real server, and the unit test carries the fractional values.
Every scalar in the aggregate reply is `optional` in aggregate.proto. FromGrpcProperty read the Int, Number and Boolean ones unconditionally, so an unset field surfaced as the field type's zero — indistinguishable from a real aggregate of zeros. The Date branch six lines below already gated on its Has* flags; Int, Number and Boolean were simply missed. Python fixed the same defect in 4.21.2 (base_executor.py, python #2036). Proven against a live 1.39.0 before the change: an OverAll with a filter matching no objects and Metric.Integer(maximum, mean, sum) returned Maximum == 0, and the new integration test failed with "Assert.Null() Failure: Value of type 'Nullable<long>' has a value / Expected: null / Actual: 0". Both aggregate shapes route through this one method, so AggregateResult and AggregateGroupByResult are fixed together. Boolean's four members were non-nullable and had to change type, which is source-breaking; PublicAPI carries the *REMOVED*/re-add pairs. That half cannot be shown against a live server, because 1.39.0 always populates all four however few objects match and whatever metrics were requested — on an empty match it sends totals of 0 and percentages of NaN. It is still wrong to invent false/0 for a field the server did not send, so the guarantee is pinned by a unit test that drives FromGrpcProperty off a proto message directly, with a companion case proving a deliberate zero still survives as zero rather than being swallowed. Count stays unconditional, as in python: 0 is the right answer for an empty aggregate, not a missing one. The integration tests record two observed server behaviours worth knowing: unrequested Int/Number sub-metrics really are absent (so the fix is what makes them null), while the boolean members are always present.
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
Summary - Weaviate C# Client CoverageSummary
CoverageWeaviate.Client - 49.7%
Weaviate.Client.Analyzers - 0%
Weaviate.Client.VectorData - 50.3%
|
Collaborator
|
Hey,
These are not available for Multivector and we simply have not removed them from python to not create a breaking change |
…ories Corrects 55b0693, which folded a vectorizeCollectionName parameter into both Gemini overloads alongside dimensions. dimensions was right; this was not. No multi2vec module reads vectorizeClassName server-side, so the parameter did nothing. Python's multi2vec_google_gemini does not expose it, and neither does any other multi2vec_* factory — f8a6d4f1 documents it as having no effect across all of them. #367 removed it from Multi2VecTwelveLabs two PRs ago for the same reason, so keeping it here would have contradicted a decision already made on this branch's own predecessor. Confirmed against the live 1.39.0 server: creating the collection without the key stores no vectorizeClassName and the server defaults nothing back in, so the integration test now asserts null there instead of false. The property itself stays on the Multi2VecGoogle record: it is shipped API and inherited by the Gemini path, so the factory pins it to null explicitly (which the WEAVIATE002 analyzer requires) rather than dropping it. Only the two Unshipped signature lines are rewritten, with no *REMOVED* markers: the parameter never shipped, it was added earlier on this same branch. The *REMOVED* entries above them still describe the genuinely removed pre-PR signatures. The Vertex Multi2VecGoogle overloads are deliberately untouched — they carried vectorizeCollectionName in PublicAPI.Shipped.txt before this branch, so removing it there is a breaking change and a separate decision.
…izers A maintainer confirmed on #368 that vectorizeCollectionName is not available for multivector modules, and that python only keeps it to avoid a breaking change. The server bears that out: no multi2vec-* or multi2multivec-* module has a VectorizeClassName() accessor, own or via an embedded BaseClassSettings. They register "vectorizeClassName" as a class-config default in config.go and never read it back — only usecases/modulecomponents/vectorizer/object_texts.go consumes it, behind an icheck interface no multivector settings type implements. So the eight multivector records get [Obsolete] on the property, mirroring python's "Deprecated, has no effect", with the XML docs saying the same. The property stays: removing it would break callers. text2vec-* is untouched — the setting is real there, and the digitalocean test asserts vectorizeClassName:false deliberately. text2multivec-jinaai is deliberately left alone despite the name. Its class settings embed basesettings.BaseClassSettings, which supplies the VectorizeClassName() accessor that base_class_settings.go actually calls, so the setting is live for it. It is a text vectorizer that happens to emit multiple vectors, not a multivector-input module. The factory parameters could not be marked: [Obsolete] is not valid on a parameter (CS0592 — it is only valid on class, struct, enum, constructor, method, property, indexer, field, event, interface and delegate), verified with the compiler rather than assumed. Their <param> docs carry python's wording instead, which is what python does too — f8a6d4f1 is documentation only. Consumers still get a real CS0618 when they read or assign the property, including on a config read back from the server. The eighteen factory assignments the client itself must keep — WEAVIATE002 requires every public property to be initialised in a vectorizer factory — are suppressed one line at a time with scoped pragmas and a reason, not at file or project level. The one test that asserts on the property is suppressed the same way rather than deleted, since the assertion is the point.
The suppressions added in a1430ec carried a standalone comment line above each pragma, repeated verbatim sixteen times. The house form puts a terse reason as a trailing comment on the pragma itself, on both the disable and the restore — Models/Extensions.cs:116-118 is the exact analogue, an obsolete property assigned inside an object initializer. Also drops two comments that had become restatements once the property carried [Obsolete]: the Gemini factories now note only the part the attribute does not say — that they omit the parameter their Vertex siblings expose — and the unit test drops a clause that repeated the obsolete message while keeping why the key still serializes here when Multi2VecTwelveLabs drops it. The suppression reason in the integration test moves onto the pragma too, where it reads as the reason rather than as a third comment line. No behaviour change: 20 lines out, 3 in.
Boost (#355) landed on main and overlapped this branch in two files. gRPC/Search.Builders.cs auto-merged: boost added BuildBoost and its call sites, we added the deepseek runtime-provider mapping, and the two do not touch the same lines. Verified structurally rather than by eye — stripping main's side from the merged file reproduces our base->branch delta exactly, and stripping ours reproduces main's. PublicAPI.Unshipped.txt was the only real conflict. Boost rewrote ~50 overload signatures, so it removed 102 base lines and added 132; we added 106 and removed none. The two sets are disjoint: nothing we added was removed by boost, nothing boost added was removed by us. Resolved as the union — boost's file plus our 106 lines — and then handed to the analyzer rather than trusted: RS0016 192, RS0017 16, RS0025 0, all identical to what origin/main reports on its own, so our entries introduce no missing or stale declarations. PublicAPI.Shipped.txt did not move between the merge base and origin/main, so our *REMOVED* lines still name signatures that exist. Unit tests account for both sides exactly: main 946 -> 985 with boost's 39, and 985 -> 997 with our 12.
The gate comment claimed 1.37.0 was the first server to accept the field; the wire field has existed since 1.34.18. Keep the 1.37.0 gate as the documented feature floor, matching python, and say so. The read side has a different floor: servers only return the base id from 1.37.6, so on older ones a null does not mean the backup is non-incremental. Say that on the property, where deleting the base backup is the hazard. Collapse the duplicated create/list tests into theories.
The gating commit added a comment saying every scalar is checked against its Has* flag, but Count was left ungated in all five branches, so an unrequested count still read 0 - the defect the commit set out to fix. Property.Count is already long?, so gating costs no API change. Correct the Boolean member docs: when nothing matches the server does send all four, so null means it did not send the value, not that nothing matched. Drop two ConvertToNumeric overloads left unreachable by the nullable callers, and update the accessor docs, which still showed the members non-nullable.
C# cannot mark a parameter [Obsolete] (CS0592), so the sweep marked only the property - and every assignment to it sat behind a pragma inside the library. A caller passing vectorizeCollectionName: true got no diagnostic at all, which is the one audience the deprecation was for. The cost was 38 suppression lines. Python never surfaced vectorize_collection_name on its multivector factories, so there is nothing to deprecate toward; converging on that is a deliberate breaking change and does not belong in this PR. Keeps the type-level [Obsolete] on the Multi2VecGoogleGemini shim, which is real, and 55b0693's Gemini collapse. Also make the Gemini serialization tests assert absence instead of an explicit null, which only tested the test's own serializer options.
Records the additions, the fixes and the breaking changes, including the two the description had not called out: Multi2VecGoogleGemini.Model is removed with no replacement under that name, and BackupCreateRequest went from six positional parameters to seven. Corrects three entries that still advertise Multi2VecGoogleGemini as a working vectorizer; the 1.0.1 entry called it new when it could never create a collection. The original text stays, with the correction after it.
multi2vec-google has no apiEndpoint before 1.34.20 (introduced in 4862194, in no 1.32.x or 1.33.x tag), so those servers fall through to the Vertex mandatory check and reject the Gemini config for missing projectId/location. RequireModule does not cover this: the module is on every lane, only the Gemini config shape is version-dependent. Verified on real servers: without the gate 1.32.27 reproduces the CI error; with it the test skips there and still runs on 1.34.20.
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
2 participants
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.
Five fixes from the python-parity audit, one commit each.
55b0693—Multi2VecGoogleGeminiemittedmulti2vec-google-gemini, which no server has, so it could never create a collection. Now emitsmulti2vec-googlewith the Gemini endpoint (server ≥ 1.34.20). DroppingrequiredfromProjectId/Locationalso fixes a cross-client break: a collection created by python'smulti2vec_google_geminicould not be read at all.0e83cd9— sendincremental_base_backup_idon backup create. The DTO carried it; nothing set it.6c64793— backup list discardedSizeand the base id it had already parsed, soSizewas null for every listed backup.4ef693c— addgenerative-deepseek(server ≥ 1.36.19), collection config and runtime provider, with the vendored proto update it needs.4356731— aggregate reported0/0.0/falsewhere the server sent nothing. Text, Int, Number, Boolean and Date are now presence-checked,Countincluded.Breaking
Multi2VecGoogleGeminiis now an[Obsolete]shim overMulti2VecGoogle. This breaks at runtime, not only at compile time: the factory returns aMulti2VecGoogle, sois/as/switcharms on the old type silently stop matching, and aswitchhandling both no longer compiles (CS8120).Multi2VecGoogleGemini.Modelremoved — use.ModelId.Multi2VecGoogle.ProjectId/Locationno longerrequired; gettersstring!→string?.Aggregate.Boolean.PercentageTrue/PercentageFalse/TotalTrue/TotalFalseare nullable.BackupCreateRequestprimary constructor andDeconstructgo 6 → 7 parameters.All of these are also binary-breaking: recompile, or
MissingMethodException. Migration notes are in the changelog.Notes
class_settings_property_helper.gofalls through todefaultValuefor any non-integerjson.Number, and deepseek passes-100.0as that default, which then trips its own validation.generative-openaibehaves the same way. The client payload is correct; fractional values are covered by unit tests.6c64793says python has this field onBackupListReturn(it has no such field), and0e83cd9says python does not lowercase the id (it lowercases both).multi2vec-google, becauseMulti2VecGoogleregisters only the legacymulti2vec-palmand the registry has no alt-identifier support. Needs a registry change; tracked separately.Closes #352