fix: use configured VECTOR_STORE.DIMENSIONS instead of hardcoded 1536#491
fix: use configured VECTOR_STORE.DIMENSIONS instead of hardcoded 1536#491voider1 wants to merge 1 commit intoplastic-labs:mainfrom
Conversation
The pgvector embedding dimensions were hardcoded to 1536 in both the SQLAlchemy models and the Gemini embedding client, ignoring the VECTOR_STORE.DIMENSIONS config setting. This made it impossible to use embedding models with different dimensions (e.g., 768-dim models like nomic-embed-text) without patching source code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR replaces hardcoded embedding dimensionality (1536) with a configurable Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/embedding_client.py (1)
90-94: Adddimensionsparameter to OpenAI/OpenRouter embedding calls for consistency with configured vector size.The Gemini provider respects
settings.VECTOR_STORE.DIMENSIONSviaoutput_dimensionality, but the OpenAI and OpenRouter providers don't passdimensionsto the embeddings API. IfVECTOR_STORE.DIMENSIONSis configured to a non-default value (e.g., 768), the OpenAI embeddings will still return 1536-dimensional vectors, causing a mismatch with the database schema.♻️ Suggested fix
else: # openai response = await self.client.embeddings.create( - model=self.model, input=query + model=self.model, + input=query, + dimensions=settings.VECTOR_STORE.DIMENSIONS, ) return response.data[0].embeddingApply similar changes to lines 126-130 and 266-268.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/embedding_client.py` around lines 90 - 94, The OpenAI/OpenRouter embedding calls (the branches that call self.client.embeddings.create in the embedding client) must pass the configured vector size so returned vectors match settings.VECTOR_STORE.DIMENSIONS; update those calls to include dimensions=settings.VECTOR_STORE.DIMENSIONS (similar to how Gemini uses output_dimensionality) in the OpenAI and OpenRouter branches, and apply the same change to the other two embedding.create call sites that currently omit dimensions (the other OpenAI/OpenRouter branches referenced in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models.py`:
- Line 282: The models now use Vector(settings.VECTOR_STORE.DIMENSIONS) for the
embedding columns but existing migrations hardcode Vector(1536); create a new
Alembic migration that alters documents.embedding and
message_embeddings.embedding to the configured dimension (use the same Vector
type with settings.VECTOR_STORE.DIMENSIONS) and include a safe downgrade path;
ensure the migration is idempotent (check current dimension before altering or
use SQL that works across Postgres versions), reference the Vector type change
for columns in the migration, and add a short note in the migration
message/README that deployments with VECTOR_STORE.DIMENSIONS != 1536 must run
the new migration (or manually ALTER TABLE ... TYPE ...) before inserting
embeddings.
---
Nitpick comments:
In `@src/embedding_client.py`:
- Around line 90-94: The OpenAI/OpenRouter embedding calls (the branches that
call self.client.embeddings.create in the embedding client) must pass the
configured vector size so returned vectors match
settings.VECTOR_STORE.DIMENSIONS; update those calls to include
dimensions=settings.VECTOR_STORE.DIMENSIONS (similar to how Gemini uses
output_dimensionality) in the OpenAI and OpenRouter branches, and apply the same
change to the other two embedding.create call sites that currently omit
dimensions (the other OpenAI/OpenRouter branches referenced in the review).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47328559-6761-46bd-8e06-84d888d9cb23
📒 Files selected for processing (2)
src/embedding_client.pysrc/models.py
| ) | ||
| content: Mapped[str] = mapped_column(TEXT) | ||
| embedding: MappedColumn[Any] = mapped_column(Vector(1536), nullable=True) | ||
| embedding: MappedColumn[Any] = mapped_column(Vector(settings.VECTOR_STORE.DIMENSIONS), nullable=True) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current migration hardcodes the dimension
rg -n "Vector\(1536\)" --glob '*.py' migrations/Repository: plastic-labs/honcho
Length of output: 531
🏁 Script executed:
#!/bin/bash
# Check lines 282 and 390 in src/models.py
echo "=== Checking src/models.py lines 282 and 390 ==="
sed -n '280,284p' src/models.py
echo "---"
sed -n '388,392p' src/models.pyRepository: plastic-labs/honcho
Length of output: 694
🏁 Script executed:
#!/bin/bash
# List all migration files chronologically to understand scope
echo "=== All migrations in versions directory ==="
ls -1 migrations/versions/ | sort
echo ""
echo "=== Count of Vector(1536) across all files ==="
rg "Vector\(1536\)" --glob '*.py' migrations/ -c --sort pathRepository: plastic-labs/honcho
Length of output: 1548
🏁 Script executed:
#!/bin/bash
# Check the latest migration(s) to see if they address dimension config
echo "=== Latest migration content (last 50 lines) ==="
latest=$(ls -1 migrations/versions/*.py | sort | tail -1)
echo "File: $latest"
tail -50 "$latest"Repository: plastic-labs/honcho
Length of output: 1706
🏁 Script executed:
#!/bin/bash
# Identify which models have embedding columns
echo "=== Classes containing embedding columns ==="
rg -B 5 "embedding: MappedColumn" src/models.py | grep "^class"Repository: plastic-labs/honcho
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Check the support_external_embeddings migration to see if it attempted a fix
echo "=== support_external_embeddings migration (context around Vector changes) ==="
sed -n '40,65p' migrations/versions/119a52b73c60_support_external_embeddings.pyRepository: plastic-labs/honcho
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Count total affected columns across all migrations
echo "=== Tables and columns affected by hardcoded Vector(1536) ==="
rg "Vector\(1536\)" -B 2 -A 0 --glob '*.py' migrations/ | grep -E "(op\.|sa\.Column)" | head -20Repository: plastic-labs/honcho
Length of output: 744
Database migration required for non-default vector dimensions.
The SQLAlchemy models now declare Vector(settings.VECTOR_STORE.DIMENSIONS) in documents and message_embeddings tables (src/models.py lines 282 and 390), but migrations still hardcode Vector(1536):
a1b2c3d4e5f6_initial_schema.py(documents, line 366)917195d9b5e9_add_messageembedding_table.py(message_embeddings, line 31)119a52b73c60_support_external_embeddings.py(referenced in alter operations, lines 45 and 53)
For deployments using VECTOR_STORE.DIMENSIONS != 1536:
- Existing databases: Columns remain
Vector(1536), causing insertion errors when the model attempts different dimensions. - New databases: Migrations create
Vector(1536)regardless of configuration.
The 119a52b73c60_support_external_embeddings.py migration only made these columns nullable; it did not address the dimension mismatch.
Recommended actions:
- Add a new migration to alter both
documents.embeddingandmessage_embeddings.embeddingcolumns to match the configured dimension. - Document that non-1536 deployments must manually run
ALTER TABLEstatements or apply the new migration before using custom dimensions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/models.py` at line 282, The models now use
Vector(settings.VECTOR_STORE.DIMENSIONS) for the embedding columns but existing
migrations hardcode Vector(1536); create a new Alembic migration that alters
documents.embedding and message_embeddings.embedding to the configured dimension
(use the same Vector type with settings.VECTOR_STORE.DIMENSIONS) and include a
safe downgrade path; ensure the migration is idempotent (check current dimension
before altering or use SQL that works across Postgres versions), reference the
Vector type change for columns in the migration, and add a short note in the
migration message/README that deployments with VECTOR_STORE.DIMENSIONS != 1536
must run the new migration (or manually ALTER TABLE ... TYPE ...) before
inserting embeddings.
1 participant
Summary
Vector(1536)in SQLAlchemy models (Document,MessageEmbedding) withVector(settings.VECTOR_STORE.DIMENSIONS)so the dimension reads from config"output_dimensionality": 1536in Gemini embedding calls with the same config valueContext
When self-hosting Honcho with a local embedding model (e.g. nomic-embed-text-v1.5 via llama.cpp, which outputs 768 dimensions), the
VECTOR_STORE.DIMENSIONSconfig setting is ignored becausesrc/models.pyhardcodesVector(1536). This causes pgvector to reject embeddings at query time withValueError: expected 1536 dimensions, not 768.The config setting already exists and defaults to 1536, so this is a no-op for existing deployments.
Test plan
VECTOR_STORE.DIMENSIONS = 768works without source patchingsrc.configimported insrc.models)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes