-
Notifications
You must be signed in to change notification settings - Fork 52
[RHDHPAI-976] Add customization profiles via path #487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5b5954b
c4d532f
5d1b456
2ff536d
b399002
4590bdf
6960ebd
8171a1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| PositiveInt, | ||||||||||||||||||||||||||||||||||||||||||||||||
| SecretStr, | ||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| from pydantic.dataclasses import dataclass | ||||||||||||||||||||||||||||||||||||||||||||||||
| from typing_extensions import Self, Literal | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| import constants | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -413,17 +415,44 @@ def jwk_configuration(self) -> JwkConfiguration: | |||||||||||||||||||||||||||||||||||||||||||||||
| return self.jwk_config | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| @dataclass | ||||||||||||||||||||||||||||||||||||||||||||||||
| class CustomProfile: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Custom profile customization for prompts and validation.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| path: str | ||||||||||||||||||||||||||||||||||||||||||||||||
| prompts: dict[str, str] = Field(default={}, init=False) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+418
to
+424
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix mutable default on dataclass field ( Using -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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||
| def __post_init__(self) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Validate and load profile.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| self._validate_and_process() | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def _validate_and_process(self) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Validate and load the profile.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| checks.file_check(Path(self.path), "custom profile") | ||||||||||||||||||||||||||||||||||||||||||||||||
| profile_module = checks.import_python_module("profile", self.path) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if profile_module is not None and checks.is_valid_profile(profile_module): | ||||||||||||||||||||||||||||||||||||||||||||||||
| self.prompts = profile_module.PROFILE_CONFIG.get("system_prompts", {}) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| def get_prompts(self) -> dict[str, str]: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Retrieve prompt attribute.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| return self.prompts | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| class Customization(ConfigurationBase): | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Service customization.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+444
to
449
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Make Prevents config input from setting it and avoids misuse of -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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||
| @model_validator(mode="after") | ||||||||||||||||||||||||||||||||||||||||||||||||
| def check_customization_model(self) -> Self: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Load system prompt from file.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| if self.system_prompt_path is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Load customizations.""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| if self.profile_path: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Want to understand the priority of system prompts when taking effect. system prompt set in query endpoint call will definitely with the highest priority. @tisnik any concern on that?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I'd like to think 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).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the prompt set in the query takes first, then profile, then prompt path with these changes: |
||||||||||||||||||||||||||||||||||||||||||||||||
| self.custom_profile = CustomProfile(path=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" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| """Custom profile for test profile.""" | ||
|
|
||
| SUBJECT_ALLOWED = "ALLOWED" | ||
| SUBJECT_REJECTED = "REJECTED" | ||
|
|
||
| # Default responses | ||
| INVALID_QUERY_RESP = ( | ||
| "Hi, I'm the Red Hat Developer Hub Lightspeed assistant, I can help you with questions about Red Hat Developer Hub or Backstage. " | ||
| "Please ensure your question is about these topics, and feel free to ask again!" | ||
| ) | ||
|
|
||
| QUERY_SYSTEM_INSTRUCTION = """ | ||
| 1. Test | ||
| This is a test system instruction | ||
|
|
||
| You achieve this by offering: | ||
| - testing | ||
| """ | ||
|
|
||
| USE_CONTEXT_INSTRUCTION = """ | ||
| Use the retrieved document to answer the question. | ||
| """ | ||
|
|
||
| USE_HISTORY_INSTRUCTION = """ | ||
| Use the previous chat history to interact and help the user. | ||
| """ | ||
|
|
||
| QUESTION_VALIDATOR_PROMPT_TEMPLATE = f""" | ||
| Instructions: | ||
| - You provide validation for testing | ||
| Example Question: | ||
| How can I integrate GitOps into my pipeline? | ||
| Example Response: | ||
| {SUBJECT_ALLOWED} | ||
| """ | ||
|
|
||
| TOPIC_SUMMARY_PROMPT_TEMPLATE = """ | ||
| Instructions: | ||
| - You are a topic summarizer | ||
| - For testing | ||
| - Your job is to extract precise topic summary from user input | ||
|
|
||
| Example Input: | ||
| Testing placeholder | ||
| Example Output: | ||
| Proper response test. | ||
| """ | ||
|
|
||
| PROFILE_CONFIG = { | ||
| "system_prompts": { | ||
| "default": QUERY_SYSTEM_INSTRUCTION, | ||
| "validation": QUESTION_VALIDATOR_PROMPT_TEMPLATE, | ||
| "topic_summary": TOPIC_SUMMARY_PROMPT_TEMPLATE, | ||
| }, | ||
| "query_responses": {"invalid_resp": INVALID_QUERY_RESP}, | ||
| "instructions": { | ||
| "context": USE_CONTEXT_INSTRUCTION, | ||
| "history": USE_HISTORY_INSTRUCTION, | ||
| }, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| """Custom profile for test profile.""" | ||
|
|
||
| SUBJECT_ALLOWED = "ALLOWED" | ||
| SUBJECT_REJECTED = "REJECTED" | ||
|
|
||
| # Default responses | ||
| INVALID_QUERY_RESP = ( | ||
| "Hi, I'm the Red Hat Developer Hub Lightspeed assistant, I can help you with questions about Red Hat Developer Hub or Backstage. " | ||
| "Please ensure your question is about these topics, and feel free to ask again!" | ||
| ) | ||
|
|
||
| QUERY_SYSTEM_INSTRUCTION = """ | ||
| 1. Test | ||
| This is a test system instruction | ||
|
|
||
| You achieve this by offering: | ||
| - testing | ||
| """ | ||
|
|
||
| USE_CONTEXT_INSTRUCTION = """ | ||
| Use the retrieved document to answer the question. | ||
| """ | ||
|
|
||
| USE_HISTORY_INSTRUCTION = """ | ||
| Use the previous chat history to interact and help the user. | ||
| """ | ||
|
|
||
| QUESTION_VALIDATOR_PROMPT_TEMPLATE = f""" | ||
| Instructions: | ||
| - You provide validation for testing | ||
| Example Question: | ||
| How can I integrate GitOps into my pipeline? | ||
| Example Response: | ||
| {SUBJECT_ALLOWED} | ||
| """ | ||
|
|
||
| TOPIC_SUMMARY_PROMPT_TEMPLATE = """ | ||
| Instructions: | ||
| - You are a topic summarizer | ||
| - For testing | ||
| - Your job is to extract precise topic summary from user input | ||
|
|
||
| Example Input: | ||
| Testing placeholder | ||
| Example Output: | ||
| Proper response test. | ||
| """ | ||
|
|
||
| PROFILE_CONFIG = ({"system_prompts": QUERY_SYSTEM_INSTRUCTION},) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| This file will fail the import. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, i missed that. Thank-you @Jdubrick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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