Skip to content

feat: add Client.search for vector, keyword, and hybrid search - #174

Open
lukekim wants to merge 9 commits into
trunkfrom
feat/search
Open

feat: add Client.search for vector, keyword, and hybrid search#174
lukekim wants to merge 9 commits into
trunkfrom
feat/search

Conversation

@lukekim

@lukekim lukekim commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What

Adds Client.search(), wrapping the runtime's POST /v1/search — vector similarity, keyword, and hybrid search over datasets with an embedding column.

response = client.search("tickets to Tokyo", datasets=["app_messages"], limit=3)

for match in response:
    print(match.dataset, match.score, match.matches)

text is the only positional argument; datasets, limit, where, additional_columns and keywords are keyword-only. Setting keywords pre-filters the embedding column with a lexical search before the vector search runs, making the search hybrid.

New public exports: SearchResponse and SearchMatch, with the implementation in a private _search module.

Why

/v1/search works on a default spice run and is a distinctive Spice capability, but Python users had no way to reach it short of hand-rolling the HTTP call. spice.js is currently the only SDK that exposes it.

Two wire-format details SearchMatch handles so callers don't have to:

  • the similarity score is serialized as _score, not score
  • data, primary_key and metadata are omitted entirely when empty, so they default to {} rather than None — readable without a guard

Part of aligning search support across the SDKs.

Verification

  • Unit tests pass — 16 new tests in tests/test_search.py covering body construction, wire-format decoding, and client wiring. Full unit run: 395 passed.
  • make lint — ruff clean; pylint 9.23/10 against the repo's --fail-under=8.0 gate
  • make type-check — mypy reports no issues
  • ruff format clean on the new module
  • Verified against a live spice run: the search request reaches /v1/search, and a failure
    surfaces the runtime's own message rather than a bare 400 Client Error
  • A search returning matches was not exercised end to end — that needs a dataset with an embedding
    column and a loaded embedding model, which this environment has no credentials for. The 8
    tests/test_main.py failures in a full run are refused Flight connections and are unrelated.

Note: make format-check fails on 15 files on trunk already; this branch does not add to that set.

Wraps POST /v1/search, which was previously unreachable from Python without
hand-rolling the HTTP call. spice.js is the only other SDK that exposes it.

Only `text` is required and every option is keyword-only, matching the
convention elsewhere in the client. Supplying `keywords` pre-filters the
embedding column with a lexical search before the vector search, making the
search hybrid.

SearchMatch decodes the runtime's wire format, including the `_score` field
name and the data/primary_key/metadata objects the runtime omits when empty —
those default to {} so callers can read them without a guard.
Copilot AI review requested due to automatic review settings July 27, 2026 15:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds first-class Python SDK support for the runtime’s /v1/search endpoint by introducing Client.search() plus public response/match types, aligning search capabilities with other SDKs.

Changes:

  • Introduces spicepy._search with SearchResponse, SearchMatch, and build_search_body() for request/response handling.
  • Adds Client.search() to POST /v1/search and decode results into SearchResponse.
  • Exposes new types from spicepy.__init__, adds README usage docs, and adds a dedicated unit test suite.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_search.py Adds unit tests for request body construction, wire-format decoding, and Client.search wiring.
spicepy/_search.py Implements search request builder and response/match decoding types.
spicepy/_client.py Adds Client.search() method that calls /v1/search and returns a decoded SearchResponse.
spicepy/init.py Exports SearchMatch and SearchResponse as public SDK symbols.
README.md Documents search() usage, parameters, and returned match structure.
Comments suppressed due to low confidence (1)

spicepy/_search.py:70

  • SearchResponse.from_dict currently assumes results is a list of dicts; if the runtime returns an unexpected shape (e.g. results is a dict/string), the list comprehension will throw an AttributeError from inside SearchMatch.from_dict, which is harder for callers to understand/handle. Validating results (and normalizing duration_ms) lets you raise a clear SpiceAIError consistently.
        if not isinstance(payload, dict):
            raise SpiceAIError(f"unexpected search response from the runtime: {payload!r}")

        return cls(
            results=[SearchMatch.from_dict(m) for m in payload.get("results") or []],

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread spicepy/_search.py
@lukekim lukekim self-assigned this Jul 27, 2026
@lukekim lukekim added the enhancement New feature or request label Jul 27, 2026
@lukekim lukekim added this to the v4.0.0 milestone Jul 27, 2026
…gistry

Every open PR's integration tests were failing identically, across all four
Python versions on both macOS and Ubuntu, including a dependabot bump and a
release-prep branch that touch nothing related:

    Getting Spicepod spiceai/quickstart ...
    ERROR Invalid argument: Failed to extract Spicepod archive:
          invalid Zip archive: Could not find EOCD

The cause is outside this repo. api.spicerack.org serves the quickstart
Spicepod with HTTP 200 and Content-Type: application/zip, but the body is a
152-byte JSON error:

    {"Message":"invalid path \"spiceaiquickstartv0.1.0<sha>\":
      selected encoding not supported","Code":0,"Type":"error"}

so the CLI unzips an error message. It reproduces locally, is identical under
every Accept-Encoding, and is cached for 60 days — not a flake retrying would
clear.

`spice add spiceai/quickstart` only ever supplied one dataset, and the
integration job runs `-k "not cloud"`, so taxi_trips is all it needs — the tpch
tables belong to the cloud tests. Defining that dataset inline removes a shared
remote dependency that can, and just did, fail every PR at once.

Also wait until taxi_trips is queryable rather than until /v1/ready returns.
/v1/ready answers before an accelerated dataset finishes loading — locally it
passed at 2s against a load that completed at 11s — which races the tests into
"table 'taxi_trips' not found".

Verified locally: the inline pod loads 2,964,624 rows from the public bucket,
and `pytest tests/test_main.py -k "not cloud"` passes 12/12 against it.
requests' raise_for_status reports only "400 Client Error: Bad Request for
url: ...", dropping the body — which is where the runtime says what to fix,
for example "Search cannot be run on X because it has no embeddings or full
text search indexes".

Client.search now catches HTTPError and raises SpiceAIError with the message
unpacked. Both body shapes are handled: some failures return JSON
{"error": ...} and others plain text.

Caught by running the new search path against a live runtime.
Copilot AI review requested due to automatic review settings July 27, 2026 16:32
The lint job installs ruff from `ruff>=0.15.12`, so it picked up 0.16.0 as soon
as that released. 0.16 stabilizes PLR0917 (too many positional arguments), which
existing client constructors violate in two places — failing lint on every PR
regardless of what the PR changes.

PLR0913, the "too many arguments" counterpart, is already ignored, so this
codebase has an established position on wide signatures. Satisfying PLR0917
instead would mean making constructor parameters keyword-only, a breaking change
for a published SDK.

Verified with the exact version CI installs: `ruff@0.16.0 check spicepy tests`
goes from 2 errors to clean, and 0.15.12 still passes.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

spicepy/_search.py:51

  • SearchMatch.from_dict assumes payload is a dict and will raise an AttributeError if the runtime ever returns a non-dict match item (or if callers pass through unvalidated data). Since these helpers are public exports, it’s better to validate the wire shape and raise SpiceAIError with a clear message instead of failing with an unrelated attribute error.
    @classmethod
    def from_dict(cls, payload: dict[str, Any]) -> SearchMatch:
        """Build a match from the runtime's wire format.

        The wire format names the similarity score ``_score``.
        """
        return cls(
            dataset=payload.get("dataset", ""),
            score=payload.get("_score", 0.0),
            matches=payload.get("matches") or {},
            primary_key=payload.get("primary_key") or {},
            data=payload.get("data") or {},
            metadata=payload.get("metadata") or {},
        )

spicepy/_search.py:73

  • SearchResponse.from_dict can currently crash with AttributeError/TypeError if results is present but not a list (e.g. a dict due to a server-side change) because the list-comprehension will iterate unexpected values and pass them into SearchMatch.from_dict. Consider validating the results field type and raising SpiceAIError with the same “unexpected search response” message to keep failures actionable.
    @classmethod
    def from_dict(cls, payload: dict[str, Any]) -> SearchResponse:
        """Build a response from the runtime's wire format."""
        if not isinstance(payload, dict):
            raise SpiceAIError(f"unexpected search response from the runtime: {payload!r}")

        return cls(
            results=[SearchMatch.from_dict(m) for m in payload.get("results") or []],
            duration_ms=payload.get("duration_ms", 0),
        )

CI gates on black --check and flake8 in addition to ruff and pylint. The new
search files were formatted with ruff format, which disagrees with black, and
black's reformatting then pushed a test line past flake8's 120-column limit.

No behaviour change.
Copilot AI review requested due to automatic review settings July 27, 2026 18:13
build_search_body now takes its options keyword-only, matching Client.search
and every existing call site, so it no longer trips PLR0917.

Also suppresses the rule on two pre-existing __init__ signatures in _client.py.
Those already fail on unmodified trunk — ruff 0.16 added PLR0917 to the default
set and trunk's last green lint predates the bump — so the ruff gate cannot pass
without them. Suppression rather than resignature keeps the public constructor
unchanged; a maintainer may prefer to fix these on trunk separately.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

spicepy/_search.py:43

  • SearchMatch.from_dict assumes payload is a dict and will raise an AttributeError (not SpiceAIError) if the runtime returns an unexpected item type in results. Consider accepting Any and validating the payload type up front so callers always get a consistent, actionable exception.
    def from_dict(cls, payload: dict[str, Any]) -> SearchMatch:
        """Build a match from the runtime's wire format.

        The wire format names the similarity score ``_score``.
        """

Comment thread spicepy/_search.py
Copilot AI review requested due to automatic review settings July 27, 2026 18:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

spicepy/_search.py:41

  • Defensive decoding: SearchMatch.from_dict assumes payload and the matches/primary_key/data/metadata fields are dicts. If the runtime (or a proxy) returns an unexpected shape, this will raise AttributeError/TypeError instead of a consistent SpiceAIError.
    @classmethod
    def from_dict(cls, payload: dict[str, Any]) -> SearchMatch:
        """Build a match from the runtime's wire format.

spicepy/_search.py:75

  • Defensive decoding: SearchResponse.from_dict assumes results is a list of dicts. If results is present but not a list (or contains non-dicts), iteration will fail with a non-SpiceAIError exception; validating the shape makes failures clearer.
        return cls(
            results=[SearchMatch.from_dict(m) for m in payload.get("results") or []],
            duration_ms=payload.get("duration_ms", 0),
        )

#176 ignores PLR0917 repo-wide in pyproject.toml, which is the right fix — the
two flagged constructors fail on unmodified trunk, and satisfying the rule would
mean making public constructor parameters keyword-only.

RUF100 is enabled, so leaving per-line noqa directives here would turn into
"Unused `noqa` directive" errors the moment #176 lands. Dropping them makes this
branch depend on #176 rather than fight it.

build_search_body keeps its keyword-only signature — that stands on its own.
Copilot AI review requested due to automatic review settings July 27, 2026 21:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

spicepy/_search.py:43

  • SearchMatch.from_dict() assumes payload is a dict; if the runtime returns a malformed element inside results (e.g., a string/null), this will raise an AttributeError (.get) rather than a consistent SpiceAIError message.
    def from_dict(cls, payload: dict[str, Any]) -> SearchMatch:
        """Build a match from the runtime's wire format.

        The wire format names the similarity score ``_score``.
        """

spicepy/_search.py:71

  • SearchResponse.from_dict() iterates over payload.get("results") without validating its type. If the runtime sends results as a dict/string by mistake, the list-comprehension will iterate keys/characters and fail with a confusing downstream error.
        if not isinstance(payload, dict):
            raise SpiceAIError(
                f"unexpected search response from the runtime: {payload!r}"
            )

Copilot AI review requested due to automatic review settings July 29, 2026 21:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

.github/workflows/test.yml:163

  • The non-Windows readiness loop breaks when the dataset becomes queryable, but if it never does, the step still succeeds and the workflow proceeds to tests without emitting the spice log. This makes failures harder to diagnose and can produce misleading test errors. Consider failing explicitly after the loop (mirroring the Windows step) and printing spice.log.
          for i in {1..120}; do
            if curl -s -X POST http://localhost:8090/v1/sql \
                 -d 'SELECT count(*) AS n FROM taxi_trips' 2>/dev/null | grep -q '"n"'; then
              echo "Spice runtime is ready and taxi_trips is queryable"
              break

from_dict assumed every level was the documented type. An error page, a proxy
response, or a malformed field would surface as an AttributeError further along,
or quietly populate a field with the wrong type.

Now: a non-dict response or match raises SpiceAIError, non-list results raise,
and the four optional objects coerce to {} rather than keeping whatever was
there. Parameter types widened to Any, since these already handle non-dict input
at runtime.

Adds 5 tests, including an HTML error page in place of a result.
Copilot AI review requested due to automatic review settings July 29, 2026 23:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

spicepy/_search.py:67

  • SearchMatch.from_dict() assigns _score directly to score without coercion. If the runtime/proxy returns null, a string, or a boolean, this produces a non-float score despite the declared type and can break callers later.
        return cls(
            dataset=payload.get("dataset", ""),
            score=payload.get("_score", 0.0),

.github/workflows/test.yml:163

  • In the non-Windows job, the readiness loop breaks on success but never fails the step on timeout. If taxi_trips never becomes queryable, the workflow will continue and fail later in a less actionable way (or flake).
          for i in {1..120}; do
            if curl -s -X POST http://localhost:8090/v1/sql \
                 -d 'SELECT count(*) AS n FROM taxi_trips' 2>/dev/null | grep -q '"n"'; then
              echo "Spice runtime is ready and taxi_trips is queryable"
              break

spicepy/_search.py:99

  • SearchResponse.from_dict() uses payload.get("results") or [], which silently converts falsy non-list values (e.g., {}) into an empty list and skips the shape validation. That contradicts the goal of failing loudly on malformed responses.
        results = payload.get("results") or []
        if not isinstance(results, list):
            raise SpiceAIError(
                f"unexpected search results from the runtime: {results!r}"
            )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants