Follow-up: address remaining default vector index review fixes#433
Follow-up: address remaining default vector index review fixes#433mpartipilo wants to merge 12 commits into
Conversation
![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 |
| // Use a linux/amd64 platform pin only when the image tag carries the ".amd64" | ||
| // suffix — same narrowing the old test did, now applied to a single version. | ||
| // Use a linux/amd64 platform pin only when the image tag carries the ".amd64" suffix. | ||
| const platform = WEAVIATE_VERSION.includes('.amd64') ? 'linux/amd64' : undefined; |
There was a problem hiding this comment.
suggestion: iiuc, this is used as a parameter to startContainer. Could that function handle this piece of logic then? The benefit of it being that every caller of startContainer will not need to consider platform pin individually.
There was a problem hiding this comment.
This may become unnecessary and I'll end up deleting it
| // ───────────────────────────────────────────────────────────────────────────── | ||
| const WEAVIATE_VERSION = process.env.WEAVIATE_VERSION; | ||
| if (!WEAVIATE_VERSION) { | ||
| throw new Error('WEAVIATE_VERSION env var is required for this integration test'); | ||
| } | ||
| const expectedDefault = process.env.DEFAULT_VECTOR_INDEX ?? 'hfresh'; |
There was a problem hiding this comment.
question: could this test run against one of the containers spinned up by ./ci/run_containers.sh? My understanding is most integration tests run against those, so it'd be good to keep the harnessing consistent
There was a problem hiding this comment.
Yeah, this is a leftover from when I used testcontainers to run before/after 1.37.5. I'll make it uniform with the rest of the tests.
| */ | ||
| function assertDefaultIndexType(actual: string) { | ||
| expect(actual).toEqual(serverAppliesDefault ? expectedDefault : 'hnsw'); | ||
| expect(actual).toEqual(expectedDefaultIndexType(serverVersion)); |
There was a problem hiding this comment.
nit: do we need a helper function for a ternary operator? (expectedDefaultIndexType)
| let client: WeaviateClient; | ||
| let serverVersion: DbVersion; |
There was a problem hiding this comment.
nit: imo these are better scoped to describe(), not the module, considering their lifecycle is managed by beforeAll and afterAll hooks
| properties: undefined, | ||
| vectorIndex: undefined, |
There was a problem hiding this comment.
question: do these need to be explicitly set to undefined? Do they default to something different if left unset?
There was a problem hiding this comment.
I am following the pattern of the other tests now.
- Add expectedDefaultIndexType helper that returns 'hfresh' for >= 1.37.5 - Update first two tests to check hfresh config when server defaults to hfresh - Update named vectors test to use dynamic indexType expectation - Update addVector test to use dynamic indexType expectation - Explicitly set hnsw index for tests that reconfigure to hnsw with PQ - Use vectorIndexType:'hnsw' in legacy vectors test
Aligns the openai test container configuration with the contextionary container, ensuring Weaviate 1.37.5+ uses hfresh as the default vector index type when no vectorIndexType is specified in the client request.
2 participants
This follow-up PR contains changes that were committed after PR #432 was merged.
Included:
Context: