Skip to content

LCORE-3370: apply enrichment after the native_override merge - #2449

Open
max-svistunov wants to merge 1 commit into
lightspeed-core:mainfrom
max-svistunov:lcore-3370-enrichment-after-override
Open

LCORE-3370: apply enrichment after the native_override merge#2449
max-svistunov wants to merge 1 commit into
lightspeed-core:mainfrom
max-svistunov:lcore-3370-enrichment-after-override

Conversation

@max-svistunov

Copy link
Copy Markdown
Contributor

Description

Fixes LCORE-3370: after --migrate-config, synthesis silently lost enrichment whenever the original legacy run.yaml carried list-shaped sections. Dumb migration lifts the whole run.yaml into llama_stack.config.native_override, and the synthesis pipeline applied enrichment to the baseline before deep-merging the override with list-replacement semantics — so the merge replaced the enriched lists wholesale: BYOK and Solr/OKP vector_io providers, registered embedding models, and the Azure model_validation=false tweak were all dropped, contradicting migrate_config_dumb's documented "enrichment keeps working" promise and R7-after-migration.

The fix reorders synthesize_configuration so enrichment is the last content step: baseline → high-level inference.providers expansion → MCP ensure (non-empty baselines) → native_override merge → enrichment → dedupe. This is exactly legacy mode's semantics — enrichment always post-processes the operator's final run.yaml, which never beats it — so R5 keeps meaning "the override wins over the baseline and the high-level expansion" while R7 parity now also holds for migrated configs. The dumb-migration round-trip (T7, no enrichment inputs) is order-insensitive and unaffected.

Bonus: the reorder fixes a latent same-family bug — azure enrichment used to run before apply_high_level_inference, so a high-level azure provider entry replaced the enriched provider and silently dropped model_validation=false even without migration involved.

Also included: the strict xfail that documented the defect (added in LCORE-2747, PR #2318 — now merged) flips to a plain assertion; the test's azure_entra_id fixture moves to the current model schema (client_secretclient_secret_path no longer exists post-OGX-rename, and the xfail had been failing partly for that stale reason); spec R5 wording + changelog record the ordering change.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: Claude Opus 4.8
  • Generated by: Claude Opus 4.8

Related Tickets & Documents

  • Related Issue # LCORE-2747, LCORE-2337, LCORE-2336
  • Closes # LCORE-3370

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

Rebased onto current main (PR #2318 merged, so this is a one-commit diff); all results below are from the rebased head:

  1. The regression test that found the bug (formerly a strict xfail, now a plain assertion):
    uv run pytest tests/integration/test_unified_synthesis.py::test_migrate_then_synthesize_preserves_enrichment_parity -v
    Expected: migrate-then-synthesize over a legacy pair with BYOK + Solr/OKP + Azure enrichment equals the legacy enrichment output exactly (dict equality).
    Actual: passes.
  2. The full synthesis surface (integration + synthesizer unit tests):
    uv run pytest tests/integration/test_unified_synthesis.py tests/unit/test_llama_stack_synthesize.py
    Actual: 64 passed — including the T7 round-trip, R7 profile-parity for all enrichment kinds, native_override replacement semantics, and every ordering-sensitive unit case, unchanged.
  3. Full unit suite: uv run pytest tests/unit
    Actual: 3212 passed, 1 skipped.
  4. uv run make format — clean. Full make verify deferred to CI (local machine constraint).

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@max-svistunov, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 934c38ab-d473-42fc-96b0-cb4212af3d46

📥 Commits

Reviewing files that changed from the base of the PR and between da64733 and 47544b3.

📒 Files selected for processing (3)
  • docs/design/llama-stack-config-merge/llama-stack-config-merge.md
  • src/llama_stack_configuration.py
  • tests/integration/test_unified_synthesis.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Dumb migration lifts the entire legacy run.yaml into
llama_stack.config.native_override, and synthesis applied enrichment to
the baseline BEFORE deep-merging the override with list-replacement
semantics — so for any migrated config whose original run.yaml carried
the usual list-shaped sections, the merge replaced the enriched lists
wholesale: BYOK and Solr/OKP vector_io providers, registered embedding
models, and the Azure model_validation tweak were silently dropped,
contradicting migrate_config_dumb's enrichment-keeps-working promise and
R7-after-migration.

Reorder the synthesis pipeline so enrichment is the last content step:
baseline -> high-level inference expansion -> ensure MCP (non-empty
baselines) -> native_override merge -> enrichment -> dedupe. This is
exactly legacy mode's semantics — enrichment always post-processes the
operator's final run.yaml and the run.yaml never beats it — so R5 keeps
meaning "the override wins over the baseline and the high-level
expansion" while R7 parity now also holds for migrated configs. The
reorder incidentally fixes a latent same-family bug: azure enrichment
used to run before apply_high_level_inference, so a high-level 'azure'
provider entry replaced the enriched provider and dropped
model_validation=false even without migration involved.

The strict xfail documenting the defect flips to a plain assertion, and
the test's azure_entra_id fixture moves to the current model schema
(client_secret; client_secret_path no longer exists post-rename — the
xfail had been failing partly for that stale reason). Spec R5 wording
and the changelog record the ordering change.

The migrate round-trip (T7) is unaffected: with no enrichment inputs the
pipeline is order-insensitive, and test_migrate_then_synthesize_reproduces_run_yaml
still passes along with the full unit suite (3138) and the synthesis
integration module (63).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant