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

Temporary fix:for o1-xx model need to covert systemMessage to aiMessage. #850

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

Emt-lin
Copy link
Contributor

@Emt-lin Emt-lin commented Nov 26, 2024

related: #828

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const chatModel = (ChainManager.chain as any).last.bound;
const modelName = chatModel.modelName || chatModel.model;
const isO1Model = modelName.startsWith("o1");
Copy link
Owner

Choose a reason for hiding this comment

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

Add a TODO: hack for o1 models, to be removed when they support system prompt

@logancyang
Copy link
Owner

Added my comments. Just merged your other PR, this one needs a rebase.

@@ -317,12 +317,26 @@ export default class ChainManager {
this.validateChatModel();
this.validateChainInitialization();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const chatModel = (ChainManager.chain as any).last.bound;
Copy link
Owner

Choose a reason for hiding this comment

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

What's the difference between using this vs. "ChatModelManager.getChatModel()"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@logancyang At first, I plan to use ChatModelManager.getChatModel(), but I copied your code for consistency.

@logancyang
Copy link
Owner

We can install the latest langchain openai and o1 models support streaming now: langchain-ai/langchainjs#7229

@logancyang
Copy link
Owner

Merging now.

With one caveat: Copilot Plus still doesn't support o1 models because its chain runner has more system messages. Right now I have no intention of enabling o1 models in Copilot Plus because

  1. The agentic query can be really expensive.
  2. I don't want to introduce too many hacks there

For the long term, o1 may support system message, let's see.

@logancyang logancyang merged commit a75ec96 into logancyang:master Dec 2, 2024
2 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.

2 participants