LCORE-1335: Parametrized RHAIIS port and added library mode tests#1164
LCORE-1335: Parametrized RHAIIS port and added library mode tests#1164tisnik merged 1 commit intolightspeed-core:mainfrom
Conversation
WalkthroughThe changes introduce matrix-based E2E testing with server/library deployment modes, dynamic configuration loading from mode-specific directories, and RHAIIS_PORT environment variable injection across Docker Compose files and CI workflows for configurable E2E deployment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
docker-compose.yaml (1)
46-46: Inconsistent default handling forRHAIIS_PORTacross compose files.In this file,
RHAIIS_PORThas no default (${RHAIIS_PORT}), whiledocker-compose-library.yamluses${RHAIIS_PORT:-}. This is consistent with how other RHAIIS variables are handled in each respective file, so likely intentional — but worth confirming that server-mode CI always setsRHAIIS_PORTbeforedocker compose up, as an unset variable here will be injected as an empty string (not cause a failure).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` at line 46, The compose file uses an unset expansion for RHAIIS_PORT ("RHAIIS_PORT=${RHAIIS_PORT}") that is inconsistent with docker-compose-library.yaml which uses the "${RHAIIS_PORT:-}" form; update the variable in docker-compose.yaml to use the same expansion (replace RHAIIS_PORT=${RHAIIS_PORT} with RHAIIS_PORT=${RHAIIS_PORT:-}) so default handling is consistent, or alternatively ensure CI/server-mode always sets RHAIIS_PORT before invoking docker compose; target the RHAIIS_PORT entry in the service environment block to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/e2e_tests_rhaiis.yaml:
- Line 112: The curl connectivity check currently uses unquoted variable
expansion which can cause shell word-splitting when RHAIIS_URL contains special
characters; update the command to quote the URL expansion (e.g., replace curl -f
${RHAIIS_URL}:${RHAIIS_PORT}/v1/models with curl -f
"${RHAIIS_URL}:${RHAIIS_PORT}/v1/models") and keep the Authorization header
using ${RHAIIS_API_KEY} as before to ensure the full URL is passed as a single
argument.
In `@tests/e2e/configs/run-rhaiis.yaml`:
- Line 25: The YAML for the vllm provider uses the wrong field name; in the
remote::vllm provider config replace the key base_url with url so the provider
schema matches llama-stack 0.4.3 (i.e., change the base_url entry to url in the
run-rhaiis.yaml vllm config to match other configs like run-rhelai.yaml and
examples/vllm-*.yaml).
---
Nitpick comments:
In `@docker-compose.yaml`:
- Line 46: The compose file uses an unset expansion for RHAIIS_PORT
("RHAIIS_PORT=${RHAIIS_PORT}") that is inconsistent with
docker-compose-library.yaml which uses the "${RHAIIS_PORT:-}" form; update the
variable in docker-compose.yaml to use the same expansion (replace
RHAIIS_PORT=${RHAIIS_PORT} with RHAIIS_PORT=${RHAIIS_PORT:-}) so default
handling is consistent, or alternatively ensure CI/server-mode always sets
RHAIIS_PORT before invoking docker compose; target the RHAIIS_PORT entry in the
service environment block to apply the change.
| - name: Test RHAIIS connectivity | ||
| run: | | ||
| curl -f ${RHAIIS_URL}:8000/v1/models -H "Authorization: Bearer ${RHAIIS_API_KEY}" | ||
| curl -f ${RHAIIS_URL}:${RHAIIS_PORT}/v1/models -H "Authorization: Bearer ${RHAIIS_API_KEY}" |
There was a problem hiding this comment.
Quote the URL in the connectivity check to prevent shell word-splitting.
If RHAIIS_URL includes a protocol prefix (e.g., https://host) or any special characters, the unquoted expansion could cause unexpected behavior.
Proposed fix
- curl -f ${RHAIIS_URL}:${RHAIIS_PORT}/v1/models -H "Authorization: Bearer ${RHAIIS_API_KEY}"
+ curl -f "${RHAIIS_URL}:${RHAIIS_PORT}/v1/models" -H "Authorization: Bearer ${RHAIIS_API_KEY}"📝 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.
| curl -f ${RHAIIS_URL}:${RHAIIS_PORT}/v1/models -H "Authorization: Bearer ${RHAIIS_API_KEY}" | |
| curl -f "${RHAIIS_URL}:${RHAIIS_PORT}/v1/models" -H "Authorization: Bearer ${RHAIIS_API_KEY}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/e2e_tests_rhaiis.yaml at line 112, The curl connectivity
check currently uses unquoted variable expansion which can cause shell
word-splitting when RHAIIS_URL contains special characters; update the command
to quote the URL expansion (e.g., replace curl -f
${RHAIIS_URL}:${RHAIIS_PORT}/v1/models with curl -f
"${RHAIIS_URL}:${RHAIIS_PORT}/v1/models") and keep the Authorization header
using ${RHAIIS_API_KEY} as before to ensure the full URL is passed as a single
argument.
| provider_type: remote::vllm | ||
| config: | ||
| url: http://${env.RHAIIS_URL}:8000/v1/ | ||
| base_url: http://${env.RHAIIS_URL}:${env.RHAIIS_PORT}/v1/ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how base_url is used in remote::vllm provider configuration across the codebase
rg -n "base_url" --type yaml -C2 --glob '**/run-*.yaml'
echo "---"
# Check if other config files use 'url' or 'base_url' for vllm
rg -n "provider_type: remote::vllm" -A5 --type yamlRepository: lightspeed-core/lightspeed-stack
Length of output: 2727
🏁 Script executed:
#!/bin/bash
# Search for vllm provider implementation or config schema
fd -type f -name "*.py" | xargs rg -l "remote::vllm" 2>/dev/null | head -10
echo "---"
# Also search for vllm config handling
rg -n "vllm" --type py -B2 -A5 | grep -E "(base_url|url|config)" | head -30Repository: lightspeed-core/lightspeed-stack
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Search for vllm config handling in Python files
rg -n "url|base_url" --type py -B3 -A3 | grep -i vllm -B5 -A5 | head -50
echo "---"
# Look for config class definitions or dataclass patterns
rg -n "class.*vllm|@dataclass" --type py -i | head -20
echo "---"
# Search for any schema or type definitions
fd -type f -name "*.py" -path "*vllm*" 2>/dev/null
echo "---"
# Check for llama-stack requirements or dependencies
cat requirements.txt 2>/dev/null | grep -i llama
cat pyproject.toml 2>/dev/null | grep -i llamaRepository: lightspeed-core/lightspeed-stack
Length of output: 719
🏁 Script executed:
#!/bin/bash
# Search for any documentation about vllm provider config in the repo
rg -n "vllm" -i docs/ --type md -B3 -A3 2>/dev/null | head -50
echo "---"
# Check for any schema or type hints in the repo
rg -n "url.*vllm|vllm.*url" -i --type py -B2 -A2 | head -40Repository: lightspeed-core/lightspeed-stack
Length of output: 2809
🌐 Web query:
llama-stack 0.4.3 vllm provider base_url vs url configuration schema
💡 Result:
In llama-stack 0.4.3, the remote::vllm inference provider config schema uses url (not base_url).
remote::vllm config fields (schema)
url: str | None— vLLM serving endpoint URLmax_tokens: int(default4096)api_token: str | None(default"fake")tls_verify: bool | str(defaultTrue)refresh_models: bool(defaultFalse) [1]
Example run.yaml
providers:
inference:
- provider_id: vllm0
provider_type: remote::vllm
config:
url: ${env.VLLM_URL:=http://localhost:8000/v1}The official examples also use config.url. [1], [2]
Where base_url does show up
- Client-side (e.g.,
LlamaStackClient(base_url="http://localhost:8321")) for talking to the Llama Stack server, not for configuring the vLLM provider. [3]
So: if you’re seeing a base_url vs url mismatch, for provider_type: remote::vllm you should configure config.url. [1]
Sources: [1] [2] [3]
Change base_url to url — this is the wrong field name for the vllm provider schema.
llama-stack 0.4.3's remote::vllm provider config expects url, not base_url. All other vllm configurations in the repository (run-rhelai.yaml, examples/vllm-*.yaml, e2e-prow/rhoai/configs/run.yaml) consistently use url. The base_url field does not exist in the vllm provider schema and will cause configuration parsing to fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/configs/run-rhaiis.yaml` at line 25, The YAML for the vllm provider
uses the wrong field name; in the remote::vllm provider config replace the key
base_url with url so the provider schema matches llama-stack 0.4.3 (i.e., change
the base_url entry to url in the run-rhaiis.yaml vllm config to match other
configs like run-rhelai.yaml and examples/vllm-*.yaml).
3 participants
Description
RHAIIS endpoint port changed, which made these changes needed (port was hardcoded). Also, e2e tests in library mode were added to RHAIIS e2e tests.
Please take a look at e2e test results here.
The interesting takeaways are that:
Type of change
Tools used to create PR
NA
Related Tickets & Documents
Checklist before requesting a review
Testing
See the linked test results
Summary by CodeRabbit
New Features
Chores