Repository navigation
Add AI embeddings, Str.to_embeddings and pgvector queries - #347
Conversation
Embeddings.of(inputs).dimensions(n).generate(provider, model) embeds text through LangChain init_embeddings for OpenAI, Google and Bedrock, with the default provider and model from AIConfig.default_embeddings. Embeddings.fake() supports random unit vectors, a list of responses or a callable, plus prevent_stray_embeddings, assert_generated, assert_not_generated, assert_nothing_generated and fake_embedding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Stringable.to_embeddings() embeds one string through Embeddings - Blueprint.vector(col, n) adds VECTOR(n); .index() builds an HNSW cosine index - Schema.ensure_vector_extension_exists() enables pgvector - select_vector_distance, where_vector_distance_less_than, order_by_vector_distance and where_vector_similar_to on Postgres - Generic list[...]/dict[...] model fields now cast as JSON - Separate a raw ORDER BY from preceding order terms with a comma Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The vector tests need the pgvector extension, which the plain postgres image lacks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tmgbedu
left a comment
There was a problem hiding this comment.
Solid feature overall: the embeddings API, the fake and the ADR are clean, and the grammar tests pin binding order. Tests: 3854 passed with 93% coverage (--ignore=tests/masoniteorm/postgres). The full postgres suite (43, including the 4 new pgvector tests) passes against pgvector/pgvector:pg18. No comment or docstring violations, and ADR 063 is indexed.
Verdict: REQUEST CHANGES (posted as a comment because GitHub blocks request-changes from the PR author account). Blocking issues (see the inline comments):
builder.py:705— vector identifiers are quoted by hand and embedded"is not escaped.select_vector_distance('emb" <=> ?) AS x FROM pg_shadow --', v)compiles toSELECT ... ("emb" <=> ?) AS x FROM pg_shadow --" .... Column and alias names reach raw SQL unescaped. Use the grammar's own column/table wrapping, as every other builder method does, or at least double any embedded". Please add a test.Blueprint.py:818— only a bare.index()creates HNSW.table.index("embedding")with an explicit name creates a btree index on the vector column. I checked this live: withvector(1536)and a btree index, every INSERT fails withindex row size exceeds btree maximum. Pick the index type from the column's type whatever way the column is named, or add an explicitvector_index(). Please add a test.
Nits (non-blocking):
caster.py:318: a non-nullableembedding: list[float]set toNoneis stored as the JSON string'null', which pgvector rejects (Vector contents must start with "["). Consider passingNonethrough inJsonCast.set, or documenting that callers should uselist[float] | None.Ai.reset_fakes()does not clearEmbeddings._faked, so a forgottenreset_fake()leaks the fake into later tests. Consider havingreset_fakesclear it too.- Vector column refs are not table-prefixed (
"embedding"), while other where clauses are ("documents"."a"). This could be ambiguous in joins unless the caller qualifies the column.
| def _vector_column(self, column: str) -> str: | ||
| if not self.grammar.supports_vectors: | ||
| raise ValueError(f"{self.grammar.__name__} does not support vector queries; use PostgreSQL with pgvector.") | ||
| return ".".join(f'"{part}"' for part in column.split(".")) |
There was a problem hiding this comment.
Embedded " is not escaped, so a column or as_ such as 'emb" <=> ?) AS x FROM pg_shadow --' breaks out of the identifier. Please use the grammar's wrapping helpers, like the other builder methods, or escape " as "", and add a test.
There was a problem hiding this comment.
Fixed in 7ee1747. Column and alias names now go through PostgresGrammar.wrap_identifier, which doubles any embedded ". Your payload compiles to ("documents"."emb"" <=> ?) AS x FROM pg_shadow --" <=> ?), so it stays one quoted identifier. The regression test is test_vector_identifiers_escape_embedded_quotes and covers both the column and as_. The same commit also adds the table prefix from the nit: unqualified columns become "documents"."embedding", or "d"."embedding" when the query has an alias.
| """ | ||
| index_type = "index" | ||
| if not column: | ||
| index_type = "vector" if self._column().column_type == "vector" else "index" |
There was a problem hiding this comment.
This only picks HNSW when index() is called with no column. table.index("embedding") falls through to a btree index. I checked on pgvector pg18: a btree index on vector(1536) makes every INSERT fail with index row size exceeds btree maximum. Please choose the index type from the column type whichever way the column is passed, and add a test.
There was a problem hiding this comment.
Fixed in 68282d4. index() now picks the index type from the column type. If any indexed column was added as vector in the same blueprint, it creates an HNSW vector_cosine_ops index, whether the column is named or taken from the last column. For an existing vector column that the blueprint cannot see (an alter that only adds an index), there is now an explicit table.vector_index(column=None, name=None), which raises on non-Postgres platforms. Tests: test_named_index_on_a_vector_column_creates_an_hnsw_index, test_named_index_on_a_regular_column_stays_a_btree_index, test_vector_index_on_an_existing_column and test_vector_indexes_require_postgres. The old alter test that pinned the btree output now expects HNSW.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Ai.reset_fakes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. All items are addressed in 7ee1747, 68282d4 and aa13437:
Tests: 3865 passed, 93% coverage ( |
tmgbedu
left a comment
There was a problem hiding this comment.
Re-review of 7ee1747, 68282d4 and aa13437 at aa13437d.
Verdict: APPROVE. Posted as a comment because the author account cannot approve its own PR. Both blocking items are fixed and tested. The notes below don't block the merge.
Tests
uv run pytest --ignore=tests/masoniteorm/postgres --cov: 3865 passed, 10 skipped, coverage 93.00%.tests/masoniteorm/postgresagainst a throwawaypgvector/pgvector:pg18: 43 passed.- I also ran live probes against that database (listed below).
Items from the earlier review
- Identifier escaping: fixed.
wrap_identifierdoubles". Live, the payloademb" <=> ?) AS x FROM pg_shadow --stays inside one quoted identifier, as the column and asas_, and nothing runs from it. The regression test istest_vector_identifiers_escape_embedded_quotes. - HNSW for named
.index(): fixed. Live,table.index("embedding", name=...)onvector(1536)createsUSING hnsw (embedding vector_cosine_ops), and a 1536-dim insert succeeds.table.vector_index("embedding")in an alter also creates HNSW, and it raises on SQLite, MySQL and MSSQL. All of this is tested. JsonCast.set(None)returns NULL: safe, but it changes behaviour. Nullable fields already skipped the cast, so only non-nullabledict/listfields are affected. A nullable column now stores SQL NULL instead of JSONnull, sowhere_nullmatches and reads are unchanged, becausegetalready maps'null'toNone. ANOT NULLJSON column givenNonenow raises an IntegrityError where it used to store'null'without complaint. This undoes whattest_bare_annotations_keep_their_none_behaviourpinned in dc7324c, and ADR 063 documents it. Please call it out in the release notes.Ai.reset_fakes()clears the embeddings fake: fixed. The test istest_ai_reset_fakes_clears_the_embeddings_fake.- Table prefix: fixed. Checked live:
- alias:
"d"."embedding"; - schema-qualified table:
"public"."documents"."embedding"; - joins: an unqualified column takes the base table, a qualified one like
chunks.embeddingis kept; - alias + self-join runs.
- alias:
ADR 063 and its index entry are updated. No new comments or docstrings were added.
Non-blocking notes
Blueprint.py:817:table.index(["embedding", "title"])becomesUSING hnsw (embedding,title vector_cosine_ops). pgvector rejects this withaccess method "hnsw" does not support multicolumn indexes. Btree was already broken for this case, so it is not a regression. Consider raising a clearValueErrorwhen a vector column is in a multi-column index.builder.py:206:select_vector_distanceon a schema-qualified table with no explicitselect()crashes withValueError: too many values to unpack. The cause is the existingadd_select_raw→public.documents.*→_table_column_stringpath, so it is not new code, but vector queries now reach it. With an explicit.select(...)it works. Worth a follow-up.- This one was already there and fails safely: the
?→$nconverter also rewrites a?inside a quoted identifier. The injection payload therefore ends in a syntax error and never executes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Adds text embeddings to the
aipackage, following Laravel's AI SDK Embeddings and Testing Embeddings. Design in ADR 063.init_embeddingsandaembed_documents. Supported providers: OpenAI, Google (google_genai) and Bedrock.dimensions()maps todimensions,output_dimensionalityormodel_kwargs={"dimensions": n}. Anthropic and ElevenLabs raiseValueError.AIConfig.default_embeddings(AI_DEFAULT_EMBEDDINGS_PROVIDER, defaultopenai) andModelType.EMBEDDINGS. Default models:text-embedding-3-small,gemini-embedding-001,amazon.titan-embed-text-v2:0.of()replaces Laravel'sfor, which is a Python keyword.Image.ofandAudio.ofuse the same name.Testing API
Str.to_embeddingsand pgvector<=>. Other platforms and grammars raiseValueError. Vectors are bound as JSON arrays, the same way Laravel does it.TypeErrorbecause the query builder is sync. Embed it first withawait Str.of(q).to_embeddings().list[...]/dict[...]now getJsonCast. Before, they fell back to the string cast, so vectors came back as strings.order_by(...).order_by_raw(...)was missing the comma between terms.Not included (follow-ups)
->cache()).Test plan
tests/ai/test_embeddings.py: provider options and dimensions per provider, config defaults and overrides, unsupported provider, all three fake modes, prevent stray, and assertions.embeddings.pyhas 100% coverage.init_embeddingsoptions against the reallangchain-openai,langchain-google-genaiandlangchain-awsclasses on Python 3.13.to_embeddingsunit tests.tests/masoniteorm/postgres/test_postgres_vector.pypasses against a livepgvector/pgvector:pg18database, along with the rest of the Postgres suite (43 passed). CI's Postgres service now usespgvector/pgvector:pg18(waspostgres:17) so the extension is available.uv run pytest --ignore=tests/masoniteorm/postgres --cov: 3854 passed, coverage 92.99%.ruffandbasedpyrightare clean.🤖 Generated with Claude Code