ci: run Postgres services on pgvector images so vector-gated tests actually execute - #383
Open
dpage wants to merge 2 commits into
Open
ci: run Postgres services on pgvector images so vector-gated tests actually execute#383dpage wants to merge 2 commits into
dpage wants to merge 2 commits into
Conversation
The chat_memories fixture in the tools package still declared its embedding column as vector(3), a leftover from before the collector migration v6 widening. The memory store pads every embedding to embedding.MaxDimensions, which is 4000, on both the insert and the query paths, so Postgres rejected the padded value with "expected 3 dimensions, not 4000" and the two embedding integration tests failed on any host that actually has pgvector installed. The column now reads halfvec(4000), matching the sibling fixture in the memory package; halfvec is required rather than a wider vector because pgvector caps the vector type at 2000 dimensions, which is precisely why the original widening chose halfvec. The stale comments claiming that the mock's three-element response matches the column type have been corrected to explain the padding instead, whilst the mock itself continues to return three elements.
Several integration tests create the pgvector extension and skip themselves when it is unavailable, and every CI Postgres service used a plain postgres:<version> image with no pgvector, so those tests have never executed in CI; a skip reads as a pass in the job summary, which is how a stale halfvec fixture survived on main until it was found by running the suite on a host that does have pgvector. Switch the four Postgres service containers to the corresponding pgvector/pgvector:pg<version> tags, which are built directly on the official postgres images, so the existing POSTGRES_PASSWORD and POSTGRES_DB environment, health-check options and port mappings all carry over unchanged. The tests create the extension themselves, so no extra CREATE EXTENSION step is needed.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughCI PostgreSQL services now use pgvector images. The memory embedding database test schema stores embeddings as Changespgvector embedding support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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
1 participant
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.
Why
Several of our integration tests create the pgvector extension and skip
themselves when it turns out to be missing, with messages such as
pgvector extension unavailable: %v. Every CI Postgres service, however,used a plain
postgres:<version>image, which carries no pgvector, soevery one of those tests has silently skipped in CI since it was
written; a skip reads as a pass in the job summary line, and nobody
looks past a green tick. That is precisely how the stale
embedding vector(3)fixture in the server memory tests survived onmain, against a store that pads every vector to 4000 dimensions, untilit was found by running the suite on a dev host that does have pgvector
installed.
What changed
The four Postgres service containers now use the corresponding
pgvector/pgvector:pg<version>tags:.github/workflows/ci-server.yml.github/workflows/ci-collector.yml.github/workflows/ci-alerter.yml.github/workflows/ci-e2e.yml(fixedpg16)Nothing else about those service blocks changed, and no Postgres or Go
version was bumped. The pgvector images are built directly on the
official images (their Dockerfile is
FROM postgres:$PG_MAJOR-$DEBIAN_CODENAMEplus a build of the extension), so the entrypoint, the
POSTGRES_PASSWORD/POSTGRES_DBenvironment,pg_isreadyhealthchecks and port mappings all carry over unchanged. The tags
pg14through
pg18all exist, covering every matrix value, and no explicitCREATE EXTENSIONstep is required because each test creates theextension itself against the container's superuser role.
Tests that start executing in CI
I ran all five pgvector-gated files for real against a local Postgres 18
with pgvector 0.8.2, which is something CI has never done. Every one of
these now executes rather than skipping, and all of them pass:
server/src/internal/memory/store_db_test.goTestStore_Store,TestStore_GetByID,TestStore_Delete,TestStore_DeleteByID,TestStore_UpdatePinned,TestStore_GetPinned,TestStore_ListByUser,TestStore_Searchserver/src/internal/tools/memory_tools_embedding_db_test.goTestStoreMemoryGeneratesEmbeddingIntegration,TestRecallMemoriesGeneratesQueryEmbeddingIntegrationcollector/src/database/migration_v6_test.goTestMigrationV6_FreshSchemaUsesHalfvec,TestMigrationV6_UpgradesLegacyVectorColumns,TestMigrationV6_Idempotent,TestUpgradeEmbeddingColumn_AbsentAndUnexpectedType,TestUpgradeEmbeddingColumn_WrongHalfvecWidth,TestUpgradeEmbeddingColumn_StepFailurescollector/src/database/schema_idempotency_test.goTestMigrateFreshDatabase,TestMigrateTwiceIsIdempotent,TestMigratePartialState,TestRunPgVectorSetup_SuccessAndErrorandTestRunChatMemoryEmbeddingSetup_SuccessAndError, including theanomaly_embeddingsforeign-key check that previously skipped with"pgvector not available; FK absent by design"
alerter/src/internal/database/queries_full_integration_test.goand theneighbouring
anomaly_queries_full_integration_test.goTestStoreAnomalyEmbeddingAndFindSimilar,TestFindSimilarAnomaliesScanError, plus theFindSimilarAnomaliespaths gated behind
pgvectorAvailableTests that still skip
None of the pgvector-gated tests skip once the extension is present.
The remaining skip conditions in these files are
SKIP_DB_TESTSand anunset
TEST_AI_WORKBENCH_SERVER, and all three Go workflows alreadyexport
TEST_AI_WORKBENCH_SERVERfor the coverage step, so the gap isgenuinely closed rather than merely narrowed. A verbose run of all four
affected packages locally produced no
--- SKIPlines at all.The one secondary gate worth naming is
pgvectorHasHalfvecin thecollector migration tests, which skips when the installed pgvector
predates 0.7.0. The floating
pgXXtags currently resolve to pgvector0.8.5, so halfvec is present on every matrix entry and the gate passes;
if a future pgvector image ever regressed that, those tests would skip
again, and pinning the tags to an explicit
0.8.x-pgXXwould be theremedy.
Fixture drift
None beyond what #382 already fixes. Everything passed once the
extension was available, so this PR uncovers no further drift and
introduces no production changes; the diff is four YAML lines.
Stacking
This PR is stacked on #382 and must be merged after it, or rebased
onto
mainonce #382 lands. Without the halfvec fixture fix in #382, theserver memory tests newly enabled here would fail immediately, which is
rather the point.
Verification boundary
Locally I ran the four affected Go packages against a real pgvector
instance and validated each changed workflow with
yaml.safe_load, butCI cannot be proven from a dev host: the local runs only cover Postgres
18, and the Postgres 14 to 17 matrix entries, along with the E2E job's
now-pgvector-capable schema path, are exercised for the very first time
by this PR's own CI run. That run is the real proof, not my local
output.
🤖 Generated with Claude Code
https://claude.ai/code/session_015nPEZyXgN1zK7EuEXhfZvW
Summary by CodeRabbit