Skip to content

fix(dspy): scope WeaviateRM tenant retrieval to the collection not the query namespace - #98

Open
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-dspy-scope-weaviaterm-tenant-retrieval-to-the-c6fa2a
Open

fix(dspy): scope WeaviateRM tenant retrieval to the collection not the query namespace#98
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-dspy-scope-weaviaterm-tenant-retrieval-to-the-c6fa2a

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 6, 2026

Copy link
Copy Markdown

Warning

GitHub issue creation failed

Detail attempted to publish this bug to GitHub, but the issue could not be created. This fix PR was created without that issue, and missing tracker references are shown as Unknown issue.

You can review and merge this PR normally. Please review your tracker integration settings before the next publish run.

Detail bug report: View on Detail

📝 Changes Description

Closes Unknown issue

Bug: On the Weaviate v4 (WeaviateClient) tenant path, WeaviateRM.forward() called self._weaviate_collection.query.with_tenant(tenant). collection.query returns a _QueryCollection, which has no with_tenant in any pinned version of weaviate-client (>=4.5.4,<4.22.0); with_tenant lives on the Collection itself. Every v4 retrieval with a tenant_id (constructor or per-call) raised AttributeError: '_QueryCollection' object has no attribute 'with_tenant', leaving the entire multi-tenancy feature (added in 10fe155a) unreachable on the v4 client — the only client for which insert/get_objects are supported.

Fix: Scope the collection to the tenant before querying — the v4 API's prescribed pattern, where Collection.with_tenant(t) returns a new tenant-scoped collection whose .query.hybrid(...) threads the tenant into the gRPC request:

collection = self._weaviate_collection.with_tenant(tenant) if tenant else self._weaviate_collection
results = collection.query.hybrid(query=query, limit=k, **kwargs)

This collapses the prior if tenant/else into one query call. The v3 Client branch is unchanged — with_tenant legitimately exists on its GraphQL Get builder.

Testing:

  • Unit tests (new, tests/retrievers/test_weaviate_rm.py): run without the weaviate extra by stubbing the optional import in sys.modules (the test_gepa.py pattern). They lock in the v4 API invariant (with_tenant on Collection, not on query), the fix's tenant-scoping behavior, the no-tenant path, that tenant_id is consumed and not forwarded to hybrid, and the v3 GraphQL-builder path. Reverting the fix makes the v4 tenant tests fail with the exact production AttributeError. Reverting/restore was confirmed during verification.
  • Routine checks: ruff check, ruff format --check, the retriever unit tests, and the broader default test suite all pass with no regressions.
  • Against the real weaviate-client==4.21.3: Collection has with_tenant; _QueryCollection has hybrid but not with_tenant; the fixed call chain reaches the gRPC send layer with the tenant threaded through (scoped._query._tenant == "T1").
  • End-to-end on a live Weaviate v4 server (Docker weaviate:1.30.0): created a multi-tenant collection with two tenants and confirmed WeaviateRM returns tenant-scoped results. Querying Tenant1 for content present only in Tenant2 returns only Tenant1 docs (no boundary leak), proving the tenant actually reaches the request rather than just chaining a method call. Per-call tenant_id override, k honoring, and __call__/forward parity also verified.
  • v3 path at weaviate-client==4.5.4 against the live server: the with_tenant GraphQL idiom returns tenant-scoped results. The full v3 forward() call could not be exercised because the base Docker image ships no vectorizer module and the test collection uses vectorizer=none; the v3 branch's with_hybrid(query=query) (no vector) is rejected by the server (VectorFromInput was called without vectorizer), which is independent of this fix.

✅ Contributor Checklist

  • Pre-Commit checks are passing (locally and remotely)
  • Title of your PR / MR corresponds to the required format
  • Commit message follows required format {label}(dspy): {message}

⚠️ Warnings

  • ruff format was applied to dspy/retrievers/weaviate_rm.py; the formatter also normalized a few lines outside the v4 tenant branch (the v3 query.get(...) continuation, # TODO comment spacing, and insert/get_objects indentation). Those are formatter-only changes, not logic changes — the project's pre-commit hook would produce the same edits.
  • Authored by Detail: Automatic Fixes.

Automatic Fixes PRs can be configured here.

@greptile-apps

greptile-apps Bot commented Sep 6, 2026

Copy link
Copy Markdown

Greptile Summary

This PR corrects Weaviate v4 tenant retrieval by scoping the collection before invoking hybrid search and adds regression coverage for tenant, non-tenant, keyword-forwarding, and legacy-client paths.

  • Moves with_tenant() from the v4 query namespace to the collection.
  • Consolidates tenant and non-tenant hybrid retrieval into one query call.
  • Adds focused retriever tests, although their global dependency stub and synthetic v3 construction should be improved.
  • Includes formatting-only cleanup in the legacy retrieval and object-management branches.

Confidence Score: 4/5

The production fix appears safe to merge, with non-blocking test-isolation and test-fidelity concerns remaining.

The v4 tenant query now follows the supported Weaviate collection-scoping API without changing non-tenant behavior; the identified issues affect test reliability and the credibility of legacy-client coverage rather than production retrieval.

Files Needing Attention: tests/retrievers/test_weaviate_rm.py

Important Files Changed

Filename Overview
dspy/retrievers/weaviate_rm.py Correctly moves v4 tenant scoping onto the collection before hybrid retrieval; remaining changes are formatting-only.
tests/retrievers/test_weaviate_rm.py Adds useful tenant regression coverage, but globally shadows the optional dependency and tests v3 through an unreachable synthetic construction path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Q[Query and optional tenant_id] --> T{Tenant supplied?}
    T -->|Yes| S[Collection.with_tenant]
    T -->|No| C[Base collection]
    S --> H[Scoped collection.query.hybrid]
    C --> H
    H --> R[Weaviate results]
    R --> P[DSPy passage predictions]
Loading

Reviews (1): Last reviewed commit: "fix(dspy): scope WeaviateRM tenant retri..." | Re-trigger Greptile

Comment on lines +21 to +22
sys.modules["weaviate"] = _weaviate_stub
sys.modules["weaviate.util"] = _weaviate_util_stub

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Stub Shadows Real Package

The module-level sys.modules assignments permanently shadow the real weaviate package whenever it is installed but has not yet been imported. Because pytest imports this file during collection, later integration tests receive this incomplete stub instead of the installed client, which can cause order-dependent failures or invalid test behavior. Use a scoped patch that restores sys.modules after importing the module under test.

Comment on lines +139 to +143
return rm, client


def test_v4_query_namespace_has_no_with_tenant():
"""The query object returned by `collection.query` must NOT expose `with_tenant`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Test Bypasses Public Constructor

This v3 regression test bypasses the public constructor and manually forces the internal client type. It therefore passes even though a real v3 client cannot construct WeaviateRM, because __init__ accesses client.collections before checking for the v3 query interface. This gives misleading coverage of behavior users cannot reach; exercise construction through the public API or avoid presenting this as a v3 behavior regression test.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant