Added compatibility for OpenAI api#1
Conversation
|
@copilot Review |
| use super::client::BaseLLMClient; | ||
|
|
||
| /// First-run bootstrap defaults for a registered provider. | ||
| #[derive(Debug, Clone, Copy)] |
There was a problem hiding this comment.
Why need this struct? The provider base_url and api_key_env are almost fixed
There was a problem hiding this comment.
Just directly use https://api.openai.com/v1 and OPENAI_API_KEY then other modifications are not needed
There was a problem hiding this comment.
As sometimes LLM API that people in use is not from their official provider, but from third-party, such as AWS, Azure and even locally deployed LLM. This PR can make it compatible to third-party API.
Also, I noticed that there is no choice to change default base url and API key name when lanuch the project at the first time, changing toml file in .mandeven is the only way, so this struct provides an more convenient entry to help inject modified provider url and API key name to replace hardcoded default them. By the way, let's think about a scenario that Deepseek or Mistral suddenly change their url? then the old one is resigned. Before this repo releases new update, people will have to locally and temporarily change code. For users, changing source code is not a decent way to solve problem, and whatever at least I think separating static configs from source code is necessary, or providing an entry to change config, which is what I did in this PR
|
llm crate was refactored you can try the latest one |
I see the refactor you made on the part of LLM API. I shortly tested the new trait, it does work for third-party API. However, another key issue in this PR is not yet resolved.
When project is run at the first time, no entry to directly create third-party API model and customised base URL for DeepSeek and Mistral as well. For now, the only way to change it is by modifying the toml file in your hidden directory, which I don't think it's a good UX design for CLI tool at least. Alternative method to change LLM config should be provided. |
|
Moreover, it seems that you are going to make a highly abstraction of Open AI SDK to force all model providers to use it, and all stuff are put into it, such as specific model features of cache control and reasoning effort. I'm not sure if this is good code style or not for now, as it looks like this module is much easy to gradually become heavy in the future dev. For instance, seen from the current code of Deepseek, it looks like u wanna set their features by setting various flags for each individual provider. However, some features like thinking ability, OpenAI has reasoning efforts to adjust, but Qwen has a totally different parameter trigger to switch thinking, so are you gonna make two irrelevant simple flags to show their status? as they all use the same trait. But one flag is actually meaningless in other LLMs, and even incompatible. By the way, I'm not sure if you have plan to add extra LLM SDK in the future, if in that case, current LLM management architecture is not a good design, another extra refactor may be needed at that time. Also a personal suggestion: I noticed that this project has too much high refactor frequency, maybe because the project work architecture is not yet cleared or still in exploring. Many modules are unnecessarily and even repeatedly stocked together. I truly suggest think more about the extendable architecture, especially for a Rust project, which brings much maintenance and update iteration difficulty. |
|
NO, I mean there is no any entry at all in UI to customise it |
To do it, it will require user to complete it via four or even more steps if it needs ui to customise, but why? use agent just one sentence to complete it. Also, current system design does provide the information of agent system configuration. |
Most of refactor is to divide some large files into small file to hold the feature responsibility and boundary. Otherwise, it will has a 3k+ lines files to orchestrate the whole system, which is not good for maintenance. Also, I guess the repetition you mean is what I made in agent crate encapsulation, which is used to encapsulate the other crate feature and for final orchestrate. This is to help further maintain |
Current repo needs more steps as it creates useless llm config at start then we have to manually modify it to adapt third-party, that's why it produces more unnecessary steps, so why don't just set correct info at the start?
Current repo needs more steps as it creates useless llm config at start then we have to manually modify it to adapt third-party, that's why it produces more unnecessary steps, so why don't just set correct info at the start? unless you say, this project only supports official deepseek and mistral, anyone else get out I'm talking about bootstrap section, UI has not yet update with third-party and no custom. |
|
I think we have recognisation diff on "provider" this term. For my understanding, "provider" identifies developer, so deepseek this model can be provided by any platform, that's why I think its url could be modified at bootstrap. |
|
I would say providers refer to official platforms, for third-party like openrouter, aws, and others would be other providers naming openrouter, aws like that |
Uh oh!
There was an error while loading. Please reload this page.