RSPEED-2408: fix(rlsapi): use customization.system_prompt in /v1/infer#1125
Conversation
Walkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
9451ea4 to
0845489
Compare
|
The e2e failure looks unrelated 🤔 |
- Check configuration.customization.system_prompt before falling back to constants.DEFAULT_SYSTEM_PROMPT in _build_instructions() - Add unit tests for custom prompt, None prompt, and None customization Signed-off-by: Major Hayden <major@redhat.com>
0845489 to
aff7004
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/app/endpoints/test_rlsapi_v1.py (1)
155-167:⚠️ Potential issue | 🟠 Major
test_build_instructionslacks mocking forconfiguration— tests will fail.The existing test at lines 155–167 has no
mockerparameter and doesn't patchconfiguration, but_build_instructionsnow accessesconfiguration.customizationat runtime. This will hit the uninitialized singleton and raise aLogicError.The two new tests correctly mock
configurationusingmocker.patch(). The existing parametrized test should either:
- Accept
mocker: MockerFixtureand patchconfigurationbefore calling_build_instructions, or- Use the
mock_configurationfixture already defined in this test file (line 76)
🧹 Nitpick comments (1)
src/app/endpoints/rlsapi_v1.py (1)
95-101: Consider caching theconfiguration.customizationaccess in a local variable.
configuration.customizationis accessed twice, and each access goes through the property getter (which includes a None-check on_configuration). A local variable avoids the redundant property call and reads more cleanly.♻️ Suggested refactor
- if ( - configuration.customization is not None - and configuration.customization.system_prompt is not None - ): - base_prompt = configuration.customization.system_prompt - else: - base_prompt = constants.DEFAULT_SYSTEM_PROMPT + customization = configuration.customization + if customization is not None and customization.system_prompt is not None: + base_prompt = customization.system_prompt + else: + base_prompt = constants.DEFAULT_SYSTEM_PROMPT
Description
The
/v1/inferendpoint hardcodesconstants.DEFAULT_SYSTEM_PROMPT("You are a helpful assistant") in_build_instructions(). The/v1/queryand/v1/streaming_queryendpoints already support configurable system prompts viacustomization.system_promptinlightspeed-stack.yaml, but/v1/inferignores this config entirely.This change makes
_build_instructions()checkconfiguration.customization.system_promptfirst, falling back toDEFAULT_SYSTEM_PROMPTwhen unset. Follows the same precedence pattern asget_system_prompt()insrc/utils/prompts.py, simplified for rlsapi (no per-request prompt, no custom profiles).Type of change
Tools used to create PR
Related Tickets & Documents
Checklist before requesting a review
Testing
uv run pytest tests/unit/app/endpoints/test_rlsapi_v1.py -v -k "build_instructions"— all 6 tests pass (3 existing + 3 new)uv run make verify— all linters cleantest_build_instructions_customization_system_prompt_set— custom prompt used when configuredtest_build_instructions_customization_system_prompt_none— falls back to DEFAULT_SYSTEM_PROMPT when system_prompt is Nonetest_build_instructions_no_customization— falls back when customization itself is NoneSummary by CodeRabbit
New Features
Tests