refactor(12C): load retrieval/query prompts via core loader, drop components.prompts - #437
Conversation
…ts.prompts Phase 12C. The orchestrators imported HYDE/MULTI_QUERY/SYS/CONTEXTUALIZER/ SPOKEN_STYLE prompts from the components.prompts shim. Those are not Python string constants — they are disk templates the shim eager-loads at import via load_config(). Distributing them as eager module-level constants into core/prompts builders would add import-time disk I/O and a config dependency to the whole core.prompts package (its __init__ imports every builder), so instead each service loads its templates from the injected Settings using the existing pure core.prompts.load_template_by_key(prompts_dir, mapping, key) — exactly the call shape template_loader documents for its callers. - retrieval_service.py: multiQuery / hyde branches load "multi_query" / "hyde" from `config` (already in __init__) instead of importing the shim inline. - query_service.py: load "query_contextualizer" / "spoken_style_answer" / "sys_prompt" once in __init__ from the injected config; store on the instance. Per-instance config binding replaces the old global eager load. - test_query_service.py: extend the fake config with the real paths/prompts so QueryService can resolve templates. No remaining `components.prompts` imports outside components/ itself.
📝 WalkthroughWalkthroughThis PR migrates ChangesPrompt Template Dynamic Loading
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/services/orchestrators/test_query_service.py (1)
22-24: 💤 Low valueOptional: defer
load_config()to a fixture instead of import time.Calling
load_config()at module scope runs disk I/O during test collection and couples this module's import to a resolvable config. A session-scoped fixture (or lazy call inside_config()) keeps collection cheap and failures localized to the tests that need real prompts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openrag/services/orchestrators/test_query_service.py` around lines 22 - 24, The module currently calls load_config() at import time via the _PROMPT_CFG variable which causes disk I/O during test collection; change this to a lazily-evaluated or fixture-based approach by removing the module-scope _PROMPT_CFG = load_config() and either (a) add a session-scoped pytest fixture (e.g., def prompt_cfg_session(): return load_config()) and update tests to accept that fixture, or (b) replace _PROMPT_CFG with a helper function (e.g., def _prompt_cfg(): return load_config()) and call it inside tests/setup where needed; update references to _PROMPT_CFG accordingly (tests that currently reference _PROMPT_CFG should use the fixture name or call _prompt_cfg()) so disk I/O happens only when those tests run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openrag/services/orchestrators/test_query_service.py`:
- Around line 22-24: The module currently calls load_config() at import time via
the _PROMPT_CFG variable which causes disk I/O during test collection; change
this to a lazily-evaluated or fixture-based approach by removing the
module-scope _PROMPT_CFG = load_config() and either (a) add a session-scoped
pytest fixture (e.g., def prompt_cfg_session(): return load_config()) and update
tests to accept that fixture, or (b) replace _PROMPT_CFG with a helper function
(e.g., def _prompt_cfg(): return load_config()) and call it inside tests/setup
where needed; update references to _PROMPT_CFG accordingly (tests that currently
reference _PROMPT_CFG should use the fixture name or call _prompt_cfg()) so disk
I/O happens only when those tests run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0678379b-2970-4438-bd71-ab91d46ec10b
📒 Files selected for processing (3)
openrag/services/orchestrators/query_service.pyopenrag/services/orchestrators/retrieval_service.pyopenrag/services/orchestrators/test_query_service.py
Phase 12C — Migrate prompt constants off
components.promptsRemoves the last
from components.prompts import …consumers so the shim can be deleted in 12H.Design note — deviation from the guide's letter, faithful to its intent
The 12C plan assumed these were Python string constants to inline into
core/prompts/builders. They are not —components/prompts/prompts.pyis a shim whose "constants" (HYDE_PROMPT,MULTI_QUERY_PROMPT,SYS_PROMPT_TMPLT,QUERY_CONTEXTUALIZER_PROMPT,SPOKEN_STYLE_ANSWER_PROMPT) are disk templates eager-loaded viaload_config()at import.Distributing them as eager module-level constants into the builders would add import-time disk I/O + a config dependency to the entire
core.promptspackage (its__init__imports every builder) — an architectural regression that also breakscore's import-time purity.Instead, each orchestrator loads its templates from the injected
Settingsusing the existing purecore.prompts.load_template_by_key(prompts_dir, mapping, key)— precisely the call shapetemplate_loader's docstring prescribes for callers. This removes the shim dependency, keepscorepure, and binds prompts to the instance's config (more correct than the old process-global eager load). The real per-template-file migration remains Phase 13D.Changes
retrieval_service.py—multiQuery/hydebranches load"multi_query"/"hyde"fromconfig(already a constructor arg) instead of the inline shim import.query_service.py— load"query_contextualizer"/"spoken_style_answer"/"sys_prompt"once in__init__from the injected config, stored on the instance; use sites updated.test_query_service.py— extend the fakeSimpleNamespaceconfig with the realpaths/promptssoQueryServicecan resolve templates.Verification
grep "components.prompts"outsidecomponents/→ zero resultsruff checkon all changed files → cleantest_query_service.py+test_retrieval_service.py+core/prompts/→ 55 passedSummary by CodeRabbit