Docs/document hybrid params - #358
Conversation
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Secrets | View in Orca |
|
To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge. |
|
agree |
|
Hey @g-despot , could I get a review on this? Happy to make changes if required. |
Document every parameter on the 30 public methods that previously had only a <summary>, including boost, so IntelliSense matches the rest of the query/generate API. Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
Add ci/check_xml_param_docs.py to fail when a public QueryClient, GenerateClient, or typed query/generate method has a <summary> but missing or incomplete <param> tags. Wire it into preflight so Hybrid-style gaps cannot regress silently. Signed-off-by: shaurya2k06 <shaurya2k06@gmail.com>
PR weaviate#358 was written against weaviate#355 (feat/boost-query-api), so its <param> tags describe that branch's API rather than main's: - `boost` does not exist on main, so 30 `<param name="boost">` tags document a parameter that is not there (CS1572). They belong with weaviate#355, which is the PR that introduces the parameter. - `diversitySelection` was added to the same 30 methods by weaviate#366 after this PR was written, and was left undocumented (CS1573). Drop the boost tags and document diversitySelection instead, placing each tag at the position its parameter occupies in the signature (after `bm25Operator` in the Hybrid overloads, `distance` in GenerateClient.NearVector, `offset` in the QueryClient.NearText extensions). Also align two style details the PR introduced: `<param name="client">` reads "The client" (26 instances repo-wide, none of "The query client"), and the two QueryClient.NearText extension blocks take the trailing period their prose-style neighbours all use. The worklist was derived from the compiler, not by hand: `dotnet build --no-incremental` now reports 0 CS1572 and 0 CS1573, and the normalized warning set is byte-identical to origin/main (55 unique warnings on both).
… every PR The check added in dab0af0 was weaker than the compiler and wired into preflight unconditionally. Four defects, each now covered by a self-test in ci/test_check_xml_param_docs.py: - Generic methods were invisible. `(?P<name>\w+)\s*\(` cannot match `Hybrid<T>(`, so the gate silently skipped 4 of the very methods this PR documents (the generic extension overloads in Typed/TypedQueryClient.Hybrid and Typed/TypedGenerateClient.Hybrid). - Unsatisfiable false positive. Splitting the parameter list on every comma turned `Dictionary<string, object> filters` into a phantom parameter named `Dictionary<string`, which no <param> tag can ever satisfy. Latent today; it would have wedged CI permanently the first time such a parameter appeared. - Silent success on an empty scan. Zero scanned files printed OK and exited 0, so any path drift would have disabled the gate forever. The scan count is now reported and an empty scan (or a missing client root) fails loudly. - Nested parens defeated the `[^)]*` signature capture, dropping methods with `= default(CancellationToken)`, a tuple return type, or an attribute holding parens in a string. Declarations are now located with a literal-aware brace/paren scanner instead of one regex, since a regex cannot balance those constructs. Scanned declarations rise from 118 to 126 (+4 generic overloads, +4 public constructors) with none dropped; the checker stays restricted to the public surface, as its docstring always claimed. The preflight step is also guarded on `changed-files.any_changed` like the neighbouring formatting check, and moved ahead of Setup .NET / restore since it needs no .NET. preflight is a `needs:` of the 8-version test matrix, so before this an unrelated PR touching no C# would have been blocked by it.
…clean it Three follow-ups on the review of the previous two commits. Use the prose wording in the two prose-style blocks. The `diversitySelection` wording on main is block-sensitive, not global: terse blocks say "The diversity selection" (28 instances) and prose blocks say "Diversity selection to apply to the results." (50 instances), and every one of the 77 pre-existing instances matches its block. The two tags added to the QueryClient.NearText extension overloads sat in prose blocks, so they now take the prose wording; the other 28 this PR adds are in terse blocks and are unchanged. The convention now holds across all 107 instances with no exceptions. Run the self-test in CI. A test nobody runs is not a test, so the guarded preflight step now runs ci/test_check_xml_param_docs.py before the checker itself. Both are plain commands on separate lines, so the runner's `bash -e` propagates a failure: with a deliberately reverted generics fix the step exits 1 and never reaches the checker. Make both files type-clean. Pyright reported 13 errors in the new test, all stemming from monkey-patching module globals and from using the `ModuleSpec | None` returned by spec_from_file_location unguarded. Rather than suppressing them, `check_file` and `main` now take their roots as parameters defaulting to the module constants, so the test injects roots instead of patching them, and the spec and its loader are asserted non-None. Both files report 0 errors, and the checker's standalone behaviour is unchanged.
3f18bf9 to
da85f56
Compare
… place Two lint leftovers from the roots-injection refactor. `sys` became unused once the tests stopped writing to stderr directly, so remove it. The `_scanned` return from check_file was discarded; rather than dropping it, assert on it — the generic fixture holds exactly one declaration, so it now pins that the generic overload is counted rather than merely parsed. 18 self-tests pass; ruff and pyright both report clean.
The param text restated the parameter name — "The alpha", "The bm 25 operator" — so it carried less than the signature already showed. Each parameter now says what it does and what happens when it is left unset, which for a nullable parameter is usually the half that matters. alpha is corrected rather than matched to the Python client, which calls it the weight of the BM25 score. Core weights the dense result set by alpha and the sparse one by 1 - alpha, so 0.0 is pure keyword and 1.0 is pure vector, and the server default is 0.75. The client also sets alpha to 1.0 itself when no query text is given, which the text now mentions. provider is corrected too: passing one when the prompt already carries a provider throws rather than being ignored. The same wording is still wrong in the files this change does not touch.
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
| Infrastructure as Code | View in Orca | ||
| SAST | View in Orca | ||
| Secrets | View in Orca | ||
| Vulnerabilities | View in Orca |
main added a boost parameter to the query and generate methods (weaviate#355), which lands in the same XML doc blocks this branch rewrote. Two files conflicted, QueryClient.NearText and GenerateClient.NearVector, and both are resolved by keeping both sides: main's boost parameter and the branch's parameter descriptions. Where main documented boost, its text is replaced by the description this branch uses for the parameter, so the surface reads with one voice.
The XML param gate this branch adds requires a <param> tag for every parameter of a documented public method. main documents boost on some of the methods that took it in weaviate#355 and not on others, so the six Hybrid, NearText and NearVector partials were left with 30 undocumented boost parameters — the branch's own check would have failed on main's code. Every boost parameter now carries the same description, taken from the summary on Boost itself: it re-scores the candidate pool the search fetches rather than excluding anything, nothing is boosted when it is left unset, and a server older than 1.38 ignores it silently. That last point is the one a caller cannot otherwise see, since the query still succeeds and only the ranking is wrong. 106 tags across 24 files, replacing the 68 shorter ones main carried.
Boost is generally available. weaviate/docs commit da77b9e8 ("Promote the
Boost API note from preview to generally available") changed the shared
feature note from ":::caution Preview - added in v1.38" to ":::info Added
in v1.39", so every "Preview feature" claim in the C# boost docs is now
false.
Three places carried it:
- the <param name="boost"> tag, 106 occurrences across 24 files
- the <summary> on the Boost record itself
- TestBoost.cs: the file summary and five RequireVersion skip messages
The version floor deliberately stays 1.38, not 1.39. The docs note says
"Added in v1.39" because that is when boost went GA, but the wire field
and this client's support for it are genuinely 1.38+: the integration
tests gate on >= 1.38.0 and the full boost surface was verified against a
live 1.38.4 server. Claiming 1.39 would understate what the client
supports. The silent-ignore warning is kept too - it is the fact a user
most needs, and GA status does not affect it.
The RequireVersion("1.38.0", ...) gates are unchanged; only the stale
"(Preview)" parenthetical is dropped from the skip messages, which still
explain why a test skipped on an older server.
Documentation only; no code or behaviour changes.
|
Sorry for multiple commits and changes, this PR got caught by our Boost work merging into main. Closing it was accidental, deleting the base branch auto-closed it, and I've retargeted it to main and reopened it. I've also pushed three commits: the Thanks a lot for the contribution 😄 |
|
Had a review, all good! Learnt a lot while contributing. I'd be happy to assist you with more features in the future 🚀 |
Summary
<param>documentation to the 30 public query/generate methods that previously had only a<summary>(all Hybrid overloads on Query/Generate + typed clients, 4 GenerateNearVectoroverloads, and 2 NearText extension overloads).QueryClient.NearVector/ BM25 (The limit,The alpha, …); copy theboostline verbatim from the rest of the public API. Rich boost prose stays onModels/Boost.cs.ci/check_xml_param_docs.pyand wire it into preflight so methods with a<summary>but missing/incomplete<param>tags fail CI on the Query/Generate (+ typed) surface — closes the CS1573/CS1591 blind spot that let this gap persist.feat/boost-query-apiso these methods are fully documented includingboostonce that lands. No behavior or public API surface changes.Test plan
dotnet build src/Weaviate.Client/→ 0 errorsdotnet test --filter "FullyQualifiedName~Unit"→ 877 passed / 0 failed / 2 skippedpython3 ci/check_xml_param_docs.py→ passesWeaviate.Client.xmlincludesboost,alpha,filters, etc. on Hybrid / Generate Hybrid / NearText extension overloadsFixes #357