LCORE-585: Dump config operation does not need to connect to Llama Stack #442
Conversation
|
Warning Rate limit exceeded@tisnik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds an early-exit for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI Args
participant Main as main()
participant Cfg as Config
participant Client as AsyncLlamaStackClient
participant Llama as Llama Stack Checker
participant UVI as UVicorn
Note over Main: Application startup
Main->>Cfg: parse args & build configuration
alt Dump mode (-d/--dump-configuration)
Main->>Cfg: dump configuration -> configuration.json
Note over Main: log success / exit early (no client, no version check, no UVicorn)
Main-->>Main: return / SystemExit on failure
else Normal mode
Main->>Client: create AsyncLlamaStackClient
Note right of Main: Version compatibility check (new)
Main->>Llama: asyncio.run(check_llama_stack_version(client))
Llama-->>Main: compatibility result
alt Version compatible
Main->>UVI: start UVicorn server (unconditional after check)
else Version incompatible
Main-->>Main: handle/report error (startup prevented)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lightspeed_stack.py (2)
61-65: Do not log full configuration objects; likely secret/API-key leakage.These info logs will stringify the entire configuration, which (per models/client config) can include api_key and TLS key password. That’s a security risk and may leak credentials into logs.
Suggested approach (outside this range):
- Replace with a simple “loaded from path” message at INFO and move any detailed config to DEBUG with redaction.
logger.info("Loaded configuration from %s", args.config_file) if args.verbose: safe = configuration.configuration.model_dump( exclude={"llama_stack": {"api_key"}, "service": {"tls_config": {"tls_key_password"}}} ) logger.debug("Configuration (redacted): %s", safe)Or drop the detailed config log entirely.
29-35: Wire up-v/--verboseto adjust logging levelThe
--verboseflag is parsed but never used, so it currently has no effect. After parsing arguments inmain(), set the root logger and its handlers toDEBUGwhenargs.verboseis true, otherwise keepINFO.• In
src/lightspeed_stack.py, immediately after line 59 (args = parser.parse_args()), insert:level = logging.DEBUG if args.verbose else logging.INFO root = logging.getLogger() root.setLevel(level) for h in root.handlers: h.setLevel(level)This ensures
-v/--verboseactually makes the output verbose.
🧹 Nitpick comments (5)
src/lightspeed_stack.py (5)
67-71: Early-return for dump mode is correct; add error handling and an explicit success log.Right now a filesystem error (permissions, read-only FS, etc.) would raise and print a stack trace. Also, there’s no positive confirmation that the dump completed. Wrap the dump in try/except and exit non‑zero on failure.
Apply this diff within this block:
if args.dump_configuration: - configuration.configuration.dump() - return + try: + configuration.configuration.dump() + logger.info("Configuration dumped to configuration.json") + except Exception as exc: + logger.error("Failed to dump configuration: %s", exc) + raise SystemExit(1) + returnOptional follow-ups (outside this range):
- Add an output path flag so users can choose the target file, defaulting to configuration.json.
# in create_argument_parser() parser.add_argument( "-o", "--output", dest="dump_output", metavar="FILE", help="write dumped configuration to FILE (default: configuration.json)", default="configuration.json", )
- Use it in dump mode:
filename = args.dump_output configuration.configuration.dump(filename) logger.info("Configuration dumped to %s", filename)
79-81: Handle version-check failures cleanly and use a non-zero exit code.If the Llama Stack is incompatible or unreachable, this currently raises and prints a traceback. Gracefully log and exit to make the UX tighter.
Apply this diff:
- # check if the Llama Stack version is supported by the service - asyncio.run(check_llama_stack_version(client)) + # check if the Llama Stack version is supported by the service + try: + asyncio.run(check_llama_stack_version(client)) + except Exception as exc: + logger.error("Llama Stack version check failed: %s", exc) + raise SystemExit(2)Optional: avoid creating two event loops by combining client init + version check into one coroutine:
async def _init_and_check(cfg): holder = AsyncLlamaStackClientHolder() await holder.load(cfg.llama_stack) await check_llama_stack_version(holder.get_client()) # then: asyncio.run(_init_and_check(configuration.configuration))
29-35: Nit: store_true options should default to False, not None.Using None works (falsy), but the conventional and clearer default for store_true flags is False.
Suggested adjustments (outside this range):
parser.add_argument("-v", "--verbose", action="store_true", default=False, help="make it verbose") parser.add_argument("-d", "--dump-configuration", action="store_true", default=False, help="dump actual configuration into JSON file and quit")Also applies to: 37-43
73-81: Optional: avoid multiple event loops by batching async startup steps.You call asyncio.run twice (once for client init, once for version check). It’s fine, but a single run with a small orchestrator coroutine is cleaner and slightly cheaper.
Example (outside this range):
async def _startup(cfg): holder = AsyncLlamaStackClientHolder() await holder.load(cfg.llama_stack) await check_llama_stack_version(holder.get_client()) asyncio.run(_startup(configuration.configuration))
67-71: Add a regression test for dump mode to ensure no Llama Stack connection or Uvicorn startup occurs.Safeguard the refactor by asserting that neither client loading nor Uvicorn is called when -d is provided.
Example pytest (outside this range):
import builtins import json from unittest.mock import patch from src import lightspeed_stack as ls def test_dump_mode_skips_client_and_uvicorn(tmp_path, monkeypatch): out = tmp_path / "conf.json" # simulate CLI args monkeypatch.setenv("PYTHONWARNINGS", "ignore") # avoid noise with patch.object(ls, "create_argument_parser") as cap: p = ls.ArgumentParser() p.add_argument("-d", "--dump-configuration", action="store_true", default=True) p.add_argument("-c", "--config", default="lightspeed-stack.yaml") cap.return_value = p # stub configuration dump target class Cfg: def dump(self, filename="configuration.json"): with open(out, "w", encoding="utf-8") as f: f.write("{}") class Ctx: configuration = Cfg() llama_stack_configuration = object() service_configuration = object() def load_configuration(self, _): pass with patch.object(ls, "configuration", Ctx()), \ patch("runners.uvicorn.start_uvicorn") as start, \ patch("client.AsyncLlamaStackClientHolder.load") as load: ls.main() start.assert_not_called() load.assert_not_called() assert out.exists()Want me to open a follow-up PR with this test?
📜 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 selected for processing (1)
src/lightspeed_stack.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lightspeed_stack.py (5)
src/configuration.py (2)
configuration(55-60)service_configuration(63-68)src/models/config.py (1)
dump(316-319)src/client.py (3)
AsyncLlamaStackClientHolder(18-50)load(23-42)get_client(44-50)src/utils/llama_stack_version.py (1)
check_llama_stack_version(22-31)src/runners/uvicorn.py (1)
start_uvicorn(12-31)
⏰ 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 (1)
src/lightspeed_stack.py (1)
82-83: LGTM: Unconditional Uvicorn startup is safely gated by the early return.This matches the PR goal: dump mode exits before reaching service startup.
tisnik
force-pushed
the
dump-config-does-not-need-to-connect-to-llama-stack
branch
3 times, most recently
from
bd1e375 to
73dcf3d
Compare
tisnik
force-pushed
the
dump-config-does-not-need-to-connect-to-llama-stack
branch
from
73dcf3d to
9cb65a2
Compare
tisnik
changed the title
1 participant
Description
Dump config operation does not need to connect to Llama Stack
Type of change
Summary by CodeRabbit