Skip to content
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.

Sign up for GitHub

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

Mark system_prompt config as not usable #2013

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Mark system_prompt config as not usable #2013

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ols/app/models/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,6 +995,13 @@ def __init__(
self.user_data_collection = UserDataCollection(
**data.get("user_data_collection", {})
)

if self.system_prompt is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably I missed discussion around this.. But if it is not used, should not we just remove this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed for upstream.

Copy link
Contributor

@asamal4 asamal4 Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is part of yaml config (one time setup done by admin) even in upstream, I am not sure why only file can not be used..
Example:
We set api key through a file.. we don't use a property to provide key directly..
I know prompt & keys are not exactly same in terms of security concern but just for comparison..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onmete hmm I was thinking about making it object attribute instead of class attribute. Would it be more clear then?

raise InvalidConfigurationError(
"The `system_prompt` is not meant for direct configuration. "
"To get the system prompt, please use the `system_prompt_path` attribute."
)

# read file containing system prompt
# if not specified, the prompt will remain None, which will be handled
# by system prompt infrastructure
Expand Down