OLS-3310 Add OKP Solr hybrid RAG via tool-based retrieval#2926
OLS-3310 Add OKP Solr hybrid RAG via tool-based retrieval#2926blublinsky wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
blublinsky
force-pushed
the
okp-rag
branch
2 times, most recently
from
4c89880 to
99d82f9
Compare
blublinsky
force-pushed
the
okp-rag
branch
3 times, most recently
from
914faae to
37a7b67
Compare
|
|
||
| SOLR_DOCS_TOOL_AGENT_SUPPLEMENT = """ | ||
| Solr docs tool: | ||
| * ``search_openshift_documentation`` searches published Red Hat product docs (not the cluster). Prefer one short query before answering from memory on product behavior, APIs, operators, upgrades, or whether docs exist. |
There was a problem hiding this comment.
"(not the cluster)" seems not clear: not the documents about this specific cluster? tool does not search inside the cluster for related objects?
|
|
||
| Omit the key to leave Solr hybrid RAG off (no client, no tool). When present, values map to `SolrHybridSettings`; `enabled: false` disables the feature without removing the block. `solr_http_base` must be a valid `http` or `https` URL with a host when `enabled` is true (checked in `validate_yaml()`). | ||
|
|
||
| When ``solr_hybrid.enabled`` is true, every entry under ``reference_content.indexes`` |
There was a problem hiding this comment.
solr_hybrid in config does not have the property enabled.
we can change the phrase to "when solr_hybrid" is defined in the config" corresponding to current code in PR
| "No docs above hybrid_score_threshold=%s for: %s", | ||
| cfg.hybrid_score_threshold, | ||
| query, | ||
| ) |
There was a problem hiding this comment.
should it be added here the fallback to "/select" endpoint?
There was a problem hiding this comment.
hybrid_score_threshold defaults to 0.0 (no filtering) and the operator does not expose this parameter — it always uses the default. When someone explicitly sets a threshold, they're choosing "no context over bad context." Falling back to /select would return the same or weaker documents that the hybrid path already rejected, undermining that intent.
| if retrieved_nodes: | ||
| logger.info( | ||
| "Retrieved %d document nodes for RAG context", len(retrieved_nodes) | ||
| ) |
There was a problem hiding this comment.
would it be useful to sort the documents from multiple sources using the same standard to avoid those at the beggining of the list are most weighted to the LLM's reasoning?
There was a problem hiding this comment.
Valid concern, but in practice:
FAISS and Solr are never used together for OpenShift docs — once we switch to Solr, FAISS-based product documentation goes away.
They can coexist for OpenShift docs (Solr) + BYOK. In that case there are two options: BYOK as direct RAG + Solr as a tool, or both BYOK and Solr as tools (PR for this is coming).
So the mixed-source scoring problem doesn't arise in the direct RAG path — it's either one source or the other, never both unsorted.
|
/retest e2e-ols-cluster |
|
/retest |
|
the new code looks good :) |
|
/retest |
1 similar comment
|
/retest |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
ols/src/query_helpers/docs_summarizer.py (1)
203-274: 📐 Maintainability & Code Quality | 🔵 TrivialKeep audit instrumentation on the Solr-only direct-RAG path.
The new
elif retrieved_nodes:branch returns Solr passages without entering therequest.ragspan or callingaudit_ctx.logger.rag_retrieved(...), sosolr_direct_rag=Truerequests lose retrieval telemetry whenever no local retriever is configured. Consider sharing the truncate/log/audit block between both branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/src/query_helpers/docs_summarizer.py` around lines 203 - 274, The Solr-only direct-RAG path in the document retrieval flow is skipping audit telemetry because the shared logging and span updates only happen inside the rag_retriever branch. Update the retrieval logic in docs_summarizer.py around the retrieval block in the main query helper so both the rag_retriever path and the retrieved_nodes-only Solr path reuse the same truncate/context logging/audit handling, including request.rag span attributes and audit_ctx.logger.rag_retrieved calls, using the existing retrieved_nodes, rag_chunks, and span symbols.tests/unit/utils/test_config.py (1)
1218-1218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Noneto the new test function.The added test omits a return annotation. As per coding guidelines, "Use type hints for all function signatures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/utils/test_config.py` at line 1218, The new test function test_solr_hybrid_search_uses_fixed_portal_rag_embedding_model is missing a return type annotation. Update the function signature to include -> None so it matches the project’s typing guidelines for all test functions.Source: Coding guidelines
tests/unit/prompts/test_prompt_generator.py (1)
394-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Noneto the new test functions.The added tests in this block omit return annotations. As per coding guidelines, "Use type hints for all function signatures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/prompts/test_prompt_generator.py` around lines 394 - 437, The new test functions in test_prompt_generator.py are missing explicit return type annotations, which violates the function signature typing guideline. Update each added test (for example, the solr_docs_tool_guidance_* tests around GeneratePrompt) to include a -> None return annotation while keeping the existing assertions and setup unchanged.Source: Coding guidelines
tests/unit/app/models/test_config.py (1)
2262-2406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Noneto the new test functions.The added tests in this block omit return annotations. As per coding guidelines, "Use type hints for all function signatures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/app/models/test_config.py` around lines 2262 - 2406, The new test functions in test_config.py are missing explicit return type annotations, which violates the function signature typing guideline. Add -> None to each added test definition in this block, including the test_ols_config_solr_hybrid_parses_from_yaml_dict, test_config_reserves_tool_budget_for_solr_tool_only_without_mcp, test_ols_config_solr_hybrid_validate_yaml_rejects_bad_url, test_ols_config_validate_yaml_solr_hybrid_rejects_invalid_base_url, test_ols_config_validate_yaml_rejects_solr_with_non_byok_indexes, and test_ols_config_validate_yaml_allows_solr_with_byok_indexes functions.Source: Coding guidelines
tests/unit/query_helpers/test_docs_summarizer.py (1)
78-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
-> Noneto the new test functions.The added tests in these blocks omit return annotations. As per coding guidelines, "Use type hints for all function signatures."
Also applies to: 218-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/query_helpers/test_docs_summarizer.py` around lines 78 - 99, The new test functions in test_docs_summarizer should explicitly declare a None return type to match the type-hinting guideline. Update the signatures of test_tool_calling_enabled_when_solr_docs_tool_active_without_mcp and test_tool_calling_disabled_without_mcp_and_without_solr_docs_tool, and also any similar newly added tests in the same DocsSummarizer test block, to include -> None. Keep the bodies unchanged; this is just a signature annotation fix.Source: Coding guidelines
pyproject.toml (1)
138-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep a single
opentelemetry-apirequirement.
project.dependenciesalready hasopentelemetry-api>=1.20.0on Line 144, so adding another entry here makes the intended minimum version ambiguous and easy to drift later.Also applies to: 144-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 138, Remove the duplicate opentelemetry-api dependency so project.dependencies keeps only one requirement entry. Update the dependencies list in pyproject.toml to keep the single canonical declaration (the existing opentelemetry-api spec) and delete the extra one, ensuring the minimum version is defined in only one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/olsconfig.yaml`:
- Around line 78-90: The sample config under reference_content.indexes does not
satisfy OLSConfig.validate_yaml() when solr_hybrid is enabled because every
index must be marked byok_index: true. Update the example so each index entry in
the indexes list explicitly includes byok_index: true, and make sure the
commented guidance matches the actual required shape in
OLSConfig.validate_yaml() and the solr_hybrid/reference_content section.
In `@ols/app/models/config.py`:
- Line 1010: Avoid truthiness coercion when setting OLSConfig.byok_index, since
bool(data.get("byok_index", False)) will treat non-empty strings like "false" as
true and can bypass _validate_solr_hybrid_vs_reference_indexes(). Update the
byok_index assignment in OLSConfig to preserve a real boolean from the config
data or explicitly parse/validate the input type before storing it, so string
values cannot accidentally enable BYOK indexing.
- Around line 1148-1154: The validate_yaml method currently allows
solr_http_base values that already contain a /solr path, which later breaks
endpoint construction in Solr support. Update validate_yaml in Config to reject
URLs whose path is already /solr (or ends with that segment) while keeping the
existing http/https host validation, so callers of solr_support’s hybrid-search
URL building do not produce a duplicated /solr/solr path at runtime.
In `@ols/src/rag_index/solr_support.py`:
- Around line 543-574: The search_openshift_documentation tool description and
return contract are inconsistent with _search_openshift_documentation, since
client.search() already swallows Solr/embedding/HTTP failures and returns an
empty list. Update _search_openshift_documentation and the
StructuredTool.from_function description to match the actual behavior: either
let exceptions propagate so the {"error": ...} path is reachable, or remove the
dead exception handling and document that the tool returns an array (including
[]) rather than an error object.
In `@ols/src/rag/stop_words.py`:
- Around line 74-76: The stop-word list currently includes negation terms, which
causes query normalization and tokenization to strip meaning from phrases like
“pod not ready.” Update the stop-word set in stop_words.py to keep “not”, “no”,
and “nor” out of the list so normalize_solr_hybrid_query() and _tokenize()
preserve negation intent.
In `@ols/utils/config.py`:
- Around line 325-330: Fail fast instead of returning None when Solr hybrid
embedding resolution fails in Config._solr_hybrid_embed_model /
_compute_tool_budgets path. The current exception handler silently disables the
Solr docs tool after tool tokens were already reserved, so change the fallback
to raise for tool-only Solr mode (or otherwise propagate the error) and keep the
existing logger.exception context so DocsSummarizer does not continue with an
unusable smaller budget.
In `@tests/unit/app/endpoints/test_health.py`:
- Around line 174-183: Restore the shared singleton state in the readiness tests
after each scenario by saving and resetting the original values of
config._rag_index_loader and config.ols_config.reference_content around the
assertions in test_health.py; update the affected cases in the readiness probe
tests so they clean up the temporary Mock loader and reference content instead
of leaving mutated config behind.
In `@tests/unit/utils/test_config.py`:
- Around line 1272-1284: The test in test_config.py is leaking cached AppConfig
state, making the HuggingFaceEmbedding assertion order-dependent. Before
accessing solr_hybrid_search, clear the actual private cache used by
AppConfig._solr_hybrid_embed_model() (including _cached_solr_embed_model, and
any related private loader cache like _rag_index_loader), not just entries in
config.__dict__. This ensures the patched constructor is always exercised and
the constructor call assertion is reliable.
---
Nitpick comments:
In `@ols/src/query_helpers/docs_summarizer.py`:
- Around line 203-274: The Solr-only direct-RAG path in the document retrieval
flow is skipping audit telemetry because the shared logging and span updates
only happen inside the rag_retriever branch. Update the retrieval logic in
docs_summarizer.py around the retrieval block in the main query helper so both
the rag_retriever path and the retrieved_nodes-only Solr path reuse the same
truncate/context logging/audit handling, including request.rag span attributes
and audit_ctx.logger.rag_retrieved calls, using the existing retrieved_nodes,
rag_chunks, and span symbols.
In `@pyproject.toml`:
- Line 138: Remove the duplicate opentelemetry-api dependency so
project.dependencies keeps only one requirement entry. Update the dependencies
list in pyproject.toml to keep the single canonical declaration (the existing
opentelemetry-api spec) and delete the extra one, ensuring the minimum version
is defined in only one place.
In `@tests/unit/app/models/test_config.py`:
- Around line 2262-2406: The new test functions in test_config.py are missing
explicit return type annotations, which violates the function signature typing
guideline. Add -> None to each added test definition in this block, including
the test_ols_config_solr_hybrid_parses_from_yaml_dict,
test_config_reserves_tool_budget_for_solr_tool_only_without_mcp,
test_ols_config_solr_hybrid_validate_yaml_rejects_bad_url,
test_ols_config_validate_yaml_solr_hybrid_rejects_invalid_base_url,
test_ols_config_validate_yaml_rejects_solr_with_non_byok_indexes, and
test_ols_config_validate_yaml_allows_solr_with_byok_indexes functions.
In `@tests/unit/prompts/test_prompt_generator.py`:
- Around line 394-437: The new test functions in test_prompt_generator.py are
missing explicit return type annotations, which violates the function signature
typing guideline. Update each added test (for example, the
solr_docs_tool_guidance_* tests around GeneratePrompt) to include a -> None
return annotation while keeping the existing assertions and setup unchanged.
In `@tests/unit/query_helpers/test_docs_summarizer.py`:
- Around line 78-99: The new test functions in test_docs_summarizer should
explicitly declare a None return type to match the type-hinting guideline.
Update the signatures of
test_tool_calling_enabled_when_solr_docs_tool_active_without_mcp and
test_tool_calling_disabled_without_mcp_and_without_solr_docs_tool, and also any
similar newly added tests in the same DocsSummarizer test block, to include ->
None. Keep the bodies unchanged; this is just a signature annotation fix.
In `@tests/unit/utils/test_config.py`:
- Line 1218: The new test function
test_solr_hybrid_search_uses_fixed_portal_rag_embedding_model is missing a
return type annotation. Update the function signature to include -> None so it
matches the project’s typing guidelines for all test functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: af5959a1-3ceb-4f37-a9b3-688e1d9f7132
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (19)
docs/ai/config.mdexamples/olsconfig.yamlols/app/endpoints/ols.pyols/app/models/config.pyols/constants.pyols/src/prompts/prompt_generator.pyols/src/prompts/prompts.pyols/src/query_helpers/docs_summarizer.pyols/src/rag/hybrid_rag.pyols/src/rag/stop_words.pyols/src/rag_index/solr_support.pyols/utils/config.pypyproject.tomltests/unit/app/endpoints/test_health.pytests/unit/app/models/test_config.pytests/unit/prompts/test_prompt_generator.pytests/unit/query_helpers/test_docs_summarizer.pytests/unit/rag_index/test_solr_support.pytests/unit/utils/test_config.py
| self.product_docs_index_path = data.get("product_docs_index_path", None) | ||
| self.product_docs_index_id = data.get("product_docs_index_id", None) | ||
| self.product_docs_origin = data.get("product_docs_origin", None) | ||
| self.byok_index = bool(data.get("byok_index", False)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid truthiness coercion for byok_index.
bool(data.get("byok_index", False)) turns non-empty strings like "false" into True, so a quoted/string config value can accidentally bypass the Solr-vs-local-index check in OLSConfig._validate_solr_hybrid_vs_reference_indexes().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/app/models/config.py` at line 1010, Avoid truthiness coercion when
setting OLSConfig.byok_index, since bool(data.get("byok_index", False)) will
treat non-empty strings like "false" as true and can bypass
_validate_solr_hybrid_vs_reference_indexes(). Update the byok_index assignment
in OLSConfig to preserve a real boolean from the config data or explicitly
parse/validate the input type before storing it, so string values cannot
accidentally enable BYOK indexing.
| def validate_yaml(self) -> None: | ||
| """Validate Solr hybrid settings.""" | ||
| if not checks.is_valid_http_url(self.solr_http_base): | ||
| raise checks.InvalidConfigurationError( | ||
| "solr_hybrid.solr_http_base must be a valid http or https URL with a " | ||
| f"host; got {self.solr_http_base!r}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject solr_http_base values that already include /solr.
This validator accepts https://host/solr, but ols/src/rag_index/solr_support.py then appends /solr/{collection}/hybrid-search on Lines 483-495, producing a broken .../solr/solr/... endpoint at runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/app/models/config.py` around lines 1148 - 1154, The validate_yaml method
currently allows solr_http_base values that already contain a /solr path, which
later breaks endpoint construction in Solr support. Update validate_yaml in
Config to reject URLs whose path is already /solr (or ends with that segment)
while keeping the existing http/https host validation, so callers of
solr_support’s hybrid-search URL building do not produce a duplicated /solr/solr
path at runtime.
| "not", | ||
| "no", | ||
| "nor", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep negation terms out of the stop-word list.
normalize_solr_hybrid_query() and _tokenize() both consume this set, so dropping not/no/nor turns queries like pod not ready into pod ready and can invert retrieval intent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/src/rag/stop_words.py` around lines 74 - 76, The stop-word list currently
includes negation terms, which causes query normalization and tokenization to
strip meaning from phrases like “pod not ready.” Update the stop-word set in
stop_words.py to keep “not”, “no”, and “nor” out of the list so
normalize_solr_hybrid_query() and _tokenize() preserve negation intent.
| try: | ||
| embed_model = self._solr_hybrid_embed_model() | ||
| encode_fn = embed_model.get_text_embedding | ||
| except Exception: | ||
| logger.exception("Failed to resolve embedding model for Solr hybrid RAG") | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't silently disable configured Solr tool-only mode.
When this returns None, DocsSummarizer disables the built-in Solr docs tool, but Config._compute_tool_budgets() has already reserved tool tokens for solr_direct_rag=False. The result is a smaller prompt budget with no usable tool. This should fail fast, at least for tool-only Solr mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/utils/config.py` around lines 325 - 330, Fail fast instead of returning
None when Solr hybrid embedding resolution fails in
Config._solr_hybrid_embed_model / _compute_tool_budgets path. The current
exception handler silently disables the Solr docs tool after tool tokens were
already reserved, so change the fallback to raise for tool-only Solr mode (or
otherwise propagate the error) and keep the existing logger.exception context so
DocsSummarizer does not continue with an unusable smaller budget.
| config.reload_empty() | ||
| config.config = config._load_config_from_yaml_stream( | ||
| io.StringIO(yaml_stream), ignore_missing_certs=True | ||
| ) | ||
| for key in ( | ||
| "solr_hybrid_search", | ||
| "rag_index_loader", | ||
| "mcp_servers_dict", | ||
| "tools_rag", | ||
| "skills_rag", | ||
| ): | ||
| config.__dict__.pop(key, None) | ||
| client = config.solr_hybrid_search |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear the actual Solr embedding cache before asserting constructor calls.
AppConfig._solr_hybrid_embed_model() short-circuits on config._cached_solr_embed_model, and reload_empty() only replaces config.config. If a previous test already primed that cache, this test never reaches the patched HuggingFaceEmbedding, so assert_called_once_with(...) becomes order-dependent. Also, pop("rag_index_loader", ...) does not clear the private _rag_index_loader field.
Suggested fix
mock_hf.return_value = _StubEmbed()
config.reload_empty()
+ config._cached_solr_embed_model = None
+ config._rag_index_loader = None
config.config = config._load_config_from_yaml_stream(
io.StringIO(yaml_stream), ignore_missing_certs=True
)
for key in (
"solr_hybrid_search",
- "rag_index_loader",
"mcp_servers_dict",
"tools_rag",
"skills_rag",
):
config.__dict__.pop(key, None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config.reload_empty() | |
| config.config = config._load_config_from_yaml_stream( | |
| io.StringIO(yaml_stream), ignore_missing_certs=True | |
| ) | |
| for key in ( | |
| "solr_hybrid_search", | |
| "rag_index_loader", | |
| "mcp_servers_dict", | |
| "tools_rag", | |
| "skills_rag", | |
| ): | |
| config.__dict__.pop(key, None) | |
| client = config.solr_hybrid_search | |
| mock_hf.return_value = _StubEmbed() | |
| config.reload_empty() | |
| config._cached_solr_embed_model = None | |
| config._rag_index_loader = None | |
| config.config = config._load_config_from_yaml_stream( | |
| io.StringIO(yaml_stream), ignore_missing_certs=True | |
| ) | |
| for key in ( | |
| "solr_hybrid_search", | |
| "mcp_servers_dict", | |
| "tools_rag", | |
| "skills_rag", | |
| ): | |
| config.__dict__.pop(key, None) | |
| client = config.solr_hybrid_search |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/utils/test_config.py` around lines 1272 - 1284, The test in
test_config.py is leaking cached AppConfig state, making the
HuggingFaceEmbedding assertion order-dependent. Before accessing
solr_hybrid_search, clear the actual private cache used by
AppConfig._solr_hybrid_embed_model() (including _cached_solr_embed_model, and
any related private loader cache like _rag_index_loader), not just entries in
config.__dict__. This ensures the patched constructor is always exercised and
the constructor call assertion is reliable.
openshift-ci
Bot
added
the
needs-rebase
blublinsky
force-pushed
the
okp-rag
branch
2 times, most recently
from
2fd2994 to
29d62eb
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/rag_index/test_index_loader.py (1)
32-40: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPatch
HuggingFaceEmbeddinghere instead of constructing the real model.This unit test now exercises the actual embedding loader, and Line 34 even clears
TRANSFORMERS_OFFLINE, so it can depend on local model state or network access. That makes the test non-hermetic and prone to CI flakes. Stub the constructor liketests/unit/utils/test_config.pydoes and assert the requestedmodel_nameon the mock instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/rag_index/test_index_loader.py` around lines 32 - 40, This unit test is still constructing the real HuggingFaceEmbedding through IndexLoader, making it non-hermetic and dependent on local or network model availability. Update the test around IndexLoader.vector_indexes to patch HuggingFaceEmbedding the same way as in tests/unit/utils/test_config.py, then assert the mock was called with model_name set to all-mpnet-base-v2 instead of inspecting a real _embed_model instance.
♻️ Duplicate comments (1)
examples/olsconfig.yaml (1)
83-89: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSample config still fails
validate_yaml().With
solr_hybridenabled,_validate_solr_hybrid_vs_reference_indexesrejects any index withoutbyok_index: true. The first index (Lines 85-86) omits the flag entirely, and the second (Lines 87-89) only shows it commented out — same issue flagged in a prior review, still unresolved.Suggested fix
reference_content: indexes: - - product_docs_index_path: "./vector_db/ocp_product_docs/4.17" - product_docs_index_id: ocp-product-docs-4_17 - product_docs_index_path: "./vector_db/user_application_docs/version_1" product_docs_index_id: user-application-docs-version_1 - # byok_index: true + byok_index: true
🧹 Nitpick comments (1)
ols/utils/config.py (1)
181-187: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one cached BYOK embedding instance.
tools_ragandskills_ragboth instantiate the sameHuggingFaceEmbeddingmodel separately. If both features are enabled, the process pays the model load cost twice and keeps duplicate weights in memory. Pull this behind one shared cached helper/property and reuse it from both paths.Also applies to: 224-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/utils/config.py` around lines 181 - 187, The BYOK embedding setup currently creates separate HuggingFaceEmbedding instances in both the tools_rag and skills_rag paths, duplicating load time and memory. Move the model creation behind one shared cached helper/property in config.py, and have both paths reuse that same cached instance instead of instantiating HuggingFaceEmbedding directly. Use the existing embed_model setup and constants.EMBEDDINGS_MODEL_BYOK_SUBDIR as the single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Containerfile`:
- Around line 89-95: The Cachi2 pre-copy step in the Containerfile is too
brittle because the Granite model copy can fail and stop the entire RUN before
the fallback download loop executes. Update the RUN block that copies into
embeddings_model/all-mpnet-base-v2 and
embeddings_model/granite-embedding-30m-english so each source file is checked
before copying, or gate each cp independently inside the existing
/cachi2/output/deps/generic path. Keep the later for f in loop intact so missing
models can still be downloaded.
In `@ols/src/query_helpers/docs_summarizer.py`:
- Around line 137-141: The docs_summarizer tool assembly is adding a duplicate
tool name when `get_openshift_docs_tool` is appended after `get_mcp_tools`.
Update the `get_mcp_tools`/`get_openshift_docs_tool` flow in `DocsSummarizer` so
`search_openshift_documentation` is only added if no existing tool with that
name is already present, or otherwise skip the local tool when MCP already
provides it. Use the unique symbols `get_mcp_tools`, `get_openshift_docs_tool`,
and `tools.append(...)` to locate and guard this merge logic.
---
Outside diff comments:
In `@tests/unit/rag_index/test_index_loader.py`:
- Around line 32-40: This unit test is still constructing the real
HuggingFaceEmbedding through IndexLoader, making it non-hermetic and dependent
on local or network model availability. Update the test around
IndexLoader.vector_indexes to patch HuggingFaceEmbedding the same way as in
tests/unit/utils/test_config.py, then assert the mock was called with model_name
set to all-mpnet-base-v2 instead of inspecting a real _embed_model instance.
---
Nitpick comments:
In `@ols/utils/config.py`:
- Around line 181-187: The BYOK embedding setup currently creates separate
HuggingFaceEmbedding instances in both the tools_rag and skills_rag paths,
duplicating load time and memory. Move the model creation behind one shared
cached helper/property in config.py, and have both paths reuse that same cached
instance instead of instantiating HuggingFaceEmbedding directly. Use the
existing embed_model setup and constants.EMBEDDINGS_MODEL_BYOK_SUBDIR as the
single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 655c8f9e-ab29-47ef-84d3-cf15fb669eca
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (28)
Containerfileartifacts.lock.yamldocs/ai/config.mdexamples/olsconfig.yamlols/app/endpoints/ols.pyols/app/models/config.pyols/constants.pyols/src/prompts/prompt_generator.pyols/src/prompts/prompts.pyols/src/query_helpers/docs_summarizer.pyols/src/rag/hybrid_rag.pyols/src/rag/stop_words.pyols/src/rag_index/index_loader.pyols/src/rag_index/solr_support.pyols/utils/config.pyols/utils/environments.pypyproject.tomlrunner.pytests/e2e/utils/ols_installer.pytests/unit/app/endpoints/test_health.pytests/unit/app/models/test_config.pytests/unit/config_status/test_config_status.pytests/unit/prompts/test_prompt_generator.pytests/unit/query_helpers/test_docs_summarizer.pytests/unit/rag_index/test_index_loader.pytests/unit/rag_index/test_solr_support.pytests/unit/utils/test_config.pytests/unit/utils/test_environments.py
💤 Files with no reviewable changes (2)
- tests/e2e/utils/ols_installer.py
- tests/unit/config_status/test_config_status.py
✅ Files skipped from review due to trivial changes (1)
- docs/ai/config.md
🚧 Files skipped from review as they are similar to previous changes (10)
- pyproject.toml
- tests/unit/prompts/test_prompt_generator.py
- ols/app/endpoints/ols.py
- ols/src/rag/stop_words.py
- ols/src/prompts/prompts.py
- ols/constants.py
- ols/src/rag/hybrid_rag.py
- tests/unit/app/endpoints/test_health.py
- ols/src/prompts/prompt_generator.py
- ols/app/models/config.py
| tools = await get_mcp_tools( | ||
| mcp_tools_query, self.user_token, self.client_headers | ||
| ) | ||
| if self._solr_hybrid is not None and self._solr_client is not None: | ||
| tools.append(get_openshift_docs_tool(self._solr_client)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid adding a second tool with the same name.
get_mcp_tools(..., deduplicate=True) only dedupes MCP results. If an MCP server already exposes search_openshift_documentation, this appends a second tool with the same name and makes name-based tool resolution ambiguous.
Suggested fix
tools = await get_mcp_tools(
mcp_tools_query, self.user_token, self.client_headers
)
if self._solr_hybrid is not None and self._solr_client is not None:
- tools.append(get_openshift_docs_tool(self._solr_client))
+ if all(tool.name != "search_openshift_documentation" for tool in tools):
+ tools.append(get_openshift_docs_tool(self._solr_client))
return tools📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tools = await get_mcp_tools( | |
| mcp_tools_query, self.user_token, self.client_headers | |
| ) | |
| if self._solr_hybrid is not None and self._solr_client is not None: | |
| tools.append(get_openshift_docs_tool(self._solr_client)) | |
| tools = await get_mcp_tools( | |
| mcp_tools_query, self.user_token, self.client_headers | |
| ) | |
| if self._solr_hybrid is not None and self._solr_client is not None: | |
| if all(tool.name != "search_openshift_documentation" for tool in tools): | |
| tools.append(get_openshift_docs_tool(self._solr_client)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/src/query_helpers/docs_summarizer.py` around lines 137 - 141, The
docs_summarizer tool assembly is adding a duplicate tool name when
`get_openshift_docs_tool` is appended after `get_mcp_tools`. Update the
`get_mcp_tools`/`get_openshift_docs_tool` flow in `DocsSummarizer` so
`search_openshift_documentation` is only added if no existing tool with that
name is already present, or otherwise skip the local tool when MCP already
provides it. Use the unique symbols `get_mcp_tools`, `get_openshift_docs_tool`,
and `tools.append(...)` to locate and guard this merge logic.
dfb71a2 to
e09d4fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/rag_index/test_index_loader.py (1)
31-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the embed-model constructor args, not just identity.
The test verifies
_embed_model is mock_embedbut never checks thatHuggingFaceEmbeddingwas invoked with the expectedmodel_name(EMBEDDINGS_MODEL_BYOK_SUBDIR). A regression that changes the model path passed inIndexLoader._get_embed_modelwould go undetected.♻️ Suggested addition
with ( patch( "ols.src.rag_index.index_loader.HuggingFaceEmbedding", return_value=mock_embed, - ), + ) as mock_hf_embedding, patch.object(il.IndexLoader, "_load_index"), ): index_loader_obj = il.IndexLoader(config.ols_config.reference_content) indexes = index_loader_obj.vector_indexes assert index_loader_obj._embed_model is mock_embed + mock_hf_embedding.assert_called_once_with(model_name=EMBEDDINGS_MODEL_BYOK_SUBDIR) assert indexes is NoneAs per coding guidelines, "Write tests that assert specific values and behaviors rather than only checking that code runs without error."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/rag_index/test_index_loader.py` around lines 31 - 44, The test in test_index_loader should verify the HuggingFaceEmbedding constructor arguments, not only that _embed_model matches the mock. Update the assertions around IndexLoader._get_embed_model / IndexLoader.__init__ to check the patched HuggingFaceEmbedding call received the expected model_name value (EMBEDDINGS_MODEL_BYOK_SUBDIR), while keeping the existing identity check if useful. Use the mocked HuggingFaceEmbedding symbol and IndexLoader to locate the behavior under test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ols/src/rag_index/solr_support.py`:
- Around line 381-388: The exception logging in the Solr search paths is
exposing raw user queries, so update the logging in the relevant search methods
(including the hybrid search flow and the other matching search helper) to avoid
printing the full query text. Replace the query argument in the logger.exception
call with either no query content or a non-reversible fingerprint/identifier,
keeping the existing error context and using the surrounding methods in
solr_support.py to locate both affected blocks.
- Line 373: The SolrHybridSearch-owned AsyncClient is created in the constructor
without any shutdown path, so add lifecycle management for the client created in
SolrHybridSearch. Expose an async cleanup method like aclose() and/or make
SolrHybridSearch usable as an async context manager so the owned
httpx.AsyncClient can be closed deterministically. If the client is injected
instead, ensure ownership is clear and the app closes it through the
SolrHybridSearch/solver integration path rather than relying on config reloads.
---
Nitpick comments:
In `@tests/unit/rag_index/test_index_loader.py`:
- Around line 31-44: The test in test_index_loader should verify the
HuggingFaceEmbedding constructor arguments, not only that _embed_model matches
the mock. Update the assertions around IndexLoader._get_embed_model /
IndexLoader.__init__ to check the patched HuggingFaceEmbedding call received the
expected model_name value (EMBEDDINGS_MODEL_BYOK_SUBDIR), while keeping the
existing identity check if useful. Use the mocked HuggingFaceEmbedding symbol
and IndexLoader to locate the behavior under test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 071a1650-91f9-4059-9fe7-9bc8a23d5b42
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (28)
Containerfileartifacts.lock.yamldocs/ai/config.mdexamples/olsconfig.yamlols/app/endpoints/ols.pyols/app/models/config.pyols/constants.pyols/src/prompts/prompt_generator.pyols/src/prompts/prompts.pyols/src/query_helpers/docs_summarizer.pyols/src/rag/hybrid_rag.pyols/src/rag/stop_words.pyols/src/rag_index/index_loader.pyols/src/rag_index/solr_support.pyols/utils/config.pyols/utils/environments.pypyproject.tomlrunner.pytests/e2e/utils/ols_installer.pytests/unit/app/endpoints/test_health.pytests/unit/app/models/test_config.pytests/unit/config_status/test_config_status.pytests/unit/prompts/test_prompt_generator.pytests/unit/query_helpers/test_docs_summarizer.pytests/unit/rag_index/test_index_loader.pytests/unit/rag_index/test_solr_support.pytests/unit/utils/test_config.pytests/unit/utils/test_environments.py
💤 Files with no reviewable changes (2)
- tests/unit/config_status/test_config_status.py
- tests/e2e/utils/ols_installer.py
✅ Files skipped from review due to trivial changes (1)
- docs/ai/config.md
🚧 Files skipped from review as they are similar to previous changes (21)
- pyproject.toml
- artifacts.lock.yaml
- ols/app/endpoints/ols.py
- ols/src/rag/hybrid_rag.py
- ols/src/prompts/prompt_generator.py
- ols/src/rag/stop_words.py
- ols/src/prompts/prompts.py
- Containerfile
- runner.py
- ols/constants.py
- tests/unit/app/endpoints/test_health.py
- ols/src/query_helpers/docs_summarizer.py
- tests/unit/utils/test_environments.py
- tests/unit/utils/test_config.py
- tests/unit/prompts/test_prompt_generator.py
- ols/utils/config.py
- ols/utils/environments.py
- tests/unit/query_helpers/test_docs_summarizer.py
- ols/src/rag_index/index_loader.py
- tests/unit/app/models/test_config.py
- ols/app/models/config.py
| try: | ||
| return await self._search_impl(query) | ||
| except Exception: | ||
| logger.exception( | ||
| "Solr hybrid search failed for query: %.200s", | ||
| query, | ||
| ) | ||
| return [] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging raw user queries.
These paths log the user’s full search text, which can contain support-case details, identifiers, or pasted secrets. Prefer omitting it or logging a non-reversible fingerprint.
Proposed fix
except Exception:
- logger.exception(
- "Solr hybrid search failed for query: %.200s",
- query,
- )
+ logger.exception("Solr hybrid search failed")
return []
@@
if not hybrid_docs:
- logger.warning("No results (hybrid-search) for: %s", query)
+ logger.warning("No results (hybrid-search)")
elif cfg.hybrid_score_threshold > 0:
logger.warning(
- "No docs above hybrid_score_threshold=%s for: %s",
+ "No docs above hybrid_score_threshold=%s",
cfg.hybrid_score_threshold,
- query,
)Also applies to: 422-429
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ols/src/rag_index/solr_support.py` around lines 381 - 388, The exception
logging in the Solr search paths is exposing raw user queries, so update the
logging in the relevant search methods (including the hybrid search flow and the
other matching search helper) to avoid printing the full query text. Replace the
query argument in the logger.exception call with either no query content or a
non-reversible fingerprint/identifier, keeping the existing error context and
using the surrounding methods in solr_support.py to locate both affected blocks.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ols/utils/config.py (1)
259-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore logger levels after suppressing embedding-load noise.
setLevel(logging.ERROR)permanently overrides the configured logging behavior forsentence_transformersandtransformersafter the Solr client is initialized. If the intent is only to quiet model creation, restore the previous levels afterward.Proposed refactor
- for name in ("sentence_transformers", "transformers"): - logging.getLogger(name).setLevel(logging.ERROR) - self._cached_solr_embed_model = HuggingFaceEmbedding( - model_name=constants.SOLR_HYBRID_EMBEDDING_MODEL_ID - ) + logger_names = ("sentence_transformers", "transformers") + previous_levels = { + name: logging.getLogger(name).level for name in logger_names + } + try: + for name in logger_names: + logging.getLogger(name).setLevel(logging.ERROR) + self._cached_solr_embed_model = HuggingFaceEmbedding( + model_name=constants.SOLR_HYBRID_EMBEDDING_MODEL_ID + ) + finally: + for name, level in previous_levels.items(): + logging.getLogger(name).setLevel(level)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/utils/config.py` around lines 259 - 263, The Solr embedding initialization in config.py permanently changes the logger levels for sentence_transformers and transformers; keep the quieting scoped to HuggingFaceEmbedding creation in the Solr client path. Update the logic around _cached_solr_embed_model so it saves each logger’s previous level before setLevel(logging.ERROR), creates the model, then restores the original levels afterward, preserving the rest of the app’s logging configuration.ols/src/rag_index/solr_support.py (1)
359-373: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse the HTTP client across searches, with an explicit close path.
_search_impl()creates a newhttpx.AsyncClienton every call; keep one onSolrHybridSearchto preserve connection pooling, and addaclose()/lifecycle wiring so the shared client is closed cleanly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ols/src/rag_index/solr_support.py` around lines 359 - 373, `SolrHybridSearch` currently creates a fresh `httpx.AsyncClient` inside `_search_impl()`, which prevents connection pooling. Add a shared client as an instance member initialized in `SolrHybridSearch.__init__`, have `_search_impl()` reuse that client for all searches, and make sure there is an explicit `aclose()` on `SolrHybridSearch` to shut it down cleanly. Wire the lifecycle so any caller that owns the search object closes it when finished.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/rag_index/test_solr_support.py`:
- Around line 4-5: Update the helper functions `_patch_httpx_client` and `_ctx`
in `test_solr_support.py` so their full signatures include explicit type hints
for all parameters and return types, matching the repo’s Python typing
conventions. Also rename the affected test at the current
`test_solr_hybrid_search_*` location to follow the `test_<function>_<scenario>`
pattern used by surrounding tests, keeping the name aligned with the
`test_solr_hybrid_search_...` convention.
---
Nitpick comments:
In `@ols/src/rag_index/solr_support.py`:
- Around line 359-373: `SolrHybridSearch` currently creates a fresh
`httpx.AsyncClient` inside `_search_impl()`, which prevents connection pooling.
Add a shared client as an instance member initialized in
`SolrHybridSearch.__init__`, have `_search_impl()` reuse that client for all
searches, and make sure there is an explicit `aclose()` on `SolrHybridSearch` to
shut it down cleanly. Wire the lifecycle so any caller that owns the search
object closes it when finished.
In `@ols/utils/config.py`:
- Around line 259-263: The Solr embedding initialization in config.py
permanently changes the logger levels for sentence_transformers and
transformers; keep the quieting scoped to HuggingFaceEmbedding creation in the
Solr client path. Update the logic around _cached_solr_embed_model so it saves
each logger’s previous level before setLevel(logging.ERROR), creates the model,
then restores the original levels afterward, preserving the rest of the app’s
logging configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 97e3a38e-3a40-4e0f-af45-1062f9c47e00
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (28)
Containerfileartifacts.lock.yamldocs/ai/config.mdexamples/olsconfig.yamlols/app/endpoints/ols.pyols/app/models/config.pyols/constants.pyols/src/prompts/prompt_generator.pyols/src/prompts/prompts.pyols/src/query_helpers/docs_summarizer.pyols/src/rag/hybrid_rag.pyols/src/rag/stop_words.pyols/src/rag_index/index_loader.pyols/src/rag_index/solr_support.pyols/utils/config.pyols/utils/environments.pypyproject.tomlrunner.pytests/e2e/utils/ols_installer.pytests/unit/app/endpoints/test_health.pytests/unit/app/models/test_config.pytests/unit/config_status/test_config_status.pytests/unit/prompts/test_prompt_generator.pytests/unit/query_helpers/test_docs_summarizer.pytests/unit/rag_index/test_index_loader.pytests/unit/rag_index/test_solr_support.pytests/unit/utils/test_config.pytests/unit/utils/test_environments.py
💤 Files with no reviewable changes (2)
- tests/unit/config_status/test_config_status.py
- tests/e2e/utils/ols_installer.py
✅ Files skipped from review due to trivial changes (2)
- docs/ai/config.md
- artifacts.lock.yaml
🚧 Files skipped from review as they are similar to previous changes (21)
- ols/src/rag/stop_words.py
- ols/src/rag/hybrid_rag.py
- pyproject.toml
- runner.py
- ols/src/prompts/prompt_generator.py
- ols/src/prompts/prompts.py
- ols/app/endpoints/ols.py
- ols/constants.py
- tests/unit/rag_index/test_index_loader.py
- tests/unit/app/endpoints/test_health.py
- tests/unit/prompts/test_prompt_generator.py
- Containerfile
- ols/src/rag_index/index_loader.py
- examples/olsconfig.yaml
- ols/utils/environments.py
- ols/src/query_helpers/docs_summarizer.py
- tests/unit/utils/test_config.py
- tests/unit/utils/test_environments.py
- tests/unit/query_helpers/test_docs_summarizer.py
- tests/unit/app/models/test_config.py
- ols/app/models/config.py
| from contextlib import asynccontextmanager | ||
| from typing import Any |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the helper signatures and test name with repo conventions.
_patch_httpx_client/_ctx are missing full signature type hints, and Line 285 should follow the surrounding test_solr_hybrid_search_<scenario> naming pattern.
Proposed cleanup
+from collections.abc import AsyncIterator, Awaitable, Callable
from contextlib import asynccontextmanager
from typing import Any
@@
-def _patch_httpx_client(fake_post):
+def _patch_httpx_client(fake_post: Callable[..., Awaitable[Any]]) -> Any:
"""Patch ``httpx.AsyncClient`` so the context manager yields a mock with *fake_post*."""
mock_client = AsyncMock()
mock_client.post = fake_post
`@asynccontextmanager`
- async def _ctx(*args, **kwargs):
+ async def _ctx(*args: Any, **kwargs: Any) -> AsyncIterator[AsyncMock]:
yield mock_client
@@
-async def test_solr_search_returns_empty_when_embedding_raises() -> None:
+async def test_solr_hybrid_search_returns_empty_when_embedding_raises() -> None:As per coding guidelines, **/*.py: “Use type hints for all function signatures,” and tests/**/*.py: “Follow the test naming convention test_<function>_<scenario>.”
Also applies to: 32-39, 284-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/rag_index/test_solr_support.py` around lines 4 - 5, Update the
helper functions `_patch_httpx_client` and `_ctx` in `test_solr_support.py` so
their full signatures include explicit type hints for all parameters and return
types, matching the repo’s Python typing conventions. Also rename the affected
test at the current `test_solr_hybrid_search_*` location to follow the
`test_<function>_<scenario>` pattern used by surrounding tests, keeping the name
aligned with the `test_solr_hybrid_search_...` convention.
Source: Coding guidelines
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
vimalk78
left a comment
vimalk78
left a comment
There was a problem hiding this comment.
Review — OLS-3310 (jira mode, round 1)
Score: 84/100 · 4/4 Jira ACs pass · 3 code quality findings (1 must-fix, 1 should-fix, 1 nice-to-have)
Adherence
All four acceptance criteria from OLS-3310 are met:
- OCP docs retrieval only via tool — Solr hybrid retrieval is exposed exclusively through
search_openshift_documentationStructuredTool. - Direct-RAG code path removed — no
solr_direct_ragconfig or code path exists. - BYOK FAISS unchanged —
ReferenceContent.indexes,IndexLoader, and the BYOK retrieval path are preserved. - Unit tests updated — 491 new lines in
test_solr_support.py, plus updates to config, summarizer, prompt, index loader, health, and environment tests.
Code quality issues below as inline comments.
| doc.get("chunk_index", "?"), | ||
| 1, | ||
| len(ordered), | ||
| len(family) if per_chunk_budget > 0 else 0, |
There was a problem hiding this comment.
Bug (must-fix): NameError when max_expansion_neighbors=0 and token_budget>0.
The debug log guards len(family) with per_chunk_budget > 0, but family is only assigned inside the if per_chunk_budget > 0 and cfg.max_expansion_neighbors > 0 block. When a user sets max_expansion_neighbors: 0 (valid — documented as "disables expansion") and the tool loop provides a token budget, the first iteration hits NameError: name 'family' is not defined. On later iterations it silently uses stale data from the previous doc.
Fix: either initialize family = [] before the loop, or change the guard to match the assignment condition:
len(family) if (per_chunk_budget > 0 and cfg.max_expansion_neighbors > 0) else 0,There was a problem hiding this comment.
Fixed in 99ba1e1 — the debug guard now matches the assignment condition (per_chunk_budget > 0 and cfg.max_expansion_neighbors > 0), and a unit test covers max_expansion_neighbors=0 with a non-zero token budget.
| query_embedding = await asyncio.to_thread(_encode) | ||
| vector_str = "[" + ",".join(str(v) for v in query_embedding) + "]" | ||
| form = self._build_hybrid_form(cleaned=cleaned, vector_str=vector_str) | ||
| async with httpx.AsyncClient(timeout=cfg.hybrid_solr_timeout_s) as client: |
There was a problem hiding this comment.
Should-fix: httpx.AsyncClient is created fresh per search() call — no connection pooling across invocations.
The PR description says "reused httpx.AsyncClient for connection pooling" but only intra-call reuse (hybrid POST + family GETs within one search) happens. Each tool invocation gets a new TCP connection to Solr. For production traffic this causes unnecessary connection churn.
Consider storing a persistent httpx.AsyncClient on SolrHybridSearch (with proper lifecycle via an aclose() hook or startup/shutdown events).
There was a problem hiding this comment.
Fixed in 99ba1e1 — SolrHybridSearch now holds a persistent httpx.AsyncClient via _get_http_client(), with aclose() / close_http_client_sync() called on config reload. Unit test confirms the client is reused across search() calls. Verified locally with 5 concurrent /v1/query requests against RHOKP.
| "rank-bm25>=0.2.2", # For ToolsRAG sparse retrieval | ||
| "python-frontmatter>=1.1.0", # For parsing skill YAML frontmatter | ||
| "opentelemetry-api>=1.20.0", # For audit tracing | ||
| "opentelemetry-api>=1.30.0", |
There was a problem hiding this comment.
Nice-to-have: Unrelated dependency change — opentelemetry-api floor bumped from >=1.20.0 to >=1.30.0. Keeping changes scoped to the feature helps reviewers and bisection. Consider splitting this into its own commit or PR.
There was a problem hiding this comment.
Fixed in 99ba1e1 — reverted opentelemetry-api floor back to >=1.20.0 and regenerated uv.lock.
|
@vimalk78 All review items addressed in 99ba1e1:
|
blublinsky
force-pushed
the
okp-rag
branch
2 times, most recently
from
66aad53 to
f31a029
Compare
|
/lgtm |
|
@blublinsky: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/lgtm |
3 participants
Description
Summary
Adds optional Solr hybrid retrieval against the OKP portal-rag
hybrid-searchendpoint, exposed as a
search_openshift_documentationLangChain tool.When
ols_config.solr_hybridis present in the config, the model can invokethe tool to search published Red Hat product documentation. Solr failures
degrade to empty passages instead of failing the request.
Embedding models (
sentence-transformers/all-mpnet-base-v2for BYOK/tools/skillsRAG,
ibm-granite/granite-embedding-30m-englishfor OKP Solr) are bundled inthe service image via Cachi2 and loaded offline — no runtime downloads.
BYOK FAISS RAG is preserved and coexists with the Solr tool path.
Key changes
SolrHybridSettingsconfig model (solr_http_base,max_results,hybrid_pool_docs,hybrid_vector_boost,hybrid_score_threshold,chunk_filter_query,hybrid_solr_timeout_s)SolrHybridSearchasync client with reusedhttpx.AsyncClientforconnection pooling
search_openshift_documentationasyncStructuredToolfor agent tool loopContainerfile+artifacts.lock.yamlupdated for both embedding modelsType of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Unit tests (
make test-unit) and integration tests (make test-integration)pass.
make verifyclean (only pre-existingwokefinding onorigin/main).Local validation:
OLS_CONFIG_FILEwithsolr_hybrid.solr_http_basepointing at anOKP portal-rag instance
search_openshift_documentationand citesdocs_urlfrom resultssolr_hybridsection — BYOK FAISS RAG works as beforeSummary by CodeRabbit
solr_hybrid) withsolr_http_base, retrieval/reranking controls, and a fixed Granite embedding model for vector consistency.solr_hybridYAML structure and example/migration guidance.