LCORE-437: check Llama Stack version on startup #422
Conversation
WalkthroughAdds runtime Llama Stack version validation. Introduces min/max version constants, a utility to fetch/compare versions using semver, integrates a preflight check into startup, updates dependencies and README, and adds unit tests for valid/invalid version scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant Service as Lightspeed Core Service
participant Client as AsyncLlamaStackClient
participant Llama as Llama Stack API
Admin->>Service: Start service
Service->>Client: Create/get client
Service->>Llama: inspect.version()
Llama-->>Service: VersionInfo(version)
Service->>Service: compare_versions(min, current, max)
alt Version out of range
Service-->>Admin: Abort startup (log/notify invalid version)
else Version acceptable
Service->>Service: Proceed to server startup (uvicorn)
Service-->>Admin: Service running
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
README.md (1)
30-30: Fix Markdown list indentation per markdownlint (MD007)markdownlint flags this line: expected indentation 2, actual 4.
Apply this diff:
- * [Llama Stack version check](#llama-stack-version-check) + * [Llama Stack version check](#llama-stack-version-check)Also ensure sibling entries under “Configuration” use consistent 2-space indentation.
src/lightspeed_stack.py (2)
15-15: Import the exception for explicit handling and cleaner shutdownYou currently let version validation errors bubble up. Importing the exception enables graceful handling and a clear exit path.
Apply this diff:
-from utils.llama_stack_version import check_llama_stack_version +from utils.llama_stack_version import ( + check_llama_stack_version, + InvalidLlamaStackVersionException, +)
70-72: Handle invalid version with clear log and exit; avoid raw stacktraceCatching
InvalidLlamaStackVersionExceptionlets you log a concise message and exit with status 1, aligning with the README claim that the service won’t start and admins are informed.Apply this diff:
- client = AsyncLlamaStackClientHolder().get_client() - asyncio.run(check_llama_stack_version(client)) + client = AsyncLlamaStackClientHolder().get_client() + try: + asyncio.run(check_llama_stack_version(client)) + except InvalidLlamaStackVersionException as exc: + logger.error("Unsupported Llama Stack version: %s", exc) + raise SystemExit(1)Optional: consider refactoring to a single async
main()(avoid multipleasyncio.run(...)) for cleaner event loop management.src/utils/llama_stack_version.py (3)
7-7: Avoid importing from a private module path
llama_stack_client._clientis an internal module; it may change without notice. Prefer the public import if available.Apply this diff:
-from llama_stack_client._client import AsyncLlamaStackClient +from llama_stack_client import AsyncLlamaStackClient
15-15: Standardize logger nameElsewhere you use
__name__. Using the module’s__name__keeps logging consistent and respects logger hierarchy/config by package.Apply this diff:
-logger = logging.getLogger("utils.llama_stack_version") +logger = logging.getLogger(__name__)
34-51: Harden parsing and polish log message
- If Llama Stack ever returns a non-strict SemVer (e.g., metadata/pre-release that vends invalid semver),
Version.parse()will raise. Catch and re-raise as your custom exception with a clear message.- Minor: fix the extra space before the colon in the success log.
Apply this diff:
-def compare_versions(version_info: str, minimal: str, maximal: str) -> None: +def compare_versions(version_info: str, minimal: str, maximal: str) -> None: """Compare current Llama Stack version with minimal and maximal allowed versions.""" - current_version = Version.parse(version_info) - minimal_version = Version.parse(minimal) - maximal_version = Version.parse(maximal) + try: + current_version = Version.parse(version_info) + minimal_version = Version.parse(minimal) + maximal_version = Version.parse(maximal) + except Exception as exc: + raise InvalidLlamaStackVersionException( + f"Unable to parse Llama Stack version '{version_info}': {exc}" + ) from exc @@ - logger.info("Correct Llama Stack version : %s", current_version) + logger.info("Correct Llama Stack version: %s", current_version)Note: The more robust parse makes operational issues easier to diagnose if upstream formatting changes.
tests/unit/utils/test_llama_stack_version.py (6)
8-16: Prefer package-qualified imports to avoid relying on sys.path tweaks
from utils.llama_stack_version ...andfrom constants ...work only ifsrc/is on PYTHONPATH. If the project is packaged (src-layout), prefer absolute package imports (e.g.,from lightspeed_stack.utils.llama_stack_version import ...) to improve portability.Would you like me to propose a follow-up patch once the canonical package name is confirmed?
19-31: Assert the async call path was exercisedCurrent test validates “no exception.” Strengthen it by asserting the mocked coroutine was awaited.
Apply:
# test if the version is checked await check_llama_stack_version(mock_client) + mock_client.inspect.version.assert_awaited_once()
33-45: Mirror the awaited-call assertion here as wellSame improvement as the minimal-version test.
# test if the version is checked await check_llama_stack_version(mock_client) + mock_client.inspect.version.assert_awaited_once()
69-75: Usere.escapeand explicitly format the version as stringKeep the regex match literal and avoid depending on
__str__of the semver class implicitly.- expected_exception_msg = ( - f"Llama Stack version <= {MAXIMAL_SUPPORTED_LLAMA_STACK_VERSION} is required, " - + f"but {bigger_version} is used" - ) + expected_exception_msg = re.escape( + f"Llama Stack version <= {MAXIMAL_SUPPORTED_LLAMA_STACK_VERSION} is required, " + f"but {str(bigger_version)} is used" + )
78-101: Subtests are fine; consider parametrize for tighter pytest idioms
pytest-subtestsworks, but nativepytest.mark.parametrizeis simpler and produces clearer test IDs. Optional nit.Example:
import pytest MAX_V = Version.parse(MAXIMAL_SUPPORTED_LLAMA_STACK_VERSION) @pytest.mark.asyncio @pytest.mark.parametrize( "desc,bigger_version", [ ("Increased patch number", MAX_V.bump_patch()), ("Increased minor number", MAX_V.bump_minor()), ("Increased major number", MAX_V.bump_major()), ("Increased all numbers", MAX_V.bump_major().bump_minor().bump_patch()), ], ids=lambda p: p[0], ) async def test_check_llama_stack_version_too_big_version_param(mocker, desc, bigger_version): mock_client = mocker.AsyncMock() await _check_version_must_fail(mock_client, bigger_version)
57-63: Escape regex metacharacters in the expected exception messageThe
match=parameter treats its argument as a regex, so dots (.) and other symbols in the version string can be misinterpreted. To ensure a literal match:• In
tests/unit/utils/test_llama_stack_version.py, add at the top:import re• Replace your two-part string with a single f-string wrapped in
re.escape:- expected_exception_msg = ( - f"Llama Stack version >= {MINIMAL_SUPPORTED_LLAMA_STACK_VERSION} " - + "is required, but 0.0.0 is used" - ) + expected_exception_msg = re.escape( + f"Llama Stack version >= {MINIMAL_SUPPORTED_LLAMA_STACK_VERSION} is required, but 0.0.0 is used" + )• Verify that in
src/utils/llama_stack_version.pythe exception is raised with exactly:f"Llama Stack version >= {minimal_version} is required, but {current_version} is used"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
README.md(2 hunks)pyproject.toml(2 hunks)src/constants.py(1 hunks)src/lightspeed_stack.py(2 hunks)src/utils/llama_stack_version.py(1 hunks)tests/unit/utils/test_llama_stack_version.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/unit/utils/test_llama_stack_version.py (1)
src/utils/llama_stack_version.py (2)
check_llama_stack_version(22-31)InvalidLlamaStackVersionException(18-19)
src/lightspeed_stack.py (2)
src/utils/llama_stack_version.py (1)
check_llama_stack_version(22-31)src/client.py (2)
AsyncLlamaStackClientHolder(18-50)get_client(44-50)
🪛 markdownlint-cli2 (0.17.2)
README.md
30-30: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (3)
pyproject.toml (1)
95-95: LGTM: Add pytest-subtests for nicer parametrized subtestsThe dev dependency addition is appropriate for the new async/subtest-style tests.
src/constants.py (1)
3-5: LGTM: Explicitly bounding supported Llama Stack versionPinning both min and max to 0.2.17 makes the behavior unambiguous and testable. This aligns with the dependency pins.
tests/unit/utils/test_llama_stack_version.py (1)
1-102: Solid coverage and clear structureGood boundary checks (min/max and out-of-range), async flow, and message validation. With the semver import fix and minor assertions in place, this test suite will be robust.
| "email-validator>=2.2.0", | ||
| "openai==1.99.9", | ||
| "sqlalchemy>=2.0.42", | ||
| "semver<4.0.0", |
There was a problem hiding this comment.
Pin semver to >=3,<4 to avoid runtime ImportError
utils/llama_stack_version.py imports from semver import Version, which is only available in semver v3+. With the current constraint semver<4.0.0, environments can resolve to v2.x, causing ImportError at startup/tests.
Apply this diff:
- "semver<4.0.0",
+ "semver>=3,<4.0.0",📝 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.
| "semver<4.0.0", | |
| "semver>=3,<4.0.0", |
🤖 Prompt for AI Agents
In pyproject.toml around line 39, the dependency constraint "semver<4.0.0" is
too loose and can resolve to semver v2.x which lacks `Version`; change the
constraint to "semver>=3,<4" to ensure semver v3+ is installed; after updating
pyproject.toml, regenerate/update your lock file (e.g., poetry.lock or
equivalent) and run tests/startup to verify the ImportError is resolved.
| ## Llama Stack version check | ||
|
|
||
| During Lightspeed Core Stack service startup, the Llama Stack version is retrieved. The version is tested against two constants `MINIMAL_SUPPORTED_LLAMA_STACK_VERSION` and `MAXIMAL_SUPPORTED_LLAMA_STACK_VERSION` which are defined in `src/constants.py`. If the actual Llama Stack version is outside the range defined by these two constants, the service won't start and administrator will be informed about this problem. | ||
|
|
||
|
|
There was a problem hiding this comment.
Documented version check is good; update earlier example to avoid guaranteed startup failure
The new section clearly explains the startup guard. However, the earlier “Llama Stack project and configuration” example pins llama-stack==0.2.14. With MIN/MAX set to 0.2.17, following that example will make lightspeed-core refuse to start. Please update the example to 0.2.17 (or expand the allowed range in constants if you intend broader compatibility).
Suggested change to the example block (shown here for clarity):
# In the earlier example section
dependencies = [
- "llama-stack==0.2.14",
+ "llama-stack==0.2.17",
"fastapi>=0.115.12",
# ...
]🤖 Prompt for AI Agents
In README.md around lines 247 to 251, the earlier "Llama Stack project and
configuration" example pins llama-stack to 0.2.14 which will conflict with the
documented MINIMAL_SUPPORTED_LLAMA_STACK_VERSION (0.2.17) and cause startup
failure; update that example to use "llama-stack==0.2.17" (or alternatively
broaden the MIN/MAX constants in src/constants.py if wider compatibility is
intended) so the example and the version guard are consistent.
| """Unit tests for utility function to check Llama Stack version.""" | ||
|
|
||
| import pytest | ||
| from semver import Version |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
semver API mismatch risk: Version is only guaranteed in semver>=3
Using from semver import Version assumes semver v3+. The PR pins semver<4.0.0, which allows v2.x to be resolved in some environments, breaking the import. Make the tests resilient across 2.x/3.x or bump the lower bound.
Two fix options (pick one):
- Make tests compatible with both semver 2.x and 3.x by falling back to VersionInfo:
-from semver import Version
+try:
+ # semver >= 3
+ from semver import Version
+except ImportError: # semver < 3
+ from semver import VersionInfo as Version- Alternatively, tighten dependency lower bound in pyproject.toml:
# pyproject.toml
semver = ">=3.0.0,<4.0.0"Run this to confirm the declared semver constraint and catch any 2.x resolution:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Semver dependency line(s):"
rg -n -C1 -i '^\s*semver\s*=' pyproject.toml || trueLength of output: 136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) All occurrences of 'semver' in the repo:"
rg -n "semver" || true
echo
echo "2) 'semver =' in TOML files:"
rg -nP "semver\s*=" --glob "*.toml" || true
echo
echo "3) 'semver' in setup.py / setup.cfg:"
grep -R "semver" setup.py setup.cfg || true
echo
echo "4) 'semver' in Pipfile / requirements*.txt:"
grep -R "semver" Pipfile requirements*.txt || trueLength of output: 1648
Tighten semver lower bound to ≥3.0.0
The project currently pins only semver<4.0.0 (pyproject.toml line 39), which allows semver 2.x—where Version doesn’t exist. Both runtime (src/utils/llama_stack_version.py:5) and tests (tests/unit/utils/test_llama_stack_version.py:4) import Version, so installing semver 2.x will break.
Please update your dependency declaration:
pyproject.toml (inside [project] or [tool.poetry.dependencies]):
- "semver<4.0.0",
+ "semver>=3.0.0,<4.0.0",If you really need to support semver 2.x, apply a fallback import in both code and tests:
-from semver import Version
+try:
+ from semver import Version
+except ImportError:
+ from semver import VersionInfo as Version🤖 Prompt for AI Agents
In tests/unit/utils/test_llama_stack_version.py around line 4, the test imports
Version from semver but the project allows semver 2.x (which lacks Version);
update pyproject.toml to require semver>=3.0.0,<4.0.0 to ensure Version exists,
and/or add a defensive fallback import in both src/utils/llama_stack_version.py
and this test so they first try to import Version and, if not present, import
the compatible type or construct from the older semver API; make the dependency
change the primary fix and add the fallback only if you must support semver 2.x.
1 participant
Description
LCORE-437: check Llama Stack version on startup
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Documentation
Chores
Tests