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

[Theia AI] Support DeepSeek reasoner (and future models with special "system" prompt requirements #14867

Open
JonasHelming opened this issue Feb 7, 2025 · 4 comments · May be fixed by #14877

Comments

@JonasHelming
Copy link
Contributor

the resoner complains if the first two messages are "user" and does not support developerMessages (nor system). This is currently our default if developer messages are not supported.
To avoid similar issues in the future, I would propose to:

  • Introduce a setting "SystemPromptRole" (String) with which you can override the system role (e.g. to "system" or "user")
  • Introduce a setting "mergeSystemPrompt" that will merge the system prompt with the first user message

This would kind of make the setting "supportsDeveloperMessage" obsolete.

@planger WDYT?

@planger
Copy link
Contributor

planger commented Feb 7, 2025

Yeah I guess we could introduce a property as follows instead of the simple flag we had before:

developerMessageSettings: {
  /** Override the role name of the developer message, defaults to 'developer */
  role?: string,
  /** Specify how the developer message is included in the request, defaults to 'putAsFirstMessage' */
  includeInMessages?: 'putAsFirstMessage' | 'mergeWithFirstUserMessage' | 'skip',
}

@JonasHelming
Copy link
Contributor Author

@planger great suggestion, just one question: Whats the difference between "putAsFirstMessage" and leaving out "includeInMessage"?

@planger
Copy link
Contributor

planger commented Feb 7, 2025

I think pushAsFirstMessage is what we do now and which I assume will remain to be the default. So when we leave out this property we'll still put the developer message as first message into the message list.

I just thought when we now start to add different strategies here, we should give a name to what we do now and continue to do by default.

@JonasHelming
Copy link
Contributor Author

Supporting arbitrary strings as roles has quite some implications for unknown benefit. Therefore, i went for:

enum: ['user', 'system', 'developer', 'mergeWithFirstUserMessage', 'skip'],

reasoning is that 'mergeWithFirstUserMessage' and 'skip' do not make sense if the role is also defined.

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 a pull request may close this issue.

2 participants