LCORE-2443: Simplify OKP RAG setup with git submodules and auto-enrichment#1885
Conversation
…hment Rewire OKP installation for new container set up. As a byproduct, replace manual provider installation with git submodules. Users no longer need to clone lightspeed-providers separately or set EXTERNAL_PROVIDERS_DIR. Changes: - Add lightspeed-providers as submodule - Bake providers into container via PYTHONPATH - Automatic llama-stack config enrichment - users no longer have to worry about llama-stack - Update CI/CD to fetch submodules - Simplify OKP guide documentation Setup: clone with --recursive, configure okp section, run make run. Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
WalkthroughThis PR integrates the ChangesLightspeed Providers Submodule Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/okp_guide.md (1)
325-325:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale step reference.
Line 325 references "Step 4" but should reference "Step 3" (where the
rag.inline/rag.toolconfiguration is documented).📝 Suggested fix
-2. `lightspeed-stack.yaml` has `okp` under `rag.inline` and/or `rag.tool` as in Step 4 +2. `lightspeed-stack.yaml` has `okp` under `rag.inline` and/or `rag.tool` as in Step 3🤖 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 `@docs/okp_guide.md` at line 325, Update the stale step reference in docs/okp_guide.md: change the mention of "Step 4" to "Step 3" where it says "lightspeed-stack.yaml has `okp` under `rag.inline` and/or `rag.tool` as in Step 4" so it correctly points to the section that documents the `rag.inline` / `rag.tool` configuration; confirm the sentence references `lightspeed-stack.yaml`, `okp`, `rag.inline`, and `rag.tool` exactly and update only the step number.
🤖 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 @.github/workflows/e2e_tests_rhaiis.yaml:
- Around line 54-55: The e2e_tests.yaml workflow is using provider resources
(provider_id / lightspeed-providers) but its actions/checkout step does not
fetch git submodules; update the checkout step for that workflow to include
submodules: 'recursive' (i.e., add the submodules: 'recursive' key to the
actions/checkout step) so lightspeed-providers submodule is available, or
alternatively remove provider usage from this workflow if you prefer not to
fetch submodules.
In `@docs/okp_guide.md`:
- Around line 41-45: Add blank lines before and after the fenced code block
containing the git commands to follow Markdown best practices: ensure there is
an empty line above the opening triple backticks and an empty line after the
closing triple backticks so the block with "git clone --recursive
https://github.com/lightspeed-core/lightspeed-stack.git" and the inline note "If
you already cloned without `--recursive`, run: `git submodule update --init`"
are separated from surrounding list items; update the section that includes the
code block (the lines with the git clone command and the submodule update note)
accordingly.
In `@README.md`:
- Around line 163-176: Add a blank line before the fenced code block that
follows the "If you already cloned without `--recursive`:" line in the README so
the markdown renders correctly; locate the paragraph containing the exact text
"If you already cloned without `--recursive`:" and insert one empty line
immediately before the subsequent "```bash" code fence.
In `@run.yaml`:
- Line 18: The hardcoded external_providers_dir setting in run.yaml points to
/opt/app-root/providers/resources/external_providers which doesn't exist with
the current repo/submodule layout; update the external_providers_dir value (the
external_providers_dir key in run.yaml) to the actual location used by
lightspeed-providers in this checkout (or ensure the providers submodule is
initialized/copied into that path) so the path resolves correctly at runtime;
verify by locating the actual directory name in the lightspeed-providers module
and replacing the value accordingly.
In `@src/llama_stack_configuration.py`:
- Around line 436-441: Update the function docstring to mention that
okp_config['rhokp_url'] (and the local variable rhokp_raw / base_url_raw)
supports ${env.VAR} expansion via replace_env_vars before constructing base_url
and solr_url; briefly describe the expansion behavior, give a short example like
"${env.RH_SERVER_OKP}" being replaced from the environment, and note that
replace_env_vars is applied to base_url_raw prior to urljoin to form solr_url.
---
Outside diff comments:
In `@docs/okp_guide.md`:
- Line 325: Update the stale step reference in docs/okp_guide.md: change the
mention of "Step 4" to "Step 3" where it says "lightspeed-stack.yaml has `okp`
under `rag.inline` and/or `rag.tool` as in Step 4" so it correctly points to the
section that documents the `rag.inline` / `rag.tool` configuration; confirm the
sentence references `lightspeed-stack.yaml`, `okp`, `rag.inline`, and `rag.tool`
exactly and update only the step number.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9a2de0d2-5432-4ddd-82be-29680a1feeca
📒 Files selected for processing (16)
.github/workflows/build_and_push_dev.yaml.github/workflows/build_and_push_release.yaml.github/workflows/build_pr.yaml.github/workflows/e2e_tests.yaml.github/workflows/e2e_tests_lightspeed_evaluation.yaml.github/workflows/e2e_tests_providers.yaml.github/workflows/e2e_tests_rhaiis.yaml.github/workflows/e2e_tests_rhelai.yaml.gitmodulesMakefileREADME.mddeploy/llama-stack/test.containerfiledocs/okp_guide.mdprovidersrun.yamlsrc/llama_stack_configuration.py
💤 Files with no reviewable changes (1)
- Makefile
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-pr
- GitHub Check: Pylinter
- GitHub Check: unit_tests (3.12)
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Llama Stack imports: Usefrom llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules must start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
All functions must have complete type annotations for parameters and return types, use modern syntax (str | int), and include descriptive docstrings
Use snake_case with descriptive, action-oriented names for functions (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying function parameters
Useasync deffor I/O operations and external API calls
Use standard log levels with clear purposes:debug()for diagnostic info,info()for program execution,warning()for unexpected events,error()for serious problems
All classes must have descriptive docstrings explaining purpose and use PascalCase with standard suffixes:Configuration,Error/Exception,Resolver,Interface
Abstract classes must use ABC with@abstractmethoddecorators
Follow Google Python docstring conventions with required sections: Parameters, Returns, Raises, and Attributes for classes
Files:
src/llama_stack_configuration.py
🧠 Learnings (5)
📚 Learning: 2026-04-15T18:53:54.785Z
Learnt from: Lifto
Repo: lightspeed-core/lightspeed-stack PR: 1510
File: src/models/requests.py:742-769
Timestamp: 2026-04-15T18:53:54.785Z
Learning: In `src/models/requests.py`, keep `ResponsesRequest.validate_body_size` using `json.dumps(values)` with Python defaults (including ASCII escaping and the default separators). This default formatting is intentional to make the 65,536-character limit conservatively strict (accounting for small encoding/format overhead). Do not recommend changing it to `ensure_ascii=False` or using compact separators for this validator, since an exact wire-format byte match with the client payload is not achievable via `json.dumps` anyway.
Applied to files:
.gitmodules
📚 Learning: 2026-04-20T15:09:45.119Z
Learnt from: major
Repo: lightspeed-core/lightspeed-stack PR: 1548
File: src/app/endpoints/rlsapi_v1.py:56-56
Timestamp: 2026-04-20T15:09:45.119Z
Learning: In `src/app/endpoints/rlsapi_v1.py`, treat the line `_get_rh_identity_context = get_rh_identity_context` as an intentional temporary backward-compatibility shim (introduced for PR `#1548`, Splunk HEC telemetry work). Do not flag it as dead/unnecessary/unused code during review until the planned part 3 is merged and the responses endpoint is fully wired up such that no tests or external consumers reference the underscore-prefixed name.
Applied to files:
.gitmodules
📚 Learning: 2025-12-18T10:21:03.056Z
Learnt from: are-ces
Repo: lightspeed-core/lightspeed-stack PR: 935
File: run.yaml:114-115
Timestamp: 2025-12-18T10:21:03.056Z
Learning: In run.yaml for Llama Stack 0.3.x, do not add a telemetry provider block under providers. To enable telemetry, set telemetry.enabled: true directly (no provider block required). This pattern applies specifically to run.yaml configuration in this repository.
Applied to files:
run.yaml
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
run.yaml
📚 Learning: 2026-05-06T08:35:54.687Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 1690
File: .github/workflows/e2e_tests_providers.yaml:279-285
Timestamp: 2026-05-06T08:35:54.687Z
Learning: In .github/workflows/e2e_tests_providers.yaml and related e2e workflow files, the show_logs step should not use docker compose logs with --tail or --since (i.e., keep logs unbounded). The quick connectivity test runs once immediately after container startup, so the log output is small and a log tail limit is unnecessary. If you adjust this, add a rationale comment in the workflow explaining why unbounded logs are acceptable and ensure CI behavior remains deterministic.
Applied to files:
.github/workflows/e2e_tests.yaml.github/workflows/e2e_tests_lightspeed_evaluation.yaml.github/workflows/build_pr.yaml.github/workflows/e2e_tests_providers.yaml.github/workflows/build_and_push_dev.yaml.github/workflows/build_and_push_release.yaml.github/workflows/e2e_tests_rhaiis.yaml.github/workflows/e2e_tests_rhelai.yaml
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 173-173: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
docs/okp_guide.md
[warning] 42-42: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 44-44: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 zizmor (1.25.2)
.github/workflows/build_pr.yaml
[warning] 25-29: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
.github/workflows/build_and_push_dev.yaml
[warning] 27-31: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
.github/workflows/build_and_push_release.yaml
[warning] 29-33: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🔇 Additional comments (15)
.gitmodules (1)
1-3: LGTM!.github/workflows/build_and_push_dev.yaml (1)
29-31: LGTM!.github/workflows/build_and_push_release.yaml (1)
31-33: LGTM!.github/workflows/build_pr.yaml (1)
27-29: LGTM!.github/workflows/e2e_tests.yaml (1)
40-41: LGTM!.github/workflows/e2e_tests_lightspeed_evaluation.yaml (1)
33-34: LGTM!.github/workflows/e2e_tests_providers.yaml (1)
65-66: LGTM!.github/workflows/e2e_tests_rhaiis.yaml (1)
54-55: LGTM!.github/workflows/e2e_tests_rhelai.yaml (1)
48-49: LGTM!providers (1)
1-1: LGTM!deploy/llama-stack/test.containerfile (2)
19-22: LGTM!
25-27: LGTM!README.md (1)
177-183: LGTM!docs/okp_guide.md (2)
21-24: LGTM!
74-77: LGTM!Also applies to: 80-99
| # Fetch submodules (required for lightspeed-providers) | ||
| submodules: 'recursive' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all GitHub Actions workflows and check which ones use checkout but don't fetch submodules
echo "=== Workflows with actions/checkout ==="
rg -l 'uses: actions/checkout' .github/workflows/
echo ""
echo "=== Workflows with actions/checkout that already have submodules config ==="
rg -l "submodules: 'recursive'" .github/workflows/
echo ""
echo "=== Workflows with actions/checkout but NO submodules config ==="
# Find workflows with checkout
WORKFLOWS_WITH_CHECKOUT=$(rg -l 'uses: actions/checkout' .github/workflows/)
for workflow in $WORKFLOWS_WITH_CHECKOUT; do
if ! grep -q "submodules: 'recursive'" "$workflow"; then
echo "$workflow"
echo " Context:"
rg -C2 'uses: actions/checkout' "$workflow" | head -20
echo ""
fi
doneRepository: lightspeed-core/lightspeed-stack
Length of output: 3839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Workflows referencing providers or lightspeed-providers ==="
rg -n --no-heading -S "lightspeed-providers|providers" .github/workflows/*.yaml .github/workflows/*.yml
echo ""
echo "=== Correlate those workflows with checkout submodules config ==="
WORKFLOWS=$(rg -l -S "lightspeed-providers|providers" .github/workflows/*.yaml .github/workflows/*.yml || true)
for w in $WORKFLOWS; do
echo "--- $w"
if rg -q "submodules:\s*'recursive'" "$w"; then
echo " checkout submodules: recursive (present)"
else
echo " checkout submodules: recursive (MISSING)"
fi
rg -n --no-heading -S "lightspeed-providers|providers" "$w" | head -n 20
doneRepository: lightspeed-core/lightspeed-stack
Length of output: 1428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
WF_DIR=".github/workflows"
echo "=== Provider-related workflows ==="
WORKFLOWS=$(rg -l -S "lightspeed-providers|provider_id" "$WF_DIR"/*.yaml)
for w in $WORKFLOWS; do
echo "--- $w"
if rg -q "submodules:\s*'recursive'" "$w"; then
echo " checkout submodules: recursive (present)"
else
echo " checkout submodules: recursive (MISSING)"
fi
doneRepository: lightspeed-core/lightspeed-stack
Length of output: 818
Fix missing submodule fetching in provider-reliant workflow(s)
All provider-related workflows set actions/checkout with submodules: 'recursive' (including .github/workflows/e2e_tests_rhelai.yaml, .github/workflows/e2e_tests_rhaiis.yaml, .github/workflows/e2e_tests_providers.yaml, .github/workflows/e2e_tests_lightspeed_evaluation.yaml, .github/workflows/build_pr.yaml, .github/workflows/build_and_push_dev.yaml, .github/workflows/build_and_push_release.yaml).
.github/workflows/e2e_tests.yaml references providers (provider_id/lightspeed-providers) but its checkout step does not include submodules: 'recursive'; update it to fetch submodules (or remove provider usage from this workflow when submodules aren’t fetched).
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 41-55: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/e2e_tests_rhaiis.yaml around lines 54 - 55, The
e2e_tests.yaml workflow is using provider resources (provider_id /
lightspeed-providers) but its actions/checkout step does not fetch git
submodules; update the checkout step for that workflow to include submodules:
'recursive' (i.e., add the submodules: 'recursive' key to the actions/checkout
step) so lightspeed-providers submodule is available, or alternatively remove
provider usage from this workflow if you prefer not to fetch submodules.
| * [lightspeed-stack repository](https://github.com/lightspeed-core/lightspeed-stack) cloned **with submodules**: | ||
| ```bash | ||
| git clone --recursive https://github.com/lightspeed-core/lightspeed-stack.git | ||
| ``` | ||
| If you already cloned without `--recursive`, run: `git submodule update --init` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor markdown formatting: Add blank lines around code blocks.
Markdown best practice suggests adding blank lines before and after the fenced code blocks at lines 42-43 and line 45.
📝 Suggested formatting fix
* [lightspeed-stack repository](https://github.com/lightspeed-core/lightspeed-stack) cloned **with submodules**:
+
```bash
git clone --recursive https://github.com/lightspeed-core/lightspeed-stack.git- If you already cloned without
--recursive, run:git submodule update --init
- Podman (or Docker) to run the OKP image
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
* [lightspeed-stack repository](https://github.com/lightspeed-core/lightspeed-stack) cloned **with submodules**:
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 42-42: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 44-44: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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 `@docs/okp_guide.md` around lines 41 - 45, Add blank lines before and after the
fenced code block containing the git commands to follow Markdown best practices:
ensure there is an empty line above the opening triple backticks and an empty
line after the closing triple backticks so the block with "git clone --recursive
https://github.com/lightspeed-core/lightspeed-stack.git" and the inline note "If
you already cloned without `--recursive`, run: `git submodule update --init`"
are separated from surrounding list items; update the section that includes the
code block (the lines with the git clone command and the submodule update note)
accordingly.
Source: Linters/SAST tools
| ## Clone the Repository | ||
|
|
||
| Lightspeed Stack uses a git submodule for external providers (required for features like OKP RAG). Clone with submodules: | ||
|
|
||
| ```bash | ||
| git clone --recursive https://github.com/lightspeed-core/lightspeed-stack.git | ||
| cd lightspeed-stack | ||
| ``` | ||
|
|
||
| **If you already cloned without `--recursive`:** | ||
| ```bash | ||
| git submodule update --init | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor markdown formatting: Add blank line before code block.
Markdown best practice suggests adding a blank line before the fenced code block at line 173 (before the "If you already cloned" command).
📝 Suggested formatting fix
cd lightspeed-stackIf you already cloned without --recursive:
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 173-173: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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 `@README.md` around lines 163 - 176, Add a blank line before the fenced code
block that follows the "If you already cloned without `--recursive`:" line in
the README so the markdown renders correctly; locate the paragraph containing
the exact text "If you already cloned without `--recursive`:" and insert one
empty line immediately before the subsequent "```bash" code fence.
Source: Linters/SAST tools
| datasets: [] | ||
| image_name: starter | ||
| external_providers_dir: ${env.EXTERNAL_PROVIDERS_DIR} | ||
| external_providers_dir: /opt/app-root/providers/resources/external_providers |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the submodule directory structure contains the expected path
# Check if providers directory exists and has the expected structure
if [ -d "providers" ]; then
echo "=== Providers directory structure ==="
tree -L 3 providers/ 2>/dev/null || find providers/ -maxdepth 3 -type d
echo ""
echo "=== Checking for resources/external_providers ==="
if [ -d "providers/resources/external_providers" ]; then
echo "✓ Found: providers/resources/external_providers"
ls -la providers/resources/external_providers/
else
echo "✗ NOT FOUND: providers/resources/external_providers"
fi
else
echo "✗ providers directory not found (submodule may not be initialized)"
fiRepository: lightspeed-core/lightspeed-stack
Length of output: 252
Fix: hardcoded external_providers_dir path not supported by current repo/submodule layout.
The expected submodule directory providers/resources/external_providers is not present in lightspeed-core/lightspeed-stack (the providers/ directory is empty in this checkout), so /opt/app-root/providers/resources/external_providers can’t be confirmed to exist. Update the path to match the actual lightspeed-providers contents (or ensure the submodule is initialized/copied into that location).
🤖 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 `@run.yaml` at line 18, The hardcoded external_providers_dir setting in
run.yaml points to /opt/app-root/providers/resources/external_providers which
doesn't exist with the current repo/submodule layout; update the
external_providers_dir value (the external_providers_dir key in run.yaml) to the
actual location used by lightspeed-providers in this checkout (or ensure the
providers submodule is initialized/copied into that path) so the path resolves
correctly at runtime; verify by locating the actual directory name in the
lightspeed-providers module and replacing the value accordingly.
| base_url_raw = ( | ||
| str(rhokp_raw) if rhokp_raw is not None else constants.RH_SERVER_OKP_DEFAULT_URL | ||
| ) | ||
| # Resolve environment variables in the URL (e.g., ${env.RH_SERVER_OKP}) | ||
| base_url = replace_env_vars(base_url_raw) | ||
| solr_url = urljoin(base_url, "/solr") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider documenting environment variable resolution in the docstring.
The new environment variable resolution logic (lines 439-440) is an important behavior change. Consider updating the function docstring (lines 409-419) to document that okp_config['rhokp_url'] values containing ${env.*} expressions will be expanded before constructing the Solr URL.
📝 Suggested docstring update
Args:
ls_config: Llama Stack configuration dict (modified in place)
rag_config: RAG configuration dict. Used keys:
- inline (list[str]): inline RAG IDs
- tool (list[str]): tool RAG IDs
okp_config: OKP configuration dict. Used keys:
- chunk_filter_query (str): Solr filter query for chunk retrieval
- - rhokp_url (str): OKP/Solr base URL (e.g. from ${env.RH_SERVER_OKP})
+ - rhokp_url (str): OKP/Solr base URL. Environment variable
+ expressions (e.g., ${env.RH_SERVER_OKP}) are resolved before
+ constructing the final Solr URL.🤖 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 `@src/llama_stack_configuration.py` around lines 436 - 441, Update the function
docstring to mention that okp_config['rhokp_url'] (and the local variable
rhokp_raw / base_url_raw) supports ${env.VAR} expansion via replace_env_vars
before constructing base_url and solr_url; briefly describe the expansion
behavior, give a short example like "${env.RH_SERVER_OKP}" being replaced from
the environment, and note that replace_env_vars is applied to base_url_raw prior
to urljoin to form solr_url.
Jazzcort
left a comment
Jazzcort
left a comment
There was a problem hiding this comment.
Like what I mentioned in the chat. If we decide to pull the providers for our users instead of having them setting these up, hardcoding the directory in run.yaml seems like a right move. But we need to clean up those references for EXTERNAL_PROPVIDER_DIR from all of the docs so it won't cause the confusion in the future.
Here is the screenshot of those reference I found. 😁
Everything else looks good to me! We can always fix the workflow if it breaks but I think it will be fine.
| str(rhokp_raw) if rhokp_raw is not None else constants.RH_SERVER_OKP_DEFAULT_URL | ||
| ) | ||
| # Resolve environment variables in the URL (e.g., ${env.RH_SERVER_OKP}) | ||
| base_url = replace_env_vars(base_url_raw) |
There was a problem hiding this comment.
Was the default value syntax (${env.FOO:=default}) working before for EXTERNAL_PROVIDER_DIR? Seems like it would not support that syntax without calling replace_env_vars(...) 😆
There was a problem hiding this comment.
Great catch 😄 ${env.EXTERNAL_PROVIDERS_DIR} in run.yaml was not being resolved by the enrichment script - llama-stack itself was resolving it. The enrichment script never touched that variable, just passed it directly to llama-stack, which in turn was reading the enriched run.yaml and llama-stack's own config parser was resolving all ${env.Var} patterns.
So without replace_env_vars(), we'd be doing:
base_url = "${env.RH_SERVER_OKP}" # literal string, not resolved!
solr_url = urljoin("${env.RH_SERVER_OKP}", "/solr")
# Result: solr_url = "${env.RH_SERVER_OKP}/solr" (malformed)
So yes, the default value syntax ${env.FOO:=default} wouldn't have worked in the enrichment script before replace_env_vars() was added. It only worked for values that passed straight through to llama-stack's parser! 😆
There was a problem hiding this comment.
Thanks for the deep dive here! I was just suspecting because I saw the default syntax was using on EXTERNAL_PROVIDERS_DIR somewhere. 😆
Thanks for the review @Jazzcort and good catch. PTAL at eb9e5c9 Files Updated:
Remaining References:
|
|
|
||
| # Fetch submodules (required for lightspeed-providers) | ||
| submodules: 'recursive' | ||
|
|
There was a problem hiding this comment.
why are we changing e2e workflow jobs in this PR?
There was a problem hiding this comment.
We're introducing a new way of pulling in the providers repo - e2e jobs need to adopt that
There was a problem hiding this comment.
Also see the description of the PR that has more info
There was a problem hiding this comment.
Wasn't the end goal to remove providers?
There was a problem hiding this comment.
there doesn't seem to be an issue or epic associated with this PR. Where is the motivation coming from?
There was a problem hiding this comment.
A git submodule would make lightspeed-stack depend on a specific commit of lightspeed-providers, not automatically on “latest main” right?
There was a problem hiding this comment.
there doesn't seem to be an issue or epic associated with this PR.
Isn't LCORE-2443 the associated issue, and its parent, the epic?
Where is the motivation coming from?
You're in the #team-collab-okp-lcore channel right? https://redhat-internal.slack.com/archives/C092RKNEQ81/p1780415148953239?thread_ts=1780406932.917899&cid=C092RKNEQ81
A git submodule would make lightspeed-stack depend on a specific commit of lightspeed-providers, not automatically on “latest main” right?
A quick google search would land you to https://git-scm.com/book/en/v2/Git-Tools-Submodules - which would give you the below info of how it works:
- Initial setup: When you add the submodule with git submodule add, lightspeed-stack records the current commit SHA of lightspeed-providers in its tree
- Cloning behavior: When someone clones lightspeed-stack and runs git submodule update --init, they get that exact pinned commit, NOT the latest from lightspeed-providers
- Updating the submodule: To use a newer version of lightspeed-providers, you must explicitly:
cd providers/
git pull origin main # or checkout specific commit
cd ..
git add providers/
git commit -m "Update providers submodule to commit abc123"
You can check which commit is currently pinned with:
$ git ls-tree HEAD providers/
160000 commit 9ab872996b1b95448a9332bc3b468fe27b4ee2e6 providers
There was a problem hiding this comment.
Btw, yes that means manual maintenance is required - need to periodically update the submodule reference.
If forgotten, it could lead to staleness and we could be using outdated providers till someone remembers.
4 participants
Description
Rewire OKP installation for new container set up. As a byproduct, replace manual provider installation with git submodules. Users no longer need to
Changes:
Setup: clone with --recursive, configure okp section, run make run.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes