Add an option to disable query system prompt #216
Conversation
WalkthroughA new configuration option, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Config
User->>API: Send query (may include system_prompt)
API->>Config: Check customization & disable_query_system_prompt
alt disable_query_system_prompt is False
alt Query includes system_prompt
API->>API: Use query's system_prompt
else Query does not include system_prompt
API->>API: Use configured system_prompt
end
else disable_query_system_prompt is True
API->>API: Use configured system_prompt
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ 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). (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
omertuc
force-pushed
the
disablequerysystemprompt
branch
from
f6ddc98 to
dc4a394
Compare
omertuc
force-pushed
the
disablequerysystemprompt
branch
2 times, most recently
from
0592868 to
bc71711
Compare
omertuc
changed the title
manstis
left a comment
manstis
left a comment
There was a problem hiding this comment.
LGTM 👍
Just a nit-pick for you to consider.
src/models/config.py
Outdated
| class Customization(BaseModel): | ||
| """Service customization.""" | ||
|
|
||
| disable_query_system_prompt: Optional[bool] = False |
There was a problem hiding this comment.
Nit picking.
Most of other bool fields are named xxxx_enabled or xxxx_disabled.
For consistency it might be nicer to name this query_system_prompt_disabled.
lightspeed-stack/src/models/config.py
Line 31 in bc71711
lightspeed-stack/src/models/config.py
Line 91 in bc71711
lightspeed-stack/src/models/config.py
Line 93 in bc71711
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
/lgtm
TBH, I don't know why we have it on the query
|
If query_system_prompt_disabled is set to true, and we didn't configure a system prompt, what will the agent use? |
yes |
|
@eranco74 when I was doing the /query, I mimic what was available in the road-core/service /query endpoint. And the system_prompt was a parameter. Now, if nobody uses it and if we see it as a problem. Let's get rid of it before there's any release instead of having 3 different ways of configurating the system_prompt ? What you folks think ? I think we should make things simple and straight-forward, adding a new config to disable something in the API makes the use of that strange. With this patch, when the config is enabled and the user make a request to the /query endpoint with the system_prompt set, the service will return 200 saying the request was accepted but in reality part of it was ignored. It sounds off to me. |
|
The only advantage I can think of, of having the system prompt in the query, is that it allows people to create a wrapper service around lightspeed stack which will act as e.g. multiple different chat bots, backed by a single lightspeed stack instance (and that wrapper can freely vary the prompt). So it's not entirely useless. I make no such requirement myself, but I'm just putting this idea out there in case you want to consider it. I have no strong opinion either way |
| ) | ||
|
|
||
|
|
||
| def get_system_prompt(query_request: QueryRequest, configuration: AppConfig) -> str: |
There was a problem hiding this comment.
So now, if user makes a request to the API with the system_prompt set. The service will return 200 saying it was accepted but, if this new option is enabled. The request was just partially accept right ? As part of it will be ignored.
I don't like it, I think that we should just get rid of the system_prompt parameter in the /query endpoint. Make the service simple, because now we have too many ways of configuring/overriding the same thing.
There was a problem hiding this comment.
So now, if user makes a request to the API with the system_prompt set. The service will return 200 saying it was accepted but, if this new option is enabled. The request was just partially accept right ? As part of it will be ignored.
Yes
I don't like it
I don't love it either, it was an arbitrary decision, I can make it an error
I think that we should just get rid of the system_prompt parameter in the /query endpoint. Make the service simple, because now we have too many ways of configuring/overriding the same thing.
I'm also fine with this, but I've proposed a situation why it might make sense in the comment above, so you might want to reconsider. FWIW I have no stakes in this
Right, I can see that too but, I don't think that's a requirement for any Lightspeed-* at the moment. Seems a bit of over engineering if we allow it for now. If someone come up with a requirement for that, then yes, we can add it. But doing it prior to any requirements seems not worth it. |
|
Ok so speaking with other folks, the "system_prompt" parameter does have some usage. We then agreed on raising an error when the option that disable the system_prompt is enabled and a user makes a request with the system_prompt set. That would solve the issue and makes the API usage clear from the user perspective. Thanks for the patch! |
|
FWIW, the Ansible Chatbot makes use of There are probably other approaches that could be taken if it was removed from |
|
Thanks @manstis for the feedback. Yeah, I can see the benefit of it now. Speaking with @omertuc, we decided to throw an error when the option to disable query_system_prompt is set and the user sends a request with a system_prompt. I think that's fair because, we don't just sneakely change or ignore parts of the user request and also we keep the endpoint for the cases like yours. Does that work for you ? |
@umago Yes, absolutely. Thank-you. |
omertuc
force-pushed
the
disablequerysystemprompt
branch
2 times, most recently
from
b63ffc6 to
748bd98
Compare
![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
🧹 Nitpick comments (1)
tests/unit/utils/test_endpoints.py (1)
80-142: Consider adding edge case tests for completeness.The current test suite is comprehensive, but consider adding tests for:
- When
disable_query_system_promptisTruebut no configured system prompt is set (should fall back to default)- When
disable_query_system_promptis explicitly set toFalse(should behave like the default case)def test_get_system_prompt_with_disable_query_system_prompt_no_config( config_without_system_prompt, query_request_with_system_prompt ): """Test that default system prompt is used when query system prompt is disabled and no config is set.""" # Modify config to disable query system prompt config_without_system_prompt.customization = type('obj', (object,), { 'system_prompt': None, 'disable_query_system_prompt': True })() with pytest.raises(HTTPException) as exc_info: endpoints.get_system_prompt( query_request_with_system_prompt, config_without_system_prompt, ) assert exc_info.value.status_code == 422
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)src/models/config.py(1 hunks)src/utils/endpoints.py(1 hunks)tests/unit/__init__.py(1 hunks)tests/unit/test_configuration.py(1 hunks)tests/unit/utils/test_endpoints.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/utils/endpoints.py
- src/models/config.py
- README.md
- tests/unit/init.py
- tests/unit/test_configuration.py
⏰ 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). (1)
- GitHub Check: build-pr
🔇 Additional comments (8)
tests/unit/utils/test_endpoints.py (8)
5-5: Good addition of HTTPException import.The import is necessary for testing the error handling behavior of the new feature.
14-14: Excellent extraction of constant for reuse.Using a module-level constant for the configured system prompt improves maintainability and reduces duplication across test fixtures.
26-65: Well-designed fixtures for comprehensive test coverage.The three configuration fixtures properly cover different scenarios:
- No customization
- Custom system prompt only
- Custom system prompt with disabled query system prompt
The fixture design follows pytest best practices and ensures proper test isolation.
68-77: Clean and focused query request fixtures.The fixtures provide clear separation between requests with and without system prompts, making test intentions explicit.
80-118: Comprehensive coverage of core functionality.The first four test functions properly validate the existing system prompt selection logic:
- Default prompt when no customization
- Configured prompt when no query prompt
- Query prompt takes precedence over default
- Query prompt takes precedence over configured prompt
The test logic is correct and assertions are appropriate.
120-130: Excellent error handling test for the new feature.The test correctly validates that an HTTPException with status code 422 is raised when a query system prompt is provided while the feature is disabled. This ensures proper API contract enforcement.
133-142: Good validation of allowed behavior with disabled feature.The test confirms that queries without system prompts are still allowed when the feature is disabled, and the configured system prompt is properly used. This ensures the feature doesn't break normal operation.
9-9: Import path for config_dict verified
config_dictis defined and exported intests/unit/__init__.py, sofrom tests.unit import config_dictis valid. No changes needed.
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Other than the pylint error, the code LGTM now
Since LCORE-293 (e60d7df), users can set specify the system prompt in the config, but users can still override it with their queries. This may not be desired in some cases, so this commit adds an option to disable the query system prompt and always use the configured system prompt (or the default one if none is set). If a user tries to use the query system prompt when this option is enabled, an HTTP 422 error is returned with a message explaining that the query system prompt is not supported in this instance.
omertuc
force-pushed
the
disablequerysystemprompt
branch
from
748bd98 to
ecb63cb
Compare
5 participants
Description
Since LCORE-293 (e60d7df), users can set specify the system prompt in the config, but users can still override it with their queries. This may not be desired in some cases, so this commit adds an option to disable the query system prompt and always use the configured system prompt (or the default one if none is set). If a user tries to use the query system prompt when this option is enabled, an HTTP 422 error is returned with a message explaining that the query system prompt is not supported in this instance.
Type of change
Related Tickets & Documents
LCORE-293
Checklist before requesting a review
Testing
I made sure the query prompt was ignored
Summary by CodeRabbit
Summary by CodeRabbit