-
Notifications
You must be signed in to change notification settings - Fork 902
feat(ai-proxy): add Fireworks AI support #2917
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
Conversation
新增对 Fireworks AI 的支持变更概述
变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
Change Overview
Change file
💡 TipsHow to communicate with lingma-agents📜 Reply to comments directly
**📜 Mark ** at line of code
📜 Ask a question during discussion
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题,需立即修复。例如:系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题,高优先级修复。例如:核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 1 | 主要问题,建议修复。例如:非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题,酬情优化。例如:代码格式不规范或注释缺失。 |
总计: 1 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议,聚焦于代码的可读性、可维护性和潜在问题。
🔹 plugins/wasm-go/extensions/ai-proxy/test/fireworks.go (1 💬)
- 测试配置初始化时应处理 JSON 序列化错误,以避免潜在的空配置问题。 (L15-L24)
🚀 架构设计建议
以下是对代码架构和设计的综合分析,聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 架构一致性问题:Fireworks provider 实现与标准 OpenAI 兼容 provider 模式的偏离
虽然 Fireworks provider 基于标准 provider 模式实现,但其具体实现中缺少对某些关键能力(如流式处理、上下文缓存)的明确支持声明。这可能导致在特定场景下行为不一致或功能缺失。建议明确声明所有支持的能力,并确保其实现符合标准 provider 模式的要求。
📌 关键代码
func (f *fireworksProviderInitializer) DefaultCapabilities() map[string]string {
return map[string]string{
string(ApiNameChatCompletion): PathOpenAIChatCompletions,
string(ApiNameCompletion): PathOpenAICompletions,
string(ApiNameModels): PathOpenAIModels,
}
}可能导致在特定场景下功能缺失或行为不一致,影响用户体验和系统稳定性。
🔍2. 跨文件问题:Fireworks provider 测试覆盖不完整
当前 Fireworks provider 的测试主要集中在配置解析功能上,缺少对核心请求处理逻辑(如请求头转换、请求体处理等)的测试。这可能导致在实际运行中出现未预期的行为或错误。建议增加对核心功能的测试覆盖,确保其与其他 OpenAI 兼容提供者的行为一致。
📌 关键代码
func TestFireworks(t *testing.T) {
// 只测试核心配置解析功能,避免测试框架的并发 mutex 死锁问题
// Fireworks provider 基于标准 provider 模式实现,功能与其他 OpenAI 兼容提供者一致
test.RunFireworksParseConfigTests(t)
}可能导致在实际运行中出现未预期的行为或错误,影响系统的稳定性和可靠性。
🔍3. 技术债务:Fireworks provider 缺少详细的文档说明
虽然在 README 和 README_EN 中添加了 Fireworks AI 的基本配置示例,但缺少对其特有行为、限制和最佳实践的详细说明。这可能增加未来维护和扩展的难度。建议补充详细的文档,包括配置选项、API 映射规则、已知限制等内容。
📌 关键代码
#### Fireworks AI
Fireworks AI 所对应的 `type` 为 `fireworks`。它并无特有的配置字段。可能导致未来维护和扩展困难,增加技术债务。
🔍4. 安全性问题:Fireworks provider 的 API Token 管理策略不明确
当前 Fireworks provider 的 API Token 管理策略依赖于全局配置,缺乏对 Token 轮换、过期处理等安全机制的支持。这可能在生产环境中带来安全风险。建议引入更完善的 Token 管理策略,支持动态更新和轮换。
📌 关键代码
func (f *fireworksProviderInitializer) ValidateConfig(config *ProviderConfig) error {
if config.apiTokens == nil || len(config.apiTokens) == 0 {
return errors.New("no apiToken found in provider config")
}
return nil
}可能导致 API Token 泄露或滥用,带来安全风险。
🔍5. 可维护性问题:Fireworks provider 的代码复用度不高
Fireworks provider 的实现中存在与其他 OpenAI 兼容 provider 重复的代码逻辑(如请求头处理、API 名称解析等)。这可能增加代码维护成本和出错概率。建议抽象出通用的处理逻辑,提高代码复用度。
📌 关键代码
func (f *fireworksProvider) TransformRequestHeaders(ctx wrapper.HttpContext, apiName ApiName, headers http.Header) {
util.OverwriteRequestPathHeaderByCapability(headers, string(apiName), f.config.capabilities)
util.OverwriteRequestHostHeader(headers, fireworksDomain)
util.OverwriteRequestAuthorizationHeader(headers, "Bearer "+f.config.GetApiTokenInUse(ctx))
headers.Del("Content-Length")
}
func (f *fireworksProvider) GetApiName(path string) ApiName {
if strings.Contains(path, PathOpenAIChatCompletions) {
return ApiNameChatCompletion
}
if strings.Contains(path, PathOpenAICompletions) {
return ApiNameCompletion
}
if strings.Contains(path, PathOpenAIModels) {
return ApiNameModels
}
return ""
}可能导致代码维护成本增加,增加出错概率,影响系统的可维护性。
审查详情
📒 文件清单 (6 个文件)
✅ 新增: 2 个文件
📝 变更: 4 个文件
✅ 新增文件:
plugins/wasm-go/extensions/ai-proxy/provider/fireworks.goplugins/wasm-go/extensions/ai-proxy/test/fireworks.go
📝 变更文件:
plugins/wasm-go/extensions/ai-proxy/README.mdplugins/wasm-go/extensions/ai-proxy/README_EN.mdplugins/wasm-go/extensions/ai-proxy/main_test.goplugins/wasm-go/extensions/ai-proxy/provider/provider.go
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论,lingma-agents 将自动处理您的请求。例如:
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案,并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如:
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如:
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
| data, _ := json.Marshal(map[string]interface{}{ | ||
| "provider": map[string]interface{}{ | ||
| "type": "fireworks", | ||
| "apiTokens": []string{"fw-test123456789"}, | ||
| "modelMapping": map[string]string{ | ||
| "*": "accounts/fireworks/models/llama-v3p1-8b-instruct", | ||
| }, | ||
| }, | ||
| }) | ||
| return data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
测试配置初始化时应处理 JSON 序列化错误,以避免潜在的空配置问题。
🟡 Major | 🐞 Bugs
📋 问题详情
在测试配置初始化函数中,使用 json.Marshal 将配置数据序列化为 json.RawMessage,但忽略了可能发生的错误。如果序列化失败,将导致测试配置为空,从而影响测试结果。建议显式处理这些错误,确保测试配置正确生成。
💡 解决方案
显式处理 json.Marshal 返回的错误,确保测试配置正确生成。
- data, _ := json.Marshal(map[string]interface{}{
- "provider": map[string]interface{}{
- "type": "fireworks",
- "apiTokens": []string{"fw-test123456789"},
- "modelMapping": map[string]string{
- "*": "accounts/fireworks/models/llama-v3p1-8b-instruct",
- },
- },
- })
+ data, err := json.Marshal(map[string]interface{}{
+ "provider": map[string]interface{}{
+ "type": "fireworks",
+ "apiTokens": []string{"fw-test123456789"},
+ "modelMapping": map[string]string{
+ "*": "accounts/fireworks/models/llama-v3p1-8b-instruct",
+ },
+ },
+ })
+ if err != nil {
+ panic(err)
+ }您的反馈对我们很重要!(建议右键在新标签页中打开以下链接)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2917 +/- ##
==========================================
+ Coverage 35.91% 44.96% +9.05%
==========================================
Files 69 82 +13
Lines 11576 13383 +1807
==========================================
+ Hits 4157 6018 +1861
+ Misses 7104 7017 -87
- Partials 315 348 +33 🚀 New features to boost your workflow:
|
johnlanni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
add Fireworks AI support
refer: https://docs.fireworks.ai/tools-sdks/openai-compatibility
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews