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.

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

feat: Add a model config option to disable system prompts #1539

Merged
merged 5 commits into from
Oct 29, 2024

Conversation

Mounayer
Copy link
Contributor

@Mounayer Mounayer commented Oct 22, 2024

Description

Closes #1432

Some models don't support system prompts, therefore, a new modelConfig option has been added: systemRoleSupported

By default, the systemRoleSupported option is true, where everything behaves as is. However, if the systemRoleSupported option is set to false, all role: "system" messages are filtered out. And the very first role: "system" message encountered, will have its content preprended to the very first user message.

Brief example, let's assume the following is our messages array, before processing:

[
    { "content": "Welcome to our service!", "role": "system" },
    { "content": "Hi, how can I help you today?", "role": "system" },
    { "content": "I need help with my account.", "role": "user" },
    { "content": "Certainly! What issue are you experiencing?", "role": "system" },
    { "content": "I forgot my password.", "role": "user" },
    { "content": "Please reset your password using the 'Forgot Password' link.", "role": "system" },
    { "content": "Thank you!", "role": "user" }
]

After we apply the filtering, and preprending the first system message's content into the first user message, we get:

[
    { "content": "Welcome to our service!\nI need help with my account.", "role": "user" },
    { "content": "I forgot my password.", "role": "user" },
    { "content": "Thank you!", "role": "user" }
]

This keeps the existing functionality untouched, while giving the option of using a wider range of models, even those that do not support system role messages!

Please let me know any thoughts or suggestions that may arise!

Copy link
Collaborator

@nsarrazin nsarrazin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! i think this is very much needed for some kind of models like gemma & the o1 models.

I think the name of the argument is maybe a bit confusing sorry I know I'm the one who wrote it in the initial issue 😬

I think people might assume it's a field to set a system prompt. What do you think of m.systemRoleSupported instead ?

@Mounayer
Copy link
Contributor Author

Thanks for the contribution! i think this is very much needed for some kind of models like gemma & the o1 models.

I think the name of the argument is maybe a bit confusing sorry I know I'm the one who wrote it in the initial issue 😬

I think people might assume it's a field to set a system prompt. What do you think of m.systemRoleSupported instead ?

Super happy to contribute!

Honestly, I agree. It can be a little confusing, I've updated the config option name to be systemRoleSupported instead. I think now it's more intuitive as to what it does!

Copy link
Collaborator

@nsarrazin nsarrazin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@nsarrazin nsarrazin merged commit b1355da into huggingface:main Oct 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a model config option to disable system prompts
2 participants