LCORE-836 spike: merge run.yaml into lightspeed-stack.yaml#1580
LCORE-836 spike: merge run.yaml into lightspeed-stack.yaml#1580max-svistunov wants to merge 9 commits into
Conversation
Add a unified `llama_stack.config` sub-section to `lightspeed-stack.yaml` that lets operators express the Llama Stack operational configuration in one place, eliminating the need for a separately maintained `run.yaml`. Legacy mode (`llama_stack.library_client_config_path` + external run.yaml) is preserved and mutually exclusive with the new path. New Pydantic classes `UnifiedLlamaStackConfig`, `UnifiedInferenceSection`, and `UnifiedInferenceProvider` define the unified schema; a new `synthesize_configuration` pipeline applies profile (or baseline) → existing BYOK RAG / Solr OKP enrichment → high-level sections → `native_override` (deep-merge, list-replacement). A `baseline: default | empty` field enables strict lossless round-trip for the migration tool. Library-mode wiring in `src/client.py` detects the unified form and writes the synthesized file to disk for `AsyncLlamaStackAsLibraryClient` (which the PoC confirmed requires a file path, not a dict). Legacy enrichment path is unchanged. A `--migrate-config` flag on the `lightspeed-stack` CLI produces a unified single-file config from a legacy (run.yaml, lightspeed-stack.yaml) pair (dumb lift-and-shift: content goes under `native_override` with `baseline: empty`, and `library_client_config_path` is removed). The LS container's `llama_stack_configuration.py` CLI now auto-detects unified vs legacy based on the presence of `llama_stack.config`; the entrypoint script requires no functional change (comment clarified). `test.containerfile` copies `src/data/` into the container so the shipped default baseline resolves at runtime. Tests: 22 new unit tests covering merge semantics, high-level inference expansion, the full synthesize pipeline, profile loading, precedence (profile < high-level < native_override), and migrate-then-synthesize round-trip lossless equality. 3 new schema tests cover unified/legacy mutual exclusion. 5 existing dump-configuration expectations updated for the new `config: None` field; 1 client error-message regex updated. Full `uv run make verify` passes (black, pylint 10/10, ruff, docstyle, mypy). `uv run pytest tests/unit/` — 2098 passed, 1 skipped, 0 failed.
Add the spike doc (decisions up front, background below, 7 proposed JIRAs) and the spec doc (requirements R1..R11, architecture, implementation guide, migration worked example) under `docs/design/llama-stack-config-merge/`. Key decisions captured for reviewer confirmation: - Overall shape: Option C (high-level + native_override) with Option E (profile feature, no shipped profiles) as an optional layer. - Deprecation: calendar-based (e.g., "legacy path removed no sooner than 6 months after WARN begins"); concrete timing deferred to PM review. - Override precedence: deep-merge with list replacement at leaf level. - Secrets handling: env-var references preserved verbatim in synthesized files; never resolved to disk. - Format detection: shape-based, with an optional `config_format_version` field that, if present, must agree with the shape. - Migration tool shape: `--migrate-config` flag (no CLI refactor); dumb lift-and-shift mode only in v1; smart mode deferred. - Profile distribution: feature only, LCORE ships no profiles of its own beyond reference examples under `examples/profiles/`. - LS process supervision and hot-reload: out of scope (LCORE-777, LCORE-778, LCORE-781 territory). The spike's PoC validated library-mode end-to-end: a `lightspeed-stack.yaml` containing only `llama_stack.config` (no external run.yaml) boots LCORE, serves /v1/query with a real model response, and a `native_override` value demonstrably takes effect in the synthesized run.yaml. Server-mode end-to-end through docker-compose was skipped because the LS container image rebuild (~2 GB, UBI + llama-stack llslibdev dependency sync) was impractical for the spike timeline; the same synthesis code path is exercised by the unit tests, including the lossless migrate-then-synthesize round-trip. PoC evidence is under `poc-evidence/library-mode/` as reference material for reviewers, and per the spike howto it is intended to be removed from the branch prior to merge. The spike doc and spec doc remain permanent.
WalkthroughThis PR implements LCORE-836, a unified configuration merge feature that consolidates legacy two-file configuration (separate lightspeed-stack.yaml and run.yaml) into a single lightspeed-stack.yaml containing a llama_stack.config section with layered synthesis (baseline, profile, high-level inference transformation, and native_override deep-merge). The feature includes synthesizer pipeline, dumb-mode migration tooling, client-side branching between unified/legacy modes, new CLI workflow, runtime integration, and comprehensive testing. ChangesUnified Configuration Synthesis and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 14
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 (1)
117-165:⚠️ Potential issue | 🟡 MinorStale
main()docstring — references non-existent flag and omits the new--migrate-configflow.The docstring still mentions
--generate-llama-stack-configuration, which is not a CLI flag in this parser, and it does not describe the newly added--migrate-config/--run-yaml/--migrate-outputbranch that exits beforeload_configurationruns. Please update the docstring to match the actual behavior so theRaises: SystemExitpaths and flag list stay accurate.✏️ Proposed docstring fix
- If --dump-schema is provided, writes the active configuration schema to schema.json and exits (exits with status 1 on failure). - - If --generate-llama-stack-configuration is provided, generates and stores - the Llama Stack configuration to the specified output file and exits - (exits with status 1 on failure). + - If --migrate-config is provided, migrates the legacy + (run.yaml + lightspeed-stack.yaml) setup into a unified single-file + configuration at --migrate-output and exits (status 1 on failure). + This branch bypasses configuration.load_configuration(). - Otherwise, sets LIGHTSPEED_STACK_CONFIG_PATH for worker processes, starts the quota scheduler, and starts the Uvicorn web service. Raises: - SystemExit: when configuration dumping or Llama Stack generation fails - (exits with status 1). + SystemExit: when configuration dumping, schema dumping, or config + migration fails (exits with status 1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lightspeed_stack.py` around lines 117 - 165, Update the main() docstring to remove the obsolete reference to --generate-llama-stack-configuration, add documentation for the new --migrate-config flow and its related flags (--run-yaml and --migrate-output), and state that when args.migrate_config is true the migration branch (handled by migrate_config_dumb) runs and exits before load_configuration; also keep the note that failures in dumping/migration raise SystemExit (status 1). Reference main(), args.migrate_config, and migrate_config_dumb so the updated docstring accurately matches the implemented control flow.src/llama_stack_configuration.py (1)
918-931:⚠️ Potential issue | 🔴 CriticalSeparate raw and expanded config to prevent writing secrets to output file.
synthesize_to_file()is documented to preserve env-var references like${env.FOO}verbatim in the output. The current code expands all environment variables on line 920 before passing tosynthesize_to_file(), causing secrets to be written in plaintext to the generatedrun.yaml.Use
raw_configforsynthesize_to_file()(preserves env refs as documented), andexpanded_configforsetup_azure_entra_id_token()(which requires actual credential values) andgenerate_configuration()(legacy mode enrichment):Keep raw unified config separate from expanded legacy config
with open(args.config, "r", encoding="utf-8") as f: - config = yaml.safe_load(f) - config = replace_env_vars(config) + raw_config = yaml.safe_load(f) + expanded_config = replace_env_vars(raw_config) - unified_present = (config.get("llama_stack") or {}).get("config") is not None + unified_present = (raw_config.get("llama_stack") or {}).get("config") is not None if unified_present: logger.info("Unified mode detected (llama_stack.config present)") # Azure Entra ID side-effect (writes .env) stays part of boot — still run it. - setup_azure_entra_id_token(config.get("azure_entra_id"), args.env_file) + setup_azure_entra_id_token(expanded_config.get("azure_entra_id"), args.env_file) synthesize_to_file( - config, + raw_config, args.output, config_file_dir=Path(args.config).resolve().parent, ) else: logger.info("Legacy mode detected (no llama_stack.config)") - generate_configuration(args.input, args.output, config, args.env_file) + generate_configuration(args.input, args.output, expanded_config, args.env_file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llama_stack_configuration.py` around lines 918 - 931, The code currently calls replace_env_vars on the loaded config and then passes that expanded config into synthesize_to_file, which will write expanded secrets into the output; instead, keep two variables: raw_config = yaml.safe_load(...) (no replace_env_vars) and expanded_config = replace_env_vars(raw_config); use raw_config when calling synthesize_to_file(...) so env-var references remain verbatim, and use expanded_config when calling setup_azure_entra_id_token(...) and when invoking generate_configuration(...) or any legacy-mode enrichment that needs real values; update references of config in this block to the appropriate raw_config or expanded_config names accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/llama-stack-config-merge/llama-stack-config-merge-spike.md`:
- Around line 274-284: The markdown file contains fenced code blocks that lack
surrounding blank lines and trigger markdownlint MD031; for each fenced block
(e.g., the "Agentic tool instruction" block and the other blocks at the noted
ranges like 310-317, 343-349, 376-381, 411-416, 438-443, 463-468, 598-626) add
an empty line immediately before the opening ``` and an empty line immediately
after the closing ``` so every fenced code block is separated by blank lines
from surrounding text/content.
- Line 147: The in-page anchor in the link "See [Merge semantics worked
examples](`#merge-semantics-worked-examples`)." doesn't match the actual heading
"### Merge semantics — worked examples"; update either the link target or the
heading so they match under markdownlint rules — e.g., change the heading to use
a plain hyphen "### Merge semantics - worked examples" or change the link to the
exact slug produced from the em-dash (percent-encoded or the renderer-specific
slug), ensuring the text in the link target and the heading "Merge semantics —
worked examples" are identical.
In `@docs/design/llama-stack-config-merge/llama-stack-config-merge.md`:
- Around line 280-291: Update the doc to stop claiming synthesized files are
safe as world-readable and instead require restrictive file permissions and
recommend env-var references; specifically change wording around
native_override, apply_high_level_inference, replace_env_vars and run.yaml to
note that native_override and migrations may include literal secrets (e.g., API
keys), that apply_high_level_inference may not always emit only ${env.<VAR>} and
replace_env_vars is not a guarantee for all inputs, and mandate default
restrictive filesystem permissions and operator guidance to prefer env refs and
secrets management rather than declaring world-readable output acceptable.
In `@docs/design/llama-stack-config-merge/poc-evidence/library-mode/README.md`:
- Around line 3-8: The fenced bash code block starting with "```bash" in the
README.md is missing blank lines before and/or after (markdownlint MD031); add a
single blank line immediately above the opening ```bash line and a single blank
line immediately below the closing ``` line so the fenced block is separated
from surrounding text and satisfies MD031.
In
`@docs/design/llama-stack-config-merge/poc-evidence/library-mode/synthesized-run.yaml`:
- Around line 107-110: The shields entry has an incorrect provider_shield_id
value: replace the OpenAI chat model id in the shields list (symbols: shields,
provider_id, provider_shield_id, shield_id) with the correct Llama Guard model
id or remove the mistaken native_override that injected it; specifically, ensure
provider_shield_id uses a guard model identifier such as
"meta-llama/Llama-Guard-3-8B" (or the intended guard model) and verify the
native_override/source that produced the OpenAI id is fixed so future artifacts
don’t carry an OpenAI model as a Llama Guard shield.
In
`@docs/design/llama-stack-config-merge/poc-evidence/lightspeed-stack-unified-library.yaml`:
- Line 18: The committed YAML contains a machine-local absolute path in the
profile key ('profile:
/home/msvistun/repos/lightspeed/stack/tests/e2e/configs/run-ci.yaml'); remove or
replace this with a portable repo-relative path (for example
'./tests/e2e/configs/run-ci.yaml') or delete the profile entry if not needed so
the PoC artifact is reusable outside the author’s workstation; update any
references that expect the old absolute path (search for the profile key) to use
the new relative path.
In `@src/client.py`:
- Around line 104-108: The code currently writes the synthesized config to a
predictable temp path via the synthesized_path variable and open(...), which is
unsafe for concurrent processes and symlink attacks; change this to create a
secure, unique temp file (e.g. via tempfile.NamedTemporaryFile(delete=False) or
os.open with tempfile.mkstemp) and write ls_config to that securely opened file
using yaml.dump, then set restrictive permissions (chmod 0o600) on the new file
and close it before returning/using the path; reference the synthesized_path
creation, the open(...) write block, and the yaml.dump call when making the
change.
In `@src/data/default_run.yaml`:
- Line 155: Remove the trailing blank line at the end of the file to satisfy
YAMLlint's empty-lines rule: edit src/data/default_run.yaml (the file containing
default run config) and delete the final blank/empty line so the file ends with
the last YAML content line only (ensure there is no extra empty line after the
last document or mapping).
- Line 18: The baseline exposes external_providers_dir:
${env.EXTERNAL_PROVIDERS_DIR} without a safe fallback, causing configs to break
when the env var is absent; either provide a sensible default (e.g., set
external_providers_dir to a known safe path or an empty string/null) or remove
the key from the baseline so profiles supply it; update the default_run.yaml
entry for external_providers_dir to use a safe literal default value or delete
the line, and ensure any code reading external_providers_dir handles the chosen
default.
In `@src/lightspeed_stack.py`:
- Around line 150-165: The except block for the migrate_config_dumb call should
log the full traceback; replace the logger.error("Migration failed: %s", e) call
with logger.exception("Migration failed") so the stack trace is captured when
args.migrate_config triggers migrate_config_dumb (refer to migrate_config_dumb
and the surrounding try/except using logger).
In `@src/llama_stack_configuration.py`:
- Around line 825-829: The file write is using open(output_file, "w") which can
create world-readable files; change the write to create the file with explicit
restrictive permissions (mode 0o600). After ensuring the parent directory is
created (the existing Path(output_file).parent.mkdir call is fine), open the
file using a low-level create with os.open(...) with flags
O_WRONLY|O_CREAT|O_TRUNC and mode 0o600, wrap the returned fd with os.fdopen to
get a text file object, then use yaml.dump(ls_config, f, Dumper=YamlDumper,
default_flow_style=False) to write; reference the variables/functions ls_config,
output_file, synthesize_configuration, and YamlDumper so you update the existing
block that writes the synthesized configuration.
In `@tests/unit/test_client.py`:
- Around line 84-90: Replace the current test that relies on
AsyncLlamaStackClientHolder().load(...) to trigger a runtime ValueError with a
direct validation test against the Pydantic model: construct an invalid
LlamaStackConfiguration(...) instance (setting library_client_config_path=None
and leaving both unified and legacy unset) inside pytest.raises(ValueError,
match="neither .*unified.* nor .*legacy.* is set") so the
check_llama_stack_model validator on the LlamaStackConfiguration model is
exercised; this ensures validation fails at model construction rather than via
AsyncLlamaStackClientHolder.load or _load_library_client runtime checks.
In `@tests/unit/test_llama_stack_synthesize.py`:
- Around line 1-371: This PoC test suite should not be merged as-is; either
delete the whole test file (remove the new tests referencing
synthesize_configuration/migrate_config_dumb/apply_high_level_inference) or, if
you intentionally keep it, change tests to avoid relying on in-place mutation by
using the functional return value of apply_high_level_inference (don't assert
that the passed ls_config instance was mutated) and annotate the
MINIMAL_BASELINE constant with Final[dict[str, Any]] and update any tests that
mutate it to operate on a copy (e.g., use copy.deepcopy) so the baseline is not
altered in-place.
---
Outside diff comments:
In `@src/lightspeed_stack.py`:
- Around line 117-165: Update the main() docstring to remove the obsolete
reference to --generate-llama-stack-configuration, add documentation for the new
--migrate-config flow and its related flags (--run-yaml and --migrate-output),
and state that when args.migrate_config is true the migration branch (handled by
migrate_config_dumb) runs and exits before load_configuration; also keep the
note that failures in dumping/migration raise SystemExit (status 1). Reference
main(), args.migrate_config, and migrate_config_dumb so the updated docstring
accurately matches the implemented control flow.
In `@src/llama_stack_configuration.py`:
- Around line 918-931: The code currently calls replace_env_vars on the loaded
config and then passes that expanded config into synthesize_to_file, which will
write expanded secrets into the output; instead, keep two variables: raw_config
= yaml.safe_load(...) (no replace_env_vars) and expanded_config =
replace_env_vars(raw_config); use raw_config when calling
synthesize_to_file(...) so env-var references remain verbatim, and use
expanded_config when calling setup_azure_entra_id_token(...) and when invoking
generate_configuration(...) or any legacy-mode enrichment that needs real
values; update references of config in this block to the appropriate raw_config
or expanded_config names accordingly.
🪄 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: e3d88bb1-37ac-4c41-b1f8-3fd87572f9ef
📒 Files selected for processing (17)
docs/design/llama-stack-config-merge/llama-stack-config-merge-spike.mddocs/design/llama-stack-config-merge/llama-stack-config-merge.mddocs/design/llama-stack-config-merge/poc-evidence/library-mode/README.mddocs/design/llama-stack-config-merge/poc-evidence/library-mode/query-response.jsondocs/design/llama-stack-config-merge/poc-evidence/library-mode/synthesized-run.yamldocs/design/llama-stack-config-merge/poc-evidence/lightspeed-stack-unified-library.yamlscripts/llama-stack-entrypoint.shsrc/client.pysrc/data/default_run.yamlsrc/lightspeed_stack.pysrc/llama_stack_configuration.pysrc/models/config.pytest.containerfiletests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_llama_stack_configuration.pytests/unit/test_client.pytests/unit/test_llama_stack_synthesize.py
📜 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). (11)
- GitHub Check: build-pr
- GitHub Check: unit_tests (3.12)
- GitHub Check: Pylinter
- GitHub Check: E2E: library mode / ci / group 1
- GitHub Check: E2E: server mode / ci / group 3
- GitHub Check: E2E: server mode / ci / group 1
- GitHub Check: E2E Tests for Lightspeed Evaluation job
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E: server mode / ci / group 2
- GitHub Check: E2E: library mode / ci / group 3
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use absolute imports for internal modules:from authentication import get_auth_dependency
Import FastAPI dependencies with:from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with:from llama_stack_client import AsyncLlamaStackClient
Checkconstants.pyfor shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Uselogger = get_logger(__name__)fromlog.pyfor module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Usetyping_extensions.Selffor model validators in Pydantic models
Use modern union type syntaxstr | intinstead ofUnion[str, int]
UseOptional[Type]for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Useasync deffor I/O operations and external API calls
HandleAPIConnectionErrorfrom Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with@abstractmethoddecorators
Use@model_validatorand@field_validatorfor Pydantic model validation
Complete type annotations for all class attributes; use specific types, notAny
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections
Files:
tests/unit/test_client.pytests/unit/models/config/test_dump_configuration.pysrc/lightspeed_stack.pytests/unit/models/config/test_llama_stack_configuration.pysrc/client.pysrc/models/config.pytests/unit/test_llama_stack_synthesize.pysrc/llama_stack_configuration.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest; pytest is the standard for this project
Usepytest-mockfor AsyncMock objects in tests
Use markerpytest.mark.asynciofor async tests
Unit tests require 60% coverage, integration tests 10%
Files:
tests/unit/test_client.pytests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_llama_stack_configuration.pytests/unit/test_llama_stack_synthesize.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Pydantic models extend
ConfigurationBasefor config,BaseModelfor data models
Files:
src/lightspeed_stack.pysrc/client.pysrc/models/config.pysrc/llama_stack_configuration.py
src/**/config*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/config*.py: All config uses Pydantic models extendingConfigurationBase
Base class setsextra="forbid"to reject unknown fields in Pydantic models
Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Use type hints likeOptional[FilePath],PositiveInt,SecretStrin Pydantic models
Files:
src/models/config.py
🧠 Learnings (7)
📚 Learning: 2026-04-19T15:40:25.624Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to **/*.py : Import Llama Stack client with: `from llama_stack_client import AsyncLlamaStackClient`
Applied to files:
tests/unit/test_client.pysrc/client.pysrc/models/config.py
📚 Learning: 2025-12-18T10:21:09.038Z
Learnt from: are-ces
Repo: lightspeed-core/lightspeed-stack PR: 935
File: run.yaml:114-115
Timestamp: 2025-12-18T10:21:09.038Z
Learning: In Llama Stack version 0.3.x, telemetry provider configuration is not supported under the `providers` section in run.yaml configuration files. Telemetry can be enabled with just `telemetry.enabled: true` without requiring an explicit provider block.
Applied to files:
docs/design/llama-stack-config-merge/poc-evidence/library-mode/synthesized-run.yamldocs/design/llama-stack-config-merge/poc-evidence/lightspeed-stack-unified-library.yamlsrc/data/default_run.yamlsrc/models/config.py
📚 Learning: 2026-04-19T15:40:25.624Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to **/*.py : Handle `APIConnectionError` from Llama Stack in error handling
Applied to files:
src/client.py
📚 Learning: 2026-04-07T15:03:11.530Z
Learnt from: jrobertboos
Repo: lightspeed-core/lightspeed-stack PR: 1396
File: src/app/endpoints/conversations_v1.py:6-6
Timestamp: 2026-04-07T15:03:11.530Z
Learning: In the `llama_stack_api` package, all imports MUST use the flat form `from llama_stack_api import <symbol>`. Sub-module imports (e.g., `from llama_stack_api.common.errors import ConversationNotFoundError`) are explicitly NOT supported and considered a code smell, as stated in `llama_stack_api/__init__.py` lines 15-19. Do not flag or suggest changing root-package imports to sub-module imports for this package.
Applied to files:
src/client.py
📚 Learning: 2026-04-19T15:40:25.624Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to src/**/config*.py : All config uses Pydantic models extending `ConfigurationBase`
Applied to files:
src/models/config.py
📚 Learning: 2026-01-12T10:58:40.230Z
Learnt from: blublinsky
Repo: lightspeed-core/lightspeed-stack PR: 972
File: src/models/config.py:459-513
Timestamp: 2026-01-12T10:58:40.230Z
Learning: In lightspeed-core/lightspeed-stack, for Python files under src/models, when a user claims a fix is done but the issue persists, verify the current code state before accepting the fix. Steps: review the diff, fetch the latest changes, run relevant tests, reproduce the issue, search the codebase for lingering references to the original problem, confirm the fix is applied and not undone by subsequent commits, and validate with local checks to ensure the issue is resolved.
Applied to files:
src/models/config.py
📚 Learning: 2026-02-25T07:46:33.545Z
Learnt from: asimurka
Repo: lightspeed-core/lightspeed-stack PR: 1211
File: src/models/responses.py:8-16
Timestamp: 2026-02-25T07:46:33.545Z
Learning: In the Python codebase, requests.py should use OpenAIResponseInputTool as Tool while responses.py uses OpenAIResponseTool as Tool. This difference is intentional due to differing schemas for input vs output tools in llama-stack-api. Apply this distinction consistently to other models under src/models (e.g., ensure request-related tools use the InputTool variant and response-related tools use the ResponseTool variant). If adding new tools, choose the corresponding InputTool or Tool class based on whether the tool represents input or output, and document the rationale in code comments.
Applied to files:
src/models/config.py
🪛 LanguageTool
docs/design/llama-stack-config-merge/llama-stack-config-merge-spike.md
[style] ~451-~451: Try moving the adverb to make the sentence clearer.
Context: ... Link to the migration doc. Legacy mode continues to fully function. Scope: - Warning emission point: ...
(SPLIT_INFINITIVE)
[style] ~655-~655: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ncluding fields LCORE doesn't model. 3. Existing CI/CD templating that treats run.yaml...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~656-~656: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...s run.yaml as a separate artifact. 4. Existing enrichment behavior (Azure Entra ID, BY...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~673-~673: To elevate your writing, try using more formal phrasing here.
Context: ...|---|---|---| | Do nothing | 0 | Legacy keeps working until deprecation window closes | | Lif...
(CONTINUE_TO_VB)
docs/design/llama-stack-config-merge/llama-stack-config-merge.md
[style] ~303-~303: To elevate your writing, try using more formal phrasing here.
Context: ...|---|---|---| | Do nothing | 0 | Legacy keeps working until deprecation closes | | Lift-and-s...
(CONTINUE_TO_VB)
[style] ~405-~405: The double modal “requires supervised” is nonstandard (only accepted in certain dialects). Consider “to be supervised”.
Context: ...oad means any implementation requires supervised restart, which is out of scope here. - ...
(NEEDS_FIXED)
🪛 markdownlint-cli2 (0.22.0)
docs/design/llama-stack-config-merge/llama-stack-config-merge-spike.md
[warning] 147-147: Link fragments should be valid
(MD051, link-fragments)
[warning] 275-275: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 311-311: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 344-344: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 377-377: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 412-412: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 439-439: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 464-464: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 599-599: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 612-612: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 618-618: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
docs/design/llama-stack-config-merge/poc-evidence/library-mode/README.md
[warning] 4-4: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 YAMLlint (1.38.0)
src/data/default_run.yaml
[error] 155-155: too many blank lines (1 > 0)
(empty-lines)
🔇 Additional comments (6)
test.containerfile (1)
39-46: LGTM.Ownership and copy additions align with the unified-mode entrypoint flow.
tests/unit/models/config/test_dump_configuration.py (1)
147-147: Assertion updates are consistent.Adding
"config": Noneto all five dumped-config expectations matches the new Optionalconfigfield onLlamaStackConfiguration.docs/design/llama-stack-config-merge/poc-evidence/library-mode/query-response.json (1)
1-1: PoC evidence artifact — no action.Per PR description, PoC evidence is expected to be removed prior to merge.
scripts/llama-stack-entrypoint.sh (1)
3-20: Comment/log update LGTM.Header and log string accurately describe the two auto-detected modes; no behavior change.
tests/unit/models/config/test_llama_stack_configuration.py (1)
85-145: LGTM — validation coverage matches the new unified/legacy rules.The tests cover the missing-config error, unified library mode, mutual exclusion, and URL normalization with unified config.
src/models/config.py (1)
750-787: LGTM — mutual-exclusion validation is enforced at model load.The validator rejects unified+legacy mode, requires one config source in library mode, and preserves legacy path validation.
| # --migrate-config runs standalone; does not load config into the singleton, | ||
| # since the input may be in legacy form and we are producing its successor. | ||
| if args.migrate_config: | ||
| # pylint: disable=import-outside-toplevel | ||
| from llama_stack_configuration import migrate_config_dumb | ||
|
|
||
| try: | ||
| migrate_config_dumb(args.run_yaml, args.config_file, args.migrate_output) | ||
| logger.info( | ||
| "Migration complete. Wrote unified config to %s", | ||
| args.migrate_output, | ||
| ) | ||
| except Exception as e: | ||
| logger.error("Migration failed: %s", e) | ||
| raise SystemExit(1) from e | ||
| return |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Migration branch looks reasonable for a spike.
Deferred import avoids pulling synthesis code into the hot boot path, and broad except Exception is acceptable at the CLI boundary. One small nit: consider logger.exception(...) instead of logger.error("Migration failed: %s", e) so the traceback is captured — helpful when users report migration failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lightspeed_stack.py` around lines 150 - 165, The except block for the
migrate_config_dumb call should log the full traceback; replace the
logger.error("Migration failed: %s", e) call with logger.exception("Migration
failed") so the stack trace is captured when args.migrate_config triggers
migrate_config_dumb (refer to migrate_config_dumb and the surrounding try/except
using logger).
There was a problem hiding this comment.
Good nit. Implementation JIRA scope — the migration tool's error-handling polish goes into "Migration tool — dumb-mode lift-and-shift" once the PoC code is replaced.
There was a problem hiding this comment.
@max-svistunov, understood — makes sense to bundle that error-handling polish with the rest of the migration tool's production hardening rather than touching PoC code. Would you like me to open a GitHub issue to track the logger.exception() improvement as part of the "Migration tool — dumb-mode lift-and-shift" work?
| def apply_high_level_inference( | ||
| ls_config: dict[str, Any], inference: dict[str, Any] | ||
| ) -> None: | ||
| """Apply a high-level `inference` block into `ls_config['providers']['inference']`. | ||
|
|
||
| Replaces the inference provider list entirely. Use `native_override` for | ||
| additive tweaks. | ||
|
|
||
| Parameters: | ||
| ls_config: Llama Stack config dict (modified in place). | ||
| inference: High-level inference section as a dict (with 'providers' list). | ||
| """ | ||
| providers_out: list[dict[str, Any]] = [] | ||
| for provider in inference.get("providers", []): | ||
| p_type = provider["type"] | ||
| entry: dict[str, Any] = { | ||
| "provider_id": p_type, | ||
| "provider_type": PROVIDER_TYPE_MAP[p_type], | ||
| } | ||
| cfg: dict[str, Any] = {} | ||
| if provider.get("api_key_env"): | ||
| cfg["api_key"] = f"${{env.{provider['api_key_env']}}}" | ||
| if provider.get("allowed_models"): | ||
| cfg["allowed_models"] = provider["allowed_models"] | ||
| if provider.get("extra"): | ||
| cfg.update(provider["extra"]) | ||
| if cfg: | ||
| entry["config"] = cfg | ||
| providers_out.append(entry) | ||
|
|
||
| if "providers" not in ls_config: | ||
| ls_config["providers"] = {} | ||
| ls_config["providers"]["inference"] = providers_out |
There was a problem hiding this comment.
Do not use high-level provider type as the emitted provider_id.
For sentence_transformers, this emits provider_id: sentence_transformers, while the baseline and model/vector-store references use sentence-transformers. That leaves synthesized configs with references to a provider ID that no longer exists after providers.inference is replaced.
Normalize emitted provider IDs separately from provider types
PROVIDER_TYPE_MAP: dict[str, str] = {
"openai": "remote::openai",
"sentence_transformers": "inline::sentence-transformers",
@@
"vllm_rhel_ai": "remote::vllm",
}
+
+PROVIDER_ID_MAP: dict[str, str] = {
+ "openai": "openai",
+ "sentence_transformers": "sentence-transformers",
+ "azure": "azure",
+ "vertexai": "vertexai",
+ "watsonx": "watsonx",
+ "vllm_rhaiis": "vllm-rhaiis",
+ "vllm_rhel_ai": "vllm-rhel-ai",
+}
@@
p_type = provider["type"]
entry: dict[str, Any] = {
- "provider_id": p_type,
+ "provider_id": PROVIDER_ID_MAP[p_type],
"provider_type": PROVIDER_TYPE_MAP[p_type],
}There was a problem hiding this comment.
Already a known issue — captured in the spike doc's "Findings discovered during PoC" section (the high-level inference naming collision). The implementation JIRA "Unified llama_stack.config schema + synthesizer" will fix it; Decision S5 (added in de734e7, updated in 8b85c4a) confirms the approach is for each backend-specific synthesizer to translate LCORE's canonical type vocabulary (sentence_transformers Literal) to the target backend's shape (hyphenated provider_id for Llama Stack today; model-string prefixes for Pydantic AI tomorrow).
There was a problem hiding this comment.
@max-svistunov, thanks for the context — acknowledged. Since this is a spike and the PoC code will be removed before merge, and Decision S5 already captures the backend-specific translation approach (LCORE canonical sentence_transformers → hyphenated Llama Stack provider_id, with analogous handling for Pydantic AI), I'll leave this as a tracked divergence. No action needed here.
✏️ Learnings added
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: src/llama_stack_configuration.py:651-683
Timestamp: 2026-05-20T08:09:39.201Z
Learning: In `src/llama_stack_configuration.py`, the `apply_high_level_inference` function currently emits `provider_id: p_type` (underscore form, e.g. `sentence_transformers`) directly from the high-level type key, which collides with Llama Stack's hyphenated provider IDs (e.g. `sentence-transformers`). This is a known PoC divergence documented in the spike doc ("Findings discovered during PoC") and tracked in the implementation JIRA "Unified llama_stack.config schema + synthesizer". Decision S5 mandates that each backend-specific synthesizer translates LCORE's canonical type Literal vocabulary to the target backend's expected shape (hyphenated provider_id for Llama Stack; model-string prefixes for Pydantic AI). The PoC code will be removed before merge; the fix belongs in the implementation ticket.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| ls_config = synthesize_configuration(lcs_config, config_file_dir=config_file_dir) | ||
| logger.info("Writing synthesized Llama Stack configuration to %s", output_file) | ||
| Path(output_file).parent.mkdir(parents=True, exist_ok=True) | ||
| with open(output_file, "w", encoding="utf-8") as f: | ||
| yaml.dump(ls_config, f, Dumper=YamlDumper, default_flow_style=False) |
There was a problem hiding this comment.
Write synthesized configs with restrictive permissions.
native_override can contain literal secrets from a migrated run.yaml, so writing with default open(..., "w") can create a world-readable file depending on umask.
Use an explicit `0600` create mode
ls_config = synthesize_configuration(lcs_config, config_file_dir=config_file_dir)
logger.info("Writing synthesized Llama Stack configuration to %s", output_file)
Path(output_file).parent.mkdir(parents=True, exist_ok=True)
- with open(output_file, "w", encoding="utf-8") as f:
+ fd = os.open(output_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+ with os.fdopen(fd, "w", encoding="utf-8") as f:
yaml.dump(ls_config, f, Dumper=YamlDumper, default_flow_style=False)📝 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.
| ls_config = synthesize_configuration(lcs_config, config_file_dir=config_file_dir) | |
| logger.info("Writing synthesized Llama Stack configuration to %s", output_file) | |
| Path(output_file).parent.mkdir(parents=True, exist_ok=True) | |
| with open(output_file, "w", encoding="utf-8") as f: | |
| yaml.dump(ls_config, f, Dumper=YamlDumper, default_flow_style=False) | |
| ls_config = synthesize_configuration(lcs_config, config_file_dir=config_file_dir) | |
| logger.info("Writing synthesized Llama Stack configuration to %s", output_file) | |
| Path(output_file).parent.mkdir(parents=True, exist_ok=True) | |
| fd = os.open(output_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| with os.fdopen(fd, "w", encoding="utf-8") as f: | |
| yaml.dump(ls_config, f, Dumper=YamlDumper, default_flow_style=False) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/llama_stack_configuration.py` around lines 825 - 829, The file write is
using open(output_file, "w") which can create world-readable files; change the
write to create the file with explicit restrictive permissions (mode 0o600).
After ensuring the parent directory is created (the existing
Path(output_file).parent.mkdir call is fine), open the file using a low-level
create with os.open(...) with flags O_WRONLY|O_CREAT|O_TRUNC and mode 0o600,
wrap the returned fd with os.fdopen to get a text file object, then use
yaml.dump(ls_config, f, Dumper=YamlDumper, default_flow_style=False) to write;
reference the variables/functions ls_config, output_file,
synthesize_configuration, and YamlDumper so you update the existing block that
writes the synthesized configuration.
There was a problem hiding this comment.
Landed in the spec doc just now — R10 mandates file mode 0600 via an explicit create flag (not umask), and Security considerations acknowledges that native_override and the dumb migration tool may carry literal secrets, so the file can't be assumed secret-free. See 6bc7a72. The PoC code is scheduled for removal pre-merge; the implementation JIRA inherits R10's requirement.
There was a problem hiding this comment.
@max-svistunov, that's a clean resolution for a spike — capturing it as a hard spec requirement (R10: explicit O_CREAT | 0o600, no umask reliance) is exactly the right level of enforcement for implementation work rather than patching PoC code that's being deleted anyway. The acknowledgment in Security Considerations that both native_override and dumb-migration paths can carry literal secrets gives the implementer the necessary context to apply R10 correctly without assumptions. Happy to close this out on my end.
| cfg.library_client_config_path = None | ||
| with pytest.raises( | ||
| ValueError, | ||
| match="Configuration problem: library_client_config_path is not set", | ||
| match="neither .*unified.* nor .*legacy.* is set", | ||
| ): | ||
| client = AsyncLlamaStackClientHolder() | ||
| await client.load(cfg) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that load() (or a helper it calls) re-runs validation raising the expected message.
rg -nP --type=py -C4 '\b(class\s+AsyncLlamaStackClientHolder|async\s+def\s+load)\b'
rg -nP --type=py -C2 'neither\s+.*unified.*\s+nor\s+.*legacy'Repository: lightspeed-core/lightspeed-stack
Length of output: 1339
🏁 Script executed:
# Read the load() method and surrounding code
sed -n '40,100p' src/client.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 2701
🏁 Script executed:
# Read the complete test function context
grep -n "def test_" tests/unit/test_client.py | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 448
🏁 Script executed:
# Find the specific test around line 84-90
sed -n '75,95p' tests/unit/test_client.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 830
🏁 Script executed:
# Search for LlamaStackConfiguration class and check_llama_stack_model validator
rg -nP --type=py 'class\s+LlamaStackConfiguration|def\s+check_llama_stack_model' -A 20Repository: lightspeed-core/lightspeed-stack
Length of output: 3016
Consider testing validation at the model layer instead of relying on load() runtime checks.
The test passes, but not through Pydantic re-validation as suggested. The load() method does not re-run the check_llama_stack_model validator. Instead, the ValueError is raised by explicit runtime checks inside _load_library_client() (src/client.py:73-77). While this works today, the test would silently pass if those runtime checks are ever removed or refactored. A more robust test would construct the invalid config directly via LlamaStackConfiguration(...) inside pytest.raises to pin the expected validation behavior at the model layer where the check_llama_stack_model validator should enforce it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_client.py` around lines 84 - 90, Replace the current test
that relies on AsyncLlamaStackClientHolder().load(...) to trigger a runtime
ValueError with a direct validation test against the Pydantic model: construct
an invalid LlamaStackConfiguration(...) instance (setting
library_client_config_path=None and leaving both unified and legacy unset)
inside pytest.raises(ValueError, match="neither .*unified.* nor .*legacy.* is
set") so the check_llama_stack_model validator on the LlamaStackConfiguration
model is exercised; this ensures validation fails at model construction rather
than via AsyncLlamaStackClientHolder.load or _load_library_client runtime
checks.
There was a problem hiding this comment.
Acknowledged. PoC-side test; scheduled for removal pre-merge.
There was a problem hiding this comment.
@max-svistunov, understood — removing the PoC test alongside the rest of the PoC code is a clean resolution. No further action needed on this comment.
…entation) Incorporates reviewer request: the work on this feature kicks off with a Story that authors the behave `.feature` files for unified mode BEFORE the feature is implemented. The intent is to keep test-shape authorship free of implementation bias and to surface any architectural gaps early. Adds two JIRAs to the spike doc's proposed-JIRAs list, bringing the total from 7 to 9: 1. LCORE-???? (Story, inserted first) — E2E feature files for unified mode (no step implementation). Authors Gherkin scenarios against the spec doc's R1..R11 requirements. Explicitly forbids reading the implementation JIRAs or the synthesizer code while authoring. behave marks resulting steps as undefined; test-e2e still green (undefined scenarios are reported, not failed). 2. LCORE-???? (Task, inserted after the migrate-e2e-configs Story) — Implement behave step definitions for the kickoff feature files. Takes the Gherkin as-is (does not water down the tests to fit implementation). Blocked by the kickoff ticket plus the feature- implementation tickets (schema + synthesizer, migration tool, LS container entrypoint). Filing both tickets together (rather than filing only the kickoff and "letting the step-def ticket appear later") makes the dependency chain explicit from the start and ensures the step-def work is not forgotten. No other JIRAs change scope. The PR template is updated to reflect the new count and to widen the "Full JIRA list" link range to cover both new sections.
Per docs/contributing/howto-organize-poc-output.md (line 9), the convention is `poc-results/`. The LCORE-836 PoC bundle was written under `poc-evidence/` by mistake; rename to match the documented convention. Also updates references in the spike doc and the PoC README.
Revisions arising from a re-read of the spike doc. Spike doc: - Add a "Design options A-E" explainer section near the top so readers encounter the option short names + one-line summaries before any decision references them. Drop options D and F (previously dropped without scoring) entirely; renumber the survivors so letters are consecutive: old E (Profiles) -> new D, old G (Kustomize patches) -> new E. References throughout the doc updated accordingly. - Restructure Decision S1: one standalone option per row in the choice table, no more "B+C" / "C+E" composite labels. Intro prose reduced to a link to the scoring section; recommendation spells out C (base shape) + D (optional profile layer). - Replace Decision S2 with a single Q3/Q4 deprecation recommendation. - Simplify Decision S3 to the "anything apart from Konflux?" framing. - Decision T6: refer to profiles as "Option D layer" (was E). - Decision T7: define "round-trip" in plain English on first use; recommend `default` as the field's default value; remove the reviewer-naming question. - Decision T8: drop the Tekton-pipelines framing, Konflux only; name @radofuchs explicitly as the owner. - Delete Decision T9 entirely (it was informational, not a decision). Its content is now an expanded bullet under "Findings discovered during PoC" covering the underlying library-client API constraint, three concrete implementation consequences, and a closing note on why a dict-only path isn't worth pursuing. - PoC results section: - Drop the "Level 3'" reference (the other levels are never defined and the term carries no value here). - Rephrase the smart-migration bullet as deferred future work pointing at the spec doc's Open Questions. - Rewrite the provider_id naming-collision text in plain English. - Fix the evidence link block: was pointing at poc-evidence/library-mode/ (wrong directory after the earlier rename) and listed lightspeed-stack-unified-library.yaml as if it lived inside that subdir, whereas it actually sits one level up. Each file now linked at its real location. - Rename "Surprise discovered during PoC" -> "Findings discovered during PoC". - Remove the stale "(Decision T9)" attribution. - Rename "Current architecture (before LCORE-836)" -> "Current architecture" (the parenthetical added no information). - "Design alternatives considered": add intro paragraph that explicitly frames the table as scoring; expand each of the 12 attribute names from a short phrase to a full definition with high/low criteria; table columns renumbered A, B, C, D, E (was A, B+C, C+E, E, G); closing paragraph swaps "E layered on top of C" -> "D layered on top of C". - Overview's recommendation paragraph: link target updated from Design options A-G to A-E; Option E -> Option D. Spec doc: - Realign the deprecation schedule with the new spike S2 recommendation (Q3/Q4 warnings, end-of-Q4 removal). Trim "subject to PM confirmation against the actual release calendar." -> "subject to PM confirmation." - Rename the stale "see PoC surprise in the spike doc" reference to point at the new "Findings discovered during PoC" section.
tisnik
left a comment
tisnik
left a comment
There was a problem hiding this comment.
It looks ok as is. There's second approach: have abstracted inference+RAG sections on config file toplevel. It might allow us to ditch llama stack and replace it by XYZ library in future w/o the need to change config schema.
WDYT?
It will be a bit more problematic - basically it means that two config models will be used + some logic to translate between those models.
| |---|---| | ||
| | A (Embedded native) | `llama_stack.config` is raw LS schema, verbatim | | ||
| | B (High-level only) | LCORE-defined high-level keys; no escape hatch | | ||
| | **C (B + `native_override`)** | High-level keys + raw-LS escape hatch | |
| **Ask**: do we need to account for anything apart from Konflux? | ||
| Reviewers from downstream teams should flag any deployment surface that | ||
| treats `run.yaml` as a separate artifact (ConfigMap, templated file, | ||
| build-time asset) that the unified design would need to accommodate. |
There was a problem hiding this comment.
good question. Whom to ask? Major + anybody from Ansible, and RHDH? @sbunciak
|
|
||
| | Strategy | Example: `safety: {excluded_categories: [a, b]}` vs override `{excluded_categories: [c]}` | | ||
| |---|---| | ||
| | Deep-merge, append lists | result: `[a, b, c]` | |
There was a problem hiding this comment.
I hope it won't be needed, but yes, this sounds as the best approach.
| | **Keep env-var refs verbatim** | `api_key: ${env.OPENAI_API_KEY}` (resolved by LS at start) | | ||
| | Resolve before writing | `api_key: sk-...` | | ||
|
|
||
| **Recommendation**: **keep env-var refs verbatim**. Security-leaning default; |
There was a problem hiding this comment.
imho there's nothing to change, it should be supported right now (need to check)
…nchor
Spec doc:
- R6 reworded: it is true that LCORE itself does not resolve env refs
on disk, but `native_override` and the dumb migration tool can carry
literal secrets from a legacy `run.yaml`. The previous wording made
a stronger claim than the design can honour, so it has been narrowed
to "secrets LCORE emits are not resolved on disk".
- R10 strengthened: the synthesized file must be created with mode
0600 via an explicit create flag (not umask), so that when
`native_override` or a migrated `run.yaml` does carry a literal
secret, the file is not world-readable.
- "Security considerations" section rewritten to (a) drop the
inaccurate "no secrets written to disk" blanket statement,
(b) acknowledge that `native_override` and dumb-mode migration
output may contain literal secrets, (c) mandate 0600 on the
synthesized file, (d) recommend the migration tool's help / docs
advise operators to replace literal secrets with `${env.<VAR>}`
refs before or after migration, (e) note that LS's own
`replace_env_vars()` (in `llama_stack.core.library_client`) is
the in-memory resolver at LS startup.
Spike doc:
- Fix broken in-page anchor on line 166: the target heading is
"### Merge semantics — worked examples", and GitHub renders that
heading's anchor with a double hyphen
(`#merge-semantics--worked-examples`) because the em-dash is
stripped and the surrounding spaces both become hyphens. Link
display text now matches the heading exactly as well.
CodeRabbit flagged twelve fenced code blocks in the spike doc that were missing blank lines before or after them (markdownlint MD031). Applied via `markdownlint-cli2 --fix`. No content change — purely blank-line insertions around `\`\`\`text` and `\`\`\`yaml` fences in the Proposed JIRAs section and Appendix B. Line numbers of section headings (S1-T8, ## Proposed JIRAs) are unchanged. ## PoC results moved from L614 to L642 because the auto-fix added blank lines inside the JIRAs section that precedes it.
In response to @tisnik's review comment about future-proofing the unified config schema against a backend swap, plus the project direction to migrate from Llama Stack to Pydantic AI over time. S5 extends S1 — it does NOT replace it. Option C + optional D remains the recommended overall shape. S5 only decides *where* the backend-agnostic high-level keys (`inference.providers` today; later `rag.providers`) live in the operator-facing YAML hierarchy: at the top level, not under the `llama_stack` subtree. LS-specific knobs (`native_override`, `profile`, `baseline`) stay under `llama_stack.config` unchanged. Confidence is 70%; the recommendation is provisional until a research pass on Pydantic AI's actual config surface lands. A TODO is captured in the decision body so the spike author resolves the per-provider `type:` vocabulary question post-research. If S5 is adopted, the existing "Unified llama_stack.config schema + synthesizer" implementation JIRA's scope shifts to ship the top-level shape from day one. No new JIRA is added.
Spike doc:
- Adds a new finding to "Findings discovered during PoC" calling
out that the PoC's safety-shield validation was vacuous: the
`native_override` registered `llama-guard` with
`provider_shield_id: openai/gpt-4o-mini` — an OpenAI chat model,
not a Llama Guard checkpoint, so the evidence row "`native_override`
took effect" only proves the key landed, not that a real shield
gated any query. Caught by CodeRabbit on
`poc-results/library-mode/synthesized-run.yaml:110`. The
implementation JIRA's e2e coverage must exercise a real Llama
Guard model (e.g. `meta-llama/Llama-Guard-3-8B`).
- Updates Decision S5 with the Pydantic AI research findings from
2026-05-20:
- Drops the "future `rag.providers`" lift from the proposed table;
research confirms Pydantic AI has no RAG / vector-store
abstraction and no public roadmap signal one is coming in 6–12
months. RAG, safety/shields, vector storage all stay under
`llama_stack.config`.
- Adds a "Pydantic AI research findings" subsection citing the
full report (now in `poc-results/pydantic-ai-research.md`)
and the researcher's confidence numbers per concept: ~75% for
`inference`, ~25% for RAG, ~20% for safety, ~60% for MCP.
- Resolves the `inference.providers[].type` vocabulary TODO:
keep LCORE's existing Literal vocabulary
(`openai`, `azure`, `sentence_transformers`, …); each
backend-specific synthesizer translates to its target shape.
- Raises decision confidence from 70% to 75%; reservation is
now research-bounded (Pydantic AI pre-V2 minor-surface churn,
very low per their stated policy) rather than
information-gap-bounded.
- Adds a note that Pydantic AI V2 timing ("April 2026 at the
earliest" — has not shipped as of 2026-05-20) likely overlaps
LCORE's migration window; re-validate when V2 ships.
Adds the full research report under
`poc-results/pydantic-ai-research.md` (Llama Stack ↔ Pydantic AI
concept mapping, dated 2026-05-20) so the spike doc's link to it
resolves and so the evidence travels with the spike.
Incorporated (though not as a second approach, but as extension of the current proposal). PTAL at decision S5. Things for you to PTAL at: S5 specifically (the new decision):
Also please see if you have any comments on:
|
|
@sbunciak Could you PTAL?
|
|
@radofuchs FYI, T8 has moved to L335-L344 after Decision S5 was added in 8b85c4a. Updated link: https://github.com/max-svistunov/lightspeed-stack/blob/lcore-836-spike-llama-stack-config-merge/docs/design/llama-stack-config-merge/llama-stack-config-merge-spike.md?plain=1#L335-L344 |
|
@sbunciak We're using lightspeed-stack/llama-stack in library mode now, but we can switch to server mode if needed or reorganize how we handle |
4 participants
LCORE-836: merge run.yaml into lightspeed-stack.yaml (spike)
Spike deliverable for LCORE-836 — single-file operator-facing configuration for Lightspeed Core.
What's in this PR
Design docs (
docs/design/llama-stack-config-merge/):llama-stack-config-merge-spike.md(use the "Outline" button) — the spike: research, design alternatives, PoC results, reviewer decisions (S1–S5, T1–T8), proposed JIRAsllama-stack-config-merge.md(use the "Outline" button) — feature spec — changeable based on final decisions, will be kept in the repo long-termPoC code (will stay in-branch for review; see "Before merge" below):
src/models/config.py—UnifiedLlamaStackConfig,UnifiedInferenceSection,UnifiedInferenceProvider+ modifiedLlamaStackConfiguration(mutual-exclusion validator)src/llama_stack_configuration.py—synthesize_configurationpipeline,deep_merge_list_replace,apply_high_level_inference,load_default_baseline,synthesize_to_file,migrate_config_dumb; CLI auto-detectsrc/data/default_run.yaml— shipped default baseline (note: needs slimming before production; see findings Python project structure #4)src/client.py— library-mode wiring branches on unified vs legacysrc/lightspeed_stack.py—--migrate-config/--run-yaml/--migrate-outputflagstest.containerfile— copiessrc/data/into the LS containertests/unit/test_llama_stack_synthesize.py— 22 new tests (synth + migration round-trip)config: Nonefield in dump expectations and error-message regexPoC evidence (
docs/design/llama-stack-config-merge/poc-results/):lightspeed-stack.yamlused for validationrun.yamlproduced at boot/v1/queryresponse JSONMain findings
lightspeed-stack.yamlwithllama_stack.config(no externalrun.yaml) boots LCORE in library mode, serves/v1/query, and appliesnative_overridevalues correctly. 2098 unit tests pass including a lossless migrate-then-synthesize round-trip.llama_stack.library_client_config_pathand the newllama_stack.configare mutually exclusive and coexist through a deprecation window. Migration tool (lightspeed-stack --migrate-config) produces a lossless unified file from the legacy pair.AsyncLlamaStackAsLibraryClientinllama-stack. Consequence: library mode must write the synthesized file to disk. Noted in "Findings discovered during PoC" in the spike doc.${env.EXTERNAL_PROVIDERS_DIR}requirement from the repo'srun.yaml(the PoC uses it verbatim). Implementation JIRA should slim the shipped baseline or change the reference to a default-aware form like${env.EXTERNAL_PROVIDERS_DIR:=~/.llama/providers.d}. Flagged in the spike doc's "Findings discovered during PoC" section.sentence_transformers(underscore) differs from the baseline'ssentence-transformers(hyphen) that other parts of the baseline reference. Per Decision S5, each backend-specific synthesizer translates LCORE's canonical vocabulary; implementation JIRA will fix the LS-side translation.poc-results/library-mode/synthesized-run.yaml:110. The PoC'snative_overrideregisteredllama-guardwithprovider_shield_id: openai/gpt-4o-mini, a chat model not a Llama Guard checkpoint. Implementation JIRA's e2e coverage must exercise a real Llama Guard model.For reviewers
Strategic decisions — @sbunciak (PM) / @tisnik:
poc-results/pydantic-ai-research.md.)Technical decisions — @tisnik / team leads (T8 — @radofuchs):
baselinefield (added during PoC)Proposed JIRAs — review scope and ordering:
Doc structure note: The decisions and proposed-JIRAs sections of the spike doc are where your input is needed. They link to background sections later in the doc (current architecture, design alternatives, merge-semantics worked examples, backward-compat scope) — read those if you need more context on a specific point, but it is optional.
Before merge
Per the spike howto, step 10: once decisions are confirmed and JIRAs are filed (via
dev-tools/file-jiras.sh), the following are expected to be removed from this branch prior to merge:src/(schema, synthesizer, library-mode wiring, migration tool) — becomes implementation in the filed JIRAstests/— becomes part of the implementation JIRAssrc/data/default_run.yaml— becomes part of the implementation JIRAtest.containerfilechange — becomes part of the implementation JIRAdocs/design/llama-stack-config-merge/poc-results/(the Pydantic AI research report stays — it is referenced from Decision S5)The spike doc and the spec doc stay in the repo.
Tools used to create PR
Related Tickets & Documents
Checklist