System prompt at App level #484
Conversation
|
@cachho Can you please review it and help with documentation and also test for |
| """ | ||
| :param config: AppConfig instance to load as configuration. Optional. | ||
| :param system_prompt: Optional. System prompt string. | ||
| """ | ||
| if config is None: | ||
| config = AppConfig() | ||
|
|
There was a problem hiding this comment.
couldn't you just say
if system_prompt and not config.system_prompt:
config.system_prompt = system_promptright here, and use latch on to the existing implementation? do this for all app classes? should be easier to maintain. You should make a setter method in config though, to make it cleaner.
There was a problem hiding this comment.
So you are saying instead of using self.system_prompt we can set config.system_prompt for app classes. And add a setter method for system_prompt in config to set its value if we get prompt while app init. Right?? @cachho
There was a problem hiding this comment.
I'm saying that instead of introducing something new we just use the App __init__ to set the existing value in config, and keep working with that. As the most minimal way to achieve this.
My example should work, but introducing a set_system_prompt method in BaseAppConfig can be easier to understand.
There was a problem hiding this comment.
Ok but why are we using BaseAppConfig when QueryConfig is used to get system_prompt on chat\query level. Using BaseAppConfig will take more changes and will be similar to current approach.
There was a problem hiding this comment.
I hope you aren't waiting for my input. I haven't had a chance to take a deeper look into this.
There was a problem hiding this comment.
Sorry I was away for few days but I don't think making changes in BaseAppConfig will be a short approach. So directly using system_prompt from app _init_ seems to me the shortest approach. Let me know if you find any better or else I'll make few minor changes for it and PR will be ready for review @cachho
|
Done with changes |
|
looks good to me. |
Description
Add
system_prompton app level as well.Fixes #452
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Run
poetry run pytestChecklist: