[RHDHPAI-976] Add customization profiles via path #487
Conversation
WalkthroughAdds support for loading custom prompt profiles from Python modules, integrates a CustomProfile into configuration and system-prompt selection precedence, extends OpenAPI and README with profile/path/literal examples, adds dynamic-import and validation utilities, and includes tests and test profiles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant AppConfig
participant Customization
participant Checks as utils.checks
participant CustomProfile
rect rgb(247,250,255)
note over User,Customization: Configuration load with profile_path
User->>AppConfig: Load YAML config
AppConfig->>Customization: Initialize customization
alt profile_path provided
Customization->>Checks: import_python_module(name, path)
Checks-->>Customization: module or None
Customization->>Checks: is_valid_profile(module)
Checks-->>Customization: True/False
alt valid profile
Customization->>CustomProfile: Create(path)
CustomProfile-->>Customization: prompts loaded from PROFILE_CONFIG.system_prompts
Customization->>Customization: set custom_profile
else invalid
Customization-->>Customization: skip custom_profile
end
else no profile_path
Customization-->>Customization: use existing system_prompt/path
end
end
sequenceDiagram
autonumber
participant Client
participant Endpoint as get_system_prompt
participant Req as QueryRequest
participant Cust as Customization
participant Prof as CustomProfile
participant Cfg as AppConfig
Client->>Endpoint: Resolve system prompt
Endpoint->>Req: check request.system_prompt
alt request override present and allowed
Endpoint-->>Client: return request.system_prompt
else
Endpoint->>Cust: check customization.custom_profile
alt custom_profile present
Cust->>Prof: get_prompts()
Prof-->>Endpoint: { default: "..." }
Endpoint-->>Client: return prompts.default
else
Endpoint->>Cust: check customization.system_prompt
alt present
Endpoint-->>Client: return customization.system_prompt
else
Endpoint->>Cfg: fallback to global defaults
Endpoint-->>Client: return fallback
end
end
end
note over Endpoint,Cust: Precedence: request -> custom_profile -> customization.system_prompt -> global
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (18)
src/utils/checks.py (2)
42-51: Strengthen profile validation a bitOptionally ensure system_prompts is a dict and contains a "default" entry to avoid empty/ill-formed configs.
Apply:
def is_valid_profile(profile_module: ModuleType) -> bool: """Validate that a profile module has the required PROFILE_CONFIG structure.""" if not hasattr(profile_module, "PROFILE_CONFIG"): return False profile_config = getattr(profile_module, "PROFILE_CONFIG", {}) - if not isinstance(profile_config, dict): + if not isinstance(profile_config, dict): return False - return "system_prompts" in profile_config + system_prompts = profile_config.get("system_prompts") + if not isinstance(system_prompts, dict): + return False + return "default" in system_prompts
24-29: Review import helper exception handling
Tests forimport_python_moduleintests/unit/utils/test_checks.pyassert it returnsNoneon non-Python files and aModuleTypeon valid.pymodules—no tests expect it to raise. If we want callers to rely solely on itsOptional[ModuleType]return rather than exceptions, catch and swallowspec.loader.exec_moduleerrors insideimport_python_moduleand returnNone.docs/openapi.json (3)
1251-1254: Mark path as a file pathImprove schema fidelity: add format "file-path" for CustomProfile.path, consistent with other path properties.
- "path": { - "type": "string", - "title": "Path" - }, + "path": { + "type": "string", + "format": "file-path", + "title": "Path" + },
1273-1283: Add file-path format to profile_pathAlign with other path-typed fields.
- "profile_path": { - "anyOf": [ - { - "type": "string" - }, + "profile_path": { + "anyOf": [ + { + "type": "string", + "format": "file-path" + },
1312-1321: Clarify precedence and mutual exclusivity for custom profile vs profile_pathIf both are provided, define precedence or make them mutually exclusive via oneOf + not to prevent ambiguity.
Proposed shape:
- "custom_profile": { - "anyOf": [ - { "$ref": "#/components/schemas/CustomProfile" }, - { "type": "null" } - ] - } + "custom_profile": { + "anyOf": [ + { "$ref": "#/components/schemas/CustomProfile" }, + { "type": "null" } + ] + } + }, + "allOf": [ + { + "not": { + "required": ["profile_path", "custom_profile"] + } + } + ]Confirm intended precedence (custom_profile vs profile_path) and I can update schema accordingly.
README.md (4)
348-349: Fix typos and tighten phrasing
- selceted → selected
- Remove “various different” redundancy
-The service uses the, so called, system prompt to put the question into context before the question is sent to the selceted LLM. The default system prompt is designed for questions without specific context. It is possible to use a different system prompt through various different avenues available in the `customization` section: +The service uses the so-called system prompt to put the question into context before the question is sent to the selected LLM. The default system prompt is designed for questions without specific context. You can use a different system prompt through various avenues in the `customization` section:
350-350: Heading level incrementUse H3 after an H2 section.
-#### System Prompt Path +### System Prompt Path
357-357: Heading level incrementUse H3.
-#### System Prompt Literal +### System Prompt Literal
367-371: Heading level and grammar
- Use H3
- it's → its
-#### Custom Profile - -You can pass a custom prompt profile via it's `path` to the customization: +### Custom Profile + +You can pass a custom prompt profile via its `path` to the customization:tests/profiles/test/profile.py (1)
28-35: Fix minor typo in instructions“tsting” → “testing”.
- - You provide validation for tsting + - You provide validation for testingtests/unit/utils/test_checks.py (1)
102-111: Tighten validation: assert system_prompts is a dict[str, str]Current is_valid_profile only checks for key presence; add a type check to prevent later runtime issues.
--- a/src/utils/checks.py +++ b/src/utils/checks.py @@ def is_valid_profile(profile_module: ModuleType) -> bool: """Validate that a profile module has the required PROFILE_CONFIG structure.""" if not hasattr(profile_module, "PROFILE_CONFIG"): return False profile_config = getattr(profile_module, "PROFILE_CONFIG", {}) if not isinstance(profile_config, dict): return False - return "system_prompts" in profile_config + if "system_prompts" not in profile_config: + return False + system_prompts = profile_config.get("system_prompts") + if not isinstance(system_prompts, dict): + return False + # Optional: ensure all values are strings + if not all(isinstance(k, str) and isinstance(v, str) for k, v in system_prompts.items()): + return False + return Truetests/unit/test_configuration.py (1)
426-508: Good coverage for profile-path customization and precedenceTests validate loading and prompt extraction from a custom profile alongside other customization knobs. Consider using Path objects for profile paths to be OS-agnostic, though CI typically runs on POSIX.
- expected_profile = CustomProfile(path="tests/profiles/test/profile.py") + expected_profile = CustomProfile(path=str((Path("tests") / "profiles" / "test" / "profile.py")))src/models/config.py (5)
4-4: Modern typingPrefer built-in generics over typing.Dict.
-from typing import Optional, Any, Pattern, Dict +from typing import Optional, Any, PatternAnd update usages to dict[str, str].
415-421: Avoid mutable default and align typingUse default_factory for prompts and modern dict typing.
-@dataclass -class CustomProfile: +@dataclass +class CustomProfile: @@ - path: str - prompts: Dict[str, str] = Field(default={}, init=False) + path: str + prompts: dict[str, str] = Field(default_factory=dict, init=False)
441-446: Stronger typing for profile_path and remove unsupported Field arg on BaseModelUse FilePath for early validation. Also, Field(init=False) is for dataclasses; on BaseModel it’s unnecessary and potentially confusing.
-class Customization(ConfigurationBase): +class Customization(ConfigurationBase): @@ - profile_path: Optional[str] = None + profile_path: Optional[FilePath] = None @@ - custom_profile: Optional[CustomProfile] = Field(default=None, init=False) + custom_profile: Optional[CustomProfile] = None
447-457: Cast FilePath to str/Path consistently when constructing CustomProfileIf adopting FilePath above, ensure correct type casting when creating CustomProfile.
- if self.profile_path: - self.custom_profile = CustomProfile(path=self.profile_path) + if self.profile_path: + self.custom_profile = CustomProfile(path=str(self.profile_path))
426-433: Security note: dynamic import executes arbitrary codeLoading Python files via exec_module runs code at import time. If profile_path is user-controlled, this is an RCE risk. Restrict who can set profile_path, consider a safer data-only format (e.g., YAML/JSON), or sandbox imports.
tests/unit/utils/test_endpoints.py (1)
74-89: Docstring mismatch with fixture name/configFixture enables query system prompt (disable_query_system_prompt=False) but doc mentions “disabled”. Update text to avoid confusion.
-def config_with_custom_profile_prompt_and_enabled_query_system_prompt_fixture(): - """Configuration with custom profile loaded for prompt and disabled query system prompt set.""" +def config_with_custom_profile_prompt_and_enabled_query_system_prompt_fixture(): + """Configuration with custom profile loaded for prompt and query system prompt enabled."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
README.md(2 hunks)docs/openapi.json(2 hunks)src/models/config.py(3 hunks)src/utils/checks.py(2 hunks)src/utils/endpoints.py(1 hunks)tests/profiles/test/profile.py(1 hunks)tests/profiles/test_two/test.txt(1 hunks)tests/unit/test_configuration.py(2 hunks)tests/unit/utils/test_checks.py(2 hunks)tests/unit/utils/test_endpoints.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/unit/utils/test_checks.py (1)
src/utils/checks.py (2)
import_python_module(32-39)is_valid_profile(42-51)
src/utils/endpoints.py (1)
src/configuration.py (1)
customization(117-121)
tests/unit/test_configuration.py (1)
src/configuration.py (3)
AppConfig(32-135)load_configuration(47-53)customization(117-121)
tests/unit/utils/test_endpoints.py (3)
src/models/requests.py (1)
QueryRequest(72-222)src/utils/endpoints.py (1)
get_system_prompt(70-110)src/configuration.py (2)
AppConfig(32-135)init_from_dict(55-57)
🪛 LanguageTool
README.md
[grammar] ~348-~348: Ensure spelling is correct
Context: ...text before the question is sent to the selceted LLM. The default system prompt is desig...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~348-~348: This phrase is redundant. Consider writing “various” or “different”.
Context: ...o use a different system prompt through various different avenues available in the `customization...
(VARIOUS_DIFFERENT)
🪛 markdownlint-cli2 (0.17.2)
README.md
350-350: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
⏰ 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). (2)
- GitHub Check: e2e_tests
- GitHub Check: build-pr
🔇 Additional comments (6)
tests/profiles/test_two/test.txt (1)
1-1: LGTM: negative fixture is clearThe file succinctly signals a non-Python payload for import-failure testing. No changes needed.
tests/profiles/test/profile.py (1)
49-60: LGTM: minimal, well-structured PROFILE_CONFIGThe test profile cleanly wires default, validation, and topic_summary prompts and sample responses/instructions.
tests/unit/utils/test_checks.py (2)
5-5: LGTM: explicit type check for imported modulesImporting ModuleType clarifies assertions in success-path test.
84-91: Positive-path coverage looks goodValidates import returns a ModuleType for a real profile module.
tests/unit/utils/test_endpoints.py (2)
9-9: LGTM: import CustomProfile for expectation buildingUsed only for deriving expected prompt; fine.
215-239: Good precedence tests for profile vs queryCovers both disable=true and disable=false scenarios and aligns with new endpoints precedence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
README.md (2)
350-356: Clarify what “System Prompt Path” expects (file type and path semantics).Readers benefit from knowing acceptable file types and path resolution.
-### System Prompt Path +### System Prompt Path +Provide a filesystem path to a plain text/Markdown file containing the prompt. Relative paths are resolved from the service working directory; absolute paths are supported.
367-374: Specify profile path target and required module interface; document precedence.Spell out what
profile_pathpoints to and the minimal interface the loader expects. Also capture prompt-precedence order to avoid ambiguity.-### Custom Profile - -You can pass a custom prompt profile via its `path` to the customization: +### Custom Profile + +You can pass a custom prompt profile via its `path` to the customization: +The path must point to either: +- a Python module file (e.g., /abs/path/to/profile.py), or +- a package directory that contains a profile.py module. + +The module must expose: +```python +# profile.py +PROFILE_CONFIG = {"system_prompts": "your system prompt here"} +``` + +Prompt precedence (highest to lowest): +1) request.system_prompt (from /v1/query, /v1/streaming_query) +2) prompts from the loaded custom profile +3) customization.system_prompt +4) customization.system_prompt_path +5) built-in defaulttests/profiles/test_three/profile.py (2)
49-50: Confirm PROFILE_CONFIG shape; prefer a dict over a 1-tuple of dict.Unless the loader explicitly expects a tuple, wrapping the dict in a single-element tuple is error-prone and mismatches typical dict-based configs.
-PROFILE_CONFIG = ({"system_prompts": QUERY_SYSTEM_INSTRUCTION},) +PROFILE_CONFIG = {"system_prompts": QUERY_SYSTEM_INSTRUCTION}If multiple named prompts are supported, consider a map for clarity:
PROFILE_CONFIG = { "system_prompts": { "query": QUERY_SYSTEM_INSTRUCTION, } }
12-18: Optional: Avoid unintended leading newline/indentation in prompt.Triple-quoted strings start with a newline; if the loader concatenates verbatim, consider stripping or dedenting.
from textwrap import dedent QUERY_SYSTEM_INSTRUCTION = dedent("""\ 1. Test This is a test system instruction You achieve this by offering: - testing """)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
README.md(2 hunks)src/utils/checks.py(2 hunks)tests/profiles/test/profile.py(1 hunks)tests/profiles/test_three/profile.py(1 hunks)tests/unit/utils/test_checks.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/checks.py
- tests/profiles/test/profile.py
- tests/unit/utils/test_checks.py
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~348-~348: This phrase is redundant. Consider writing “various” or “different”.
Context: ...o use a different system prompt through various different avenues available in the `customization...
(VARIOUS_DIFFERENT)
⏰ 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 (1)
README.md (1)
357-365: LGTM: Clear inline literal example.Example is concise and correctly indented for YAML block scalars.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
348-349: Drop “so-called”; add a blank line before the header.Removes loaded phrasing and fixes the paragraph-to-heading separation flagged by the linter.
-The service uses a so-called system prompt to put the question into context before it is sent to the selected LLM. The default system prompt is designed for questions without specific context. You can supply a different system prompt through various avenues available in the `customization` section: +The service uses a system prompt to put the question into context before it is sent to the selected LLM. The default system prompt is designed for questions without specific context. You can supply a different system prompt through various avenues in the `customization` section: +
🧹 Nitpick comments (1)
README.md (1)
366-374: Clarify profile_path semantics, include a minimal profile example, and document resolution precedence.Tighten wording to name the exact key, show the expected profile shape, and state the prompt resolution order introduced by this PR. Also add a short trust note about dynamic imports.
-### Custom Profile - -You can pass a custom prompt profile via its `path` to the customization: +### Custom Profile + +You can pass a custom prompt profile via its filesystem path in `customization.profile_path`: ```yaml customization: profile_path: <your/profile/path>
+Example profile module (profile.py):
+
+```python
+# A minimal profile exposing prompt strings
+prompts = {
- "system": "You are a helpful assistant..."
+}
+```+Resolution precedence (highest to lowest):
+1. Request override:system_promptpassed to/v1/queryor/v1/streaming_query
+2. Custom profile prompts (fromprofile_path)
+3.customization.system_prompt(literal)
+4.customization.system_prompt_path
+5. Built-in default
+
+Note: Loading a profile from a filesystem path performs a Python import; ensure the path is trusted.Please confirm the exact expected profile API (e.g., a top-level `prompts` dict) and the intended precedence order match the implementation/tests; adjust the snippet if the shape differs. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d3ea28679af76b71e612677c9d86cfd0cfe4db96 and 025bcdbc24433af9f40b03d3b9c14a6f1f64e93d. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [grammar] ~348-~348: There might be a mistake here. Context: ...vailable in the `customization` section: ### System Prompt Path ```yaml customizatio... (QB_NEW_EN) </details> </details> </details> <details> <summary>⏰ 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). (2)</summary> * GitHub Check: build-pr * GitHub Check: e2e_tests </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>README.md (1)</summary><blockquote> `356-364`: **LGTM: Clear literal example.** The inline YAML block is readable and correctly indented. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Jordan Dubrick <jdubrick@redhat.com>
Jdubrick
force-pushed
the
add-customization-profiles
branch
from
1c22102 to
8171a1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/utils/endpoints.py (1)
95-103: Harden profile-prompt fallback to avoid AttributeError and empty prompts.Validate the structure and ensure a non-empty string before returning; otherwise fall back. This mirrors an earlier suggestion.
- # profile takes precedence for setting prompt - if ( - config.customization is not None - and config.customization.custom_profile is not None - ): - prompt = config.customization.custom_profile.get_prompts().get("default") - if prompt: - return prompt + # profile takes precedence for setting prompt + if ( + config.customization is not None + and config.customization.custom_profile is not None + ): + prompts = config.customization.custom_profile.get_prompts() + if isinstance(prompts, dict): + prompt = prompts.get("default") + if isinstance(prompt, str) and prompt.strip(): + return prompt
🧹 Nitpick comments (4)
README.md (2)
348-349: Add a blank line before the next heading to fix formatting warning.Prevents heading from sticking to the preceding paragraph.
-The service uses a so-called system prompt to put the question into context before it is sent to the selected LLM. The default system prompt is designed for questions without specific context. You can supply a different system prompt through various avenues available in the `customization` section: +The service uses a so-called system prompt to put the question into context before it is sent to the selected LLM. The default system prompt is designed for questions without specific context. You can supply a different system prompt through various avenues available in the `customization` section: + ### System Prompt Path
366-374: Document profile module shape and precedence.Clarify expected profile file contents and the resolution order to match code behavior.
### Custom Profile You can pass a custom prompt profile via its `path` to the customization: ```yaml customization: profile_path: <your/profile/path>
+The profile must be a Python module (.py) that defines
PROFILE_CONFIGwith asystem_promptsmapping. Minimum example:
+
+```python
+# <your/profile/path>/profile.py
+PROFILE_CONFIG = {
- "system_prompts": {
"default": "You are a helpful assistant.",# optional named prompts:"redhat": "You answer with Red Hat context..."- }
+}
+```+Prompt resolution precedence:
+1.system_promptprovided in the request (ifdisable_query_system_promptis false)
+2.customization.profile_pathdefault prompt (system_prompts["default"])
+3.customization.system_prompt
+4. Built-in defaultI can add a short “Troubleshooting” note (e.g., non-.py paths are ignored, malformed modules are skipped) if helpful. </blockquote></details> <details> <summary>src/models/config.py (2)</summary><blockquote> `429-435`: **Coerce/validate profile prompts to string->string mapping.** Even with `is_valid_profile`, defensive coercion avoids downstream surprises and keeps types consistent. ```diff - if profile_module is not None and checks.is_valid_profile(profile_module): - self.prompts = profile_module.PROFILE_CONFIG.get("system_prompts", {}) + if profile_module is not None and checks.is_valid_profile(profile_module): + system_prompts = profile_module.PROFILE_CONFIG.get("system_prompts", {}) + if isinstance(system_prompts, dict): + self.prompts = { + k: v for k, v in system_prompts.items() + if isinstance(k, str) and isinstance(v, str) + } + else: + self.prompts = {}
451-460: Guard invalid/non-.py profile paths with early return and log context.Minor hardening: skip constructing
CustomProfileif the path doesn’t end with.py, and avoid raising on unreadable files during load phase.def check_customization_model(self) -> Self: """Load customizations.""" - if self.profile_path: - self.custom_profile = CustomProfile(path=self.profile_path) + if self.profile_path: + # only Python modules are supported + if str(self.profile_path).endswith(".py"): + self.custom_profile = CustomProfile(path=str(self.profile_path)) elif self.system_prompt_path is not None: checks.file_check(self.system_prompt_path, "system prompt") self.system_prompt = checks.get_attribute_from_file( dict(self), "system_prompt_path" ) return self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
README.md(2 hunks)docs/openapi.json(2 hunks)src/models/config.py(2 hunks)src/utils/checks.py(2 hunks)src/utils/endpoints.py(1 hunks)tests/profiles/test/profile.py(1 hunks)tests/profiles/test_three/profile.py(1 hunks)tests/profiles/test_two/test.txt(1 hunks)tests/unit/test_configuration.py(2 hunks)tests/unit/utils/test_checks.py(2 hunks)tests/unit/utils/test_endpoints.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/profiles/test_two/test.txt
- tests/profiles/test_three/profile.py
- tests/unit/utils/test_checks.py
- tests/unit/utils/test_endpoints.py
- tests/profiles/test/profile.py
- src/utils/checks.py
- tests/unit/test_configuration.py
- docs/openapi.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/utils/endpoints.py (2)
src/models/config.py (2)
config(132-138)get_prompts(436-438)src/configuration.py (1)
customization(117-121)
src/models/config.py (1)
src/utils/checks.py (3)
file_check(24-29)import_python_module(32-52)is_valid_profile(55-67)
🪛 LanguageTool
README.md
[grammar] ~348-~348: There might be a mistake here.
Context: ...vailable in the customization section: ### System Prompt Path ```yaml customizatio...
(QB_NEW_EN)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
| @dataclass | ||
| class CustomProfile: | ||
| """Custom profile customization for prompts and validation.""" | ||
|
|
||
| path: str | ||
| prompts: dict[str, str] = Field(default={}, init=False) | ||
|
|
There was a problem hiding this comment.
Fix mutable default on dataclass field (prompts).
Using {} creates a shared mutable default across instances. Use default_factory.
-from pydantic.dataclasses import dataclass
+from pydantic.dataclasses import dataclass
+from dataclasses import field
@@
@dataclass
class CustomProfile:
@@
- prompts: dict[str, str] = Field(default={}, init=False)
+ prompts: dict[str, str] = field(default_factory=dict, init=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.
| @dataclass | |
| class CustomProfile: | |
| """Custom profile customization for prompts and validation.""" | |
| path: str | |
| prompts: dict[str, str] = Field(default={}, init=False) | |
| from pydantic.dataclasses import dataclass | |
| from dataclasses import field | |
| @dataclass | |
| class CustomProfile: | |
| """Custom profile customization for prompts and validation.""" | |
| path: str | |
| prompts: dict[str, str] = field(default_factory=dict, init=False) |
🤖 Prompt for AI Agents
In src/models/config.py around lines 418 to 424, the dataclass field 'prompts'
uses a mutable default (`{}`) which creates a shared object across instances;
replace the mutable default with a default factory by using
Field(default_factory=dict, init=False) (i.e., switch to default_factory to
create a fresh dict per instance).
| profile_path: Optional[str] = None | ||
| disable_query_system_prompt: bool = False | ||
| system_prompt_path: Optional[FilePath] = None | ||
| system_prompt: Optional[str] = None | ||
| custom_profile: Optional[CustomProfile] = Field(default=None, init=False) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make custom_profile a private attribute; tighten profile_path type.
Prevents config input from setting it and avoids misuse of init in Field; also validates path type.
-from pydantic import (
+from pydantic import (
BaseModel,
ConfigDict,
Field,
+ PrivateAttr,
@@
-class Customization(ConfigurationBase):
+class Customization(ConfigurationBase):
@@
- profile_path: Optional[str] = None
+ profile_path: Optional[FilePath] = None
@@
- custom_profile: Optional[CustomProfile] = Field(default=None, init=False)
+ custom_profile: CustomProfile | None = PrivateAttr(default=None)📝 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.
| profile_path: Optional[str] = None | |
| disable_query_system_prompt: bool = False | |
| system_prompt_path: Optional[FilePath] = None | |
| system_prompt: Optional[str] = None | |
| custom_profile: Optional[CustomProfile] = Field(default=None, init=False) | |
| from pydantic import ( | |
| BaseModel, | |
| ConfigDict, | |
| Field, | |
| PrivateAttr, | |
| ) | |
| # … other imports … | |
| from typing import Optional | |
| from pydantic.types import FilePath | |
| class Customization(ConfigurationBase): | |
| # tighten profile_path to be a validated FilePath | |
| profile_path: Optional[FilePath] = None | |
| disable_query_system_prompt: bool = False | |
| system_prompt_path: Optional[FilePath] = None | |
| system_prompt: Optional[str] = None | |
| # make custom_profile truly private | |
| custom_profile: CustomProfile | None = PrivateAttr(default=None) |
🤖 Prompt for AI Agents
In src/models/config.py around lines 444-449, change profile_path to use
Optional[FilePath] (tighten its type) and make custom_profile a true private
attribute instead of a Field(init=False): remove the Field definition and define
_custom_profile as a pydantic PrivateAttr (e.g., from pydantic import
PrivateAttr; _custom_profile: Optional[CustomProfile] =
PrivateAttr(default=None)) so it cannot be set via config input or included in
model schema; update imports accordingly and remove the old init=False Field
line.
There was a problem hiding this comment.
It's a shame this PR doesn't go further.
The profile path could be a folder containing multiple profile definitions, a profile could include default model_id and provider_id plus QueryRequest could contain an optional profile_id... then selection of a System Prompt, model_id and provider_id could be selected at runtime when making a request...
It's help our immediate use case tremendously.
@TamiTakamiya @ldjebran FYI.
| Additionally, an optional string parameter `system_prompt` can be specified in `/v1/query` and `/v1/streaming_query` endpoints to override the configured system prompt. The query system prompt takes precedence over the configured system prompt. You can use this config to disable query system prompts: | ||
|
|
||
| ```yaml | ||
| customization: | ||
| system_prompt_path: "system_prompts/system_prompt_for_product_XYZZY" |
There was a problem hiding this comment.
This will be a breaking change for Ansible.
Not that it's not a reasonable simple refactor for us.
These type of changes should be called out in release notes.
There was a problem hiding this comment.
The prompt path is still there, it just got moved in the docs: https://github.com/lightspeed-core/lightspeed-stack/pull/487/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R353
If that is what you mean? This PR won't change the way anyone is currently consuming LCS
There was a problem hiding this comment.
Ah, i missed that. Thank-you @Jdubrick
There was a problem hiding this comment.
no worries, the diff was weird and really wasn't identifying that it was moved.
I get what you are saying about how this can be extended, and I believe we are definitely open to doing so, I know this right now is in early stages to get us moving on some of our RCS -> LCS parity pieces (cc @yangcao77), I can make additional changes if they help out/increase its usability outside of just my team
| """Load system prompt from file.""" | ||
| if self.system_prompt_path is not None: | ||
| """Load customizations.""" | ||
| if self.profile_path: |
There was a problem hiding this comment.
Want to understand the priority of system prompts when taking effect.
system prompt set in query endpoint call will definitely with the highest priority.
from this code block, if profile_path and system_prompt_path are both set, seems like profile_path will take place?
@tisnik any concern on that?
There was a problem hiding this comment.
Personally I'd like to think QueryRequest's Prompt had maximum priority; system_prompt_path could possibly be deprecated and profiles have secondary priority.
I'm moving on to a new project and will have to defer further contemplation to my colleagues @ldjebran and @TamiTakamiya and others (that I can't ping here.. not sure they're part of the organisation).
There was a problem hiding this comment.
I believe the prompt set in the query takes first, then profile, then prompt path with these changes:
|
@tisnik @yangcao77 may I please get a re-review whenever you have time? Thank you! |
|
Looks good to me from Developer Lightspeed's perspective |
|
@tisnik can we merge this PR if no other issues? We can make further enhancement on the profile customization if you think there will be some additional features needed, just like to unblock our team with this feature available. |
Description
This is a new PR of a refactor suggested in #434
Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
New Features
Documentation
OpenAPI
Tests