added tools filtering by blublinsky · Pull Request #2741 · openshift/lightspeed-service

blublinsky · 2026-02-10T10:39:36Z

Description

Type of change

Related Tickets & Documents

Related Issue #
OLS-2275
Closes #
OLS-2275

Checklist before requesting a review

I have performed a self-review of my code.
PR has passed all pre-merge test jobs.
If it is a core feature, I have added thorough tests.

Testing

Please provide detailed steps to perform tests related to this code change.
How were the fix/results from this change verified? Please provide relevant screenshots or results.

openshift-ci · 2026-02-10T10:39:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bparees for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

blublinsky · 2026-02-10T12:03:45Z

/retest

onmete

A couple of high-level items.

onmete · 2026-02-10T12:29:12Z

ols/src/tools/tools_rag/hybrid_tools_rag.py

+            top_k: Number of tools to retrieve
+            threshold: Minimum similarity threshold for filtering results
+        """
+        self.embed_model = embed_model or "sentence-transformers/all-mpnet-base-v2"


This shouldn't be configurable for now. It needs to use the hardcoded value of embedding that we have baked in the image.

Why not. We already have this config integrated with the operator

Because it would fail in disconnected environments. Why do we need to support the embedding model switch?

onmete · 2026-02-10T12:31:45Z

ols/utils/mcp_utils.py

+                user_token,
+                client_headers,
+                allowed_tool_names=filtered_tool_names,
+            )


After this method identifies which tools are relevant, it opens new MCP connections to re-fetch those same tools (_gather_and_populate_tools with allowed_tool_names). Why not cache the StructuredTool objects from the initial population and return those directly?

Good question.
The issue is that tools have authentication baked in. We need to refetch them with a new authentication to make calls correct

onmete · 2026-02-10T12:50:23Z

ols/utils/config.py

+        Without this lock, one user's client tools could leak into another user's
+        filtered results.
+        """
+        return asyncio.Lock()


So the lock is here basically because of the requests with client headers, right?
Client headers are an edge case - a minority of requests, for a minority of server configurations. And the current OLS (UI) is not supporting that.
I really don't like that it affects the architecture of implementation this much.

See this scenario:

Index contains: get_pods (k8s-auth) + github_create_pr (client-auth)

Query: "create a pull request"

ToolsRAG returns: github_create_pr (most relevant)

Client didn't send GitHub headers this time

Tool gets dropped -> empty result
The user gets nothing, even though get_pods was in the index and accessible. The most relevant tool won the ranking but was inaccessible, and it pushed out the tools that were actually available.

I think we need static index for tools that are ALWAYS accessible and ANOTHER one for client tools which will be added based on the request headers (and then combine/filter results).
Then both indexes are static, hence no need for lock.

The lock is only for mcp_headers. If they are not present, there is no lock. So if you consider mcp_headers an edge case, so is the lock

onmete · 2026-02-10T12:53:38Z

ols/src/tools/tools_rag/hybrid_tools_rag.py

+        self.alpha = alpha
+        self.top_k = top_k
+        self.threshold = threshold
+        self.embedding_model = SentenceTransformer(embed_model)


We are creating a second instance of the same embedding model we have for RAG. We need to reuse that or at least confirm the library itself is handling that, to not load another ~400MB into memory.

fixed in config

blublinsky · 2026-02-10T14:36:37Z

/retest

blublinsky · 2026-02-11T08:22:08Z

/retest

blublinsky · 2026-02-11T09:01:26Z

/retest

blublinsky · 2026-02-12T09:01:39Z

/retest

blublinsky · 2026-02-12T19:37:55Z

/retest

blublinsky · 2026-02-13T15:28:14Z

/override "ci/prow/e2e-ols-cluster"
Error is unrelated to the code

openshift-ci · 2026-02-13T15:29:38Z

@blublinsky: Overrode contexts on behalf of blublinsky: ci/prow/e2e-ols-cluster

Details

In response to this:

/override "ci/prow/e2e-ols-cluster"
Error is unrelated to the code

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci · 2026-02-13T23:59:42Z

@blublinsky: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot requested review from onmete and tisnik February 10, 2026 10:39

onmete reviewed Feb 10, 2026

View reviewed changes

blublinsky force-pushed the tools-filtering branch from 89e5515 to b1b66d8 Compare February 10, 2026 14:22

blublinsky force-pushed the tools-filtering branch 2 times, most recently from f7d057c to 70904be Compare February 10, 2026 21:32

blublinsky requested a review from onmete February 11, 2026 11:12

blublinsky force-pushed the tools-filtering branch 3 times, most recently from 805285d to 8aa9c0a Compare February 12, 2026 08:53

blublinsky force-pushed the tools-filtering branch 3 times, most recently from 5e4f971 to 5b0818a Compare February 12, 2026 17:07

blublinsky force-pushed the tools-filtering branch 4 times, most recently from 9e3954b to 14a9bac Compare February 13, 2026 12:18

added tools filtering

64a169a

blublinsky force-pushed the tools-filtering branch from 14a9bac to 64a169a Compare February 13, 2026 22:33

Conversation

blublinsky commented Feb 10, 2026

Description

Type of change

Related Tickets & Documents

Checklist before requesting a review

Testing

Uh oh!

openshift-ci bot commented Feb 10, 2026

Uh oh!

blublinsky commented Feb 10, 2026

Uh oh!

onmete left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blublinsky Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blublinsky commented Feb 10, 2026

Uh oh!

blublinsky commented Feb 11, 2026

Uh oh!

blublinsky commented Feb 11, 2026

Uh oh!

blublinsky commented Feb 12, 2026

Uh oh!

blublinsky commented Feb 12, 2026

Uh oh!

blublinsky commented Feb 13, 2026

Uh oh!

openshift-ci bot commented Feb 13, 2026

Uh oh!

openshift-ci bot commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blublinsky Feb 10, 2026 •

edited

Loading