Repository navigation
Conversation
The offline pipeline embedded `type: <Type>\n<field>: <value>` while a text
`nearest($v, $q)` embeds `$q` verbatim, so a record whose source equaled the
query text did not rank at distance zero: unrelated vectors under the mock
provider, a systematic skew under a real model.
`@embed("source")` declares `vector = embed(source)` with one String source
(RFC 0015), and a query string carries no type or field to template, so the
pipeline is the side that changes: it now embeds the source value unchanged,
which is what the query path already does. The spec's `fields` must name
exactly that one property; a record with an absent, null or blank source gets
no vector instead of one embedded from its type name, and a re-embed removes
such a record's stale vector; a non-string source is refused.
The no-source path of `omnigraph embed` kept the target whenever `embedding_missing` held, which is also true for an empty list: a record with a blank source and `embedding: []` came out unchanged, with `cleaned_records: 0`, and its load then failed with "expects 4 dimensions, got 0". Selecting which records need embedding is not the same contract as deciding what a record without source text may keep. The no-source path now removes whatever the target holds (a stale vector, a null, an empty placeholder) and reports `Removed` exactly when it removed something, which `--clean` already counts the same way. The existing `a_record_without_source_text_keeps_no_vector` test covers all three target values; before this change it failed on the `[]` case with `Unchanged` versus `Removed`.
…d` declarations
`omnigraph embed` took a hand-written spec (`{"dimension": N, "types": {"T":
{"target": …, "fields": […]}}}`, or a seed manifest's `embeddings:` entry)
that restated what `embedding: Vector(N) @embed("title", model="…")` already
declares, and nothing compared the two. A spec naming another source or
dimension silently wrote vectors a text `nearest()` could not match, unknown
spec keys were ignored, and a recorded `@embed` model was never checked
against the provider, although the query path refuses that mismatch.
The command now takes `--schema <schema.pg>` (a seed manifest names it with
`schema:`) and fills every `@embed` vector the catalog declares, with that
declaration's source and dimension. A type may carry several vectors of
different dimensions, which the spec could not express. `--spec` and the
manifest's `embeddings` entry are removed, and the manifest refuses keys it
does not read.
Before reading input or creating output, a run refuses a recorded model the
provider does not resolve, through `EmbeddingConfig::embeds_with_model`,
which `resolve_nearest_query_vec` now uses too. It also refuses a `--type`
without an `@embed` vector, a schema without one, and, by line, a selected
record without source text whose vector is a required `Vector(N)`. JSON
output reports `vectors` and `schema` in place of `dimension`.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: request changes before merge for the one P2 finding below. This is a COMMENT review because the authenticated account is the PR author.
Reviewed 3f437a6670a60c176968138134959681546b0a50 against its parent 3721e4640c0944c0b45e3cd830b851b8daa103a6 in a clean isolated checkout. This covers the final schema-authority commit. The prerequisite #929 is now merged.
The old file pipeline repeated the schema in an embedding spec. The two could disagree about the source, dimension, or model. This change reads targets from the compiler catalog, checks recorded models, and generates each declared vector. Users must replace --spec and the manifest's embeddings section with a schema path. It remains a file preparation command. Loading the result remains a separate graph operation.
Required change
The model check couples every vector on a node to one provider, even though the schema permits different models per property. Both model choices then refuse, and --type cannot select between properties. The inline comment gives the reproduction and the required regression. Preserve schema authority while allowing a run to select the declared vector it will generate. Do not restore a second source/dimension/model mapping or silently ignore a mismatched target.
Correctness, tradeoffs, and liability
The central fix is sound: remove duplicate declarations at the CLI boundary. The catalog-derived plan covers each node's declarations and dimensions. It uses the same model comparison as text nearest. Strict manifest decoding closes the ignored-key class. Required-vector refusal agrees with the loader.
This removes a configuration format and its drift. It adds an in-memory target plan, a small public model-comparison method, and a changed JSON summary. It adds no durable state, queue, or publication rule. Those costs are justified. Five future vector options can derive from the catalog instead of adding five parallel spec fields. The unresolved target-selection gap would otherwise force users to maintain reduced schema copies, which recreates the drift this change removes.
The intended workload is offline seed preparation. The loop streams records but still makes serial provider calls for each vector it generates. More declared targets can mean more calls. I did not measure throughput. The caller must supply the intended schema file. Existing vectors remain trusted in fill-missing mode; this is not a provenance audit or automatic re-embedding on source changes.
I surveyed the other 29 open PRs. The relevant connections are #793's vector-space contracts, #884's compiler-owned type expansion, #951's proposed views, and the skill edits in #811/#892. Keeping this work in the catalog and offline CLI fits those boundaries. Merge the overlapping skill edits carefully. The planner, merge/cache, deployment/admission, GQT corpus, and release/documentation work does not require another embedding authority. RFC 0015's future graph reconciler can reuse the same declarations and provider rule without moving network calls into ordinary writes.
I consulted the repository guides and the complete relevant Lance type, array, vector-search, and index documentation. I checked stock Lance 11.0.0 source (ab6b5bbe46009ed78746b444df8db59a8bc5d842). OmniGraph maps vectors to FixedSizeList<Float32>; Lance's scanner checks query dimensions. Neither check establishes an embedding model's identity. That remains OmniGraph's responsibility. The current Lance 14 beta does not justify a substrate dependency for this CLI fix. No storage or deployment behavior changes here.
Local validation and before/after proof
All Cargo commands used RUSTUP_TOOLCHAIN=1.97.1, CARGO_TARGET_DIR=/tmp/review-928/target, and RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'.
| Command | Parent | Reviewed head |
|---|---|---|
cargo test -p omnigraph-cli --locked --bin omnigraph embed |
9 passed | 14 passed |
cargo test -p omnigraph-cli --locked --test integration cli_data::embed_ |
4 passed | 5 passed |
cargo test -p omnigraph-engine --locked --test search nearest_string_ |
Not run | 4 passed |
cargo test -p omnigraph-cli --locked --test integration cli_data::embed_review_ |
4 behavioral failures | 4 passed |
cargo test -p omnigraph-cli --locked --bin omnigraph review_seed_source_refuses_an_unread_model |
1 behavioral failure | 1 passed |
The last two rows used temporary tests in the existing CLI owners. The four CLI cases were embed_review_schema_owns_source, embed_review_schema_owns_dimension, embed_review_recorded_model_mismatch_refuses, and embed_review_required_vector_without_source_refuses. The parent wrote the wrong source vector, wrote eight dimensions instead of four, accepted another model, and installed a row without its required vector. The fifth test showed that the parent accepted an unread sources.model key. All five passed on the reviewed head.
The same test source and behavioral assertions ran on both commits. One compatibility branch selected the parent's --spec interface or the head's --schema interface from embed --help. An unchanged command would only prove an unknown-flag failure. No compile, setup, skipped, or zero-test result counts as this proof. GQT cannot exercise the file CLI, and its runner explicitly refuses @embed schemas, so the existing Rust owners are appropriate. All temporary source/test edits were restored. No changes were pushed.
cargo fmt --all --check, git diff HEAD^ HEAD --check, changed-file typos, python scripts/check-docs.py (253 files), and bash scripts/check-agents-md.sh passed. The macOS linker emitted a compact-unwind size warning, but tests ran.
CI and limits
The exact head has green Fix Regression Gate and PR Title checks. These are not runtime test evidence. I found no workspace/GQT build checks on this draft head. I did not run the full workspace, live provider calls, cloud tests, or performance measurements. No separate optional finding.
| fn check_model(&self, config: &EmbeddingConfig) -> Result<()> { | ||
| for target in self.types.values().flatten() { | ||
| if let Some(recorded) = &target.model | ||
| && !config.embeds_with_model(recorded) |
There was a problem hiding this comment.
[P2] Let a run select one declared vector when a type uses several models
This loop requires every vector on a node to use the same provider model. The schema permits vec_a: Vector(4)? @embed("text", model="model-a") and vec_b: Vector(4)? @embed("text", model="model-b") on one Doc. With one row containing text: "hello", embed --schema schema.pg --input raw.jsonl --output out.jsonl --type Doc refuses vec_b under model-a and refuses vec_a under model-b. I reproduced both with the mock provider on this head. Changing --type cannot help, and the check also blocks a missing vector when the other model's vector is already present. On parent 3721e464, two runs with a spec selecting each target populated both properties.
Please preserve the ability to process an individual schema-declared target. Its source, dimension, and recorded model must still come from the catalog. A target selector can do that without restoring the parallel spec or adding automatic provider dispatch. Extend the existing CLI embedding case with two differently modeled vectors on one node: run each target with its matching model, preserve the other vector, and refuse a wrong model for the selected target. The invariant belongs at the selected vector, not at the entire node type.
What & why
omnigraph embedfilled vectors from a hand-written spec ({"dimension": N, "types": {"T": {"target": "embedding", "fields": ["title"]}}}, or a seed manifest'sembeddings:entry). After #929 that spec restates exactly what the schema already declares:embedding: Vector(N) @embed("title", model="…")gives the target, the source, the dimension and the model. Nothing compared the two, so:bodyfor a schema declaring@embed("title"), or a wrong dimension, silently wrote vectors a textnearest()cannot match;embeddings.model:that did nothing);@embed(model=…)was never compared with the provider model the pipeline resolves, although the query path refuses that mismatch (resolve_nearest_query_vec).This PR makes the schema the only statement of what to embed:
--schema <schema.pg>replaces--spec. Every@embeddeclaration in the catalog (NodeType::embed_sources) is a vector the run fills, with that declaration's source property and itsVector(N)dimension. A type can now carry several vectors of different dimensions, which the spec could not express (one globaldimension, one target per type).schema:.embeddings:is gone, and the manifest now refuses keys it does not read (deny_unknown_fields, nested sections included).EmbeddingConfig::embeds_with_modelis the comparison the query path already made;resolve_nearest_query_vecnow calls it, andomnigraph embedcalls it for every selected vector with a recorded model, before it reads input or creates output.--typenaming a type with no@embedvector; a schema with none; and, by line, a selected record without source text whose vector is a requiredVector(N)(a load would refuse that record anyway;--cleanis unchanged).vectors(each processed vector'stype,property,source,dimension) andschemareplacedimension, which has no single value once a schema declares several vectors.Why refuse the old spec instead of validating it
Validating a spec against the schema keeps two sources of truth plus a comparison, and the spec format cannot even describe valid schemas.
--specis removed outright, following the precedent of--allow-data-loss(scripts get clap's unknown-argument error; the breaking note says what to do). #929 already changes the spec format in a breaking way for v0.13.1, so users migrate once if both land in that release; for that reason this PR also drops the "fieldsmust list one property" sentence from #929's breaking note, which would otherwise contradict this one.Why a schema file, not the graph's accepted schema
embedis alocal-plane command (RFC 0010/0011): it reads and writes files and takes no graph address. RFC 0015 plansomnigraph embed --graph <scope>as a reconciler that writes to the graph, so a read-only graph flag now would collide with it. Seed files are often prepared beforeinit, when no graph exists.omnigraph schema showprints a graph's accepted schema for anyone without the file; the docs say so.Backing issue / RFC
omnigraph embedembed the exact@embedsource text #929's description names. I can open one if a maintainer wants it to back the PR.@embedderivation RFC 0015 declares (docs/rfcs/0015-ingest-embeddings.md, "Embeddings are derived state, keyed off@embed", and its "model mismatch refuses loudly, reusing the query-path same-space validation"), though RFC 0015 is a draft.Checklist
docs/user/search/embeddings.md(offline pipeline, seed manifests, the model and required-vector refusals),docs/dev/execution.md,skills/omnigraph/(SKILL.md, references/search.md, references/commands.md); changelogembed-schema-authority.breaking.mdand.fixed.mdLocal verification
cargo test -p omnigraph-cli --locked --bin omnigraph embed: 9 passed;cargo test -p omnigraph-cli --locked --test integration -- cli_data::embed_ cli_data::optimize_json_succeeds_on_local_graph: 5 passed.cargo test -p omnigraph-cli --locked: unit 147 passed (6 ignored), integration 295 passed (8 ignored). The first run failed 57 server-backed tests because a-p omnigraph-clirun does not buildomnigraph-server; aftercargo build -p omnigraph-server --locked, those 57 passed with--exact.cargo test -p omnigraph-engine --locked --test search nearest_string_: 4 passed (the query path's model checks through the shared rule).check_modeldisabled,a_run_embeds_each_declared_source_at_its_declared_dimensionfails; with the required-vector refusal disabled,a_required_vector_without_source_text_is_refusedfails. Both pass restored.cargo clippy -p omnigraph-cli -p omnigraph-engine --all-targets --locked -- -D warnings -W clippy::dbg_macro: clean.cargo fmt --all --check: clean.python3 scripts/check-docs.py,bash scripts/check-agents-md.sh,typoson the changed files,python3 scripts/release_notes.py preview --working-tree: clean; the notes render.@embedschemas, so no case reaches this code; CI runs both.Notes for reviewers
@embedschemas because they resolve a provider from the process environment, so the regression tests are in-source incrates/omnigraph-cli/src/embed.rs.run_embed_jobnow takes the client as a parameter, so a whole run is tested against temp files with a mock client that resolves an explicit model: each stored vector must equalembed_query_text(source, N)at the declared dimension, and a provider resolving another model must be refused before the output file exists. One CLI test (cli_data::embed_reads_vectors_from_the_schema_and_refuses_another_model) covers the env-to-refusal wiring and that--specis no longer accepted.schema.pg, and its pre-existing vector is 4-dimensional; it was 2-dimensional under a 4-dimension spec, which a load would have refused.--typerefusal is a behavior change: before, a--typenaming a type the spec did not cover was a silent no-op.--specerror text: clap's "a similar argument exists: '--select'" tip on the removed flag is unhelpful; I left it rather than add a hidden flag only to refuse it, consistent with--allow-data-loss.parse_schema+build_catalog(admission mode), the same path aslint --schema.