增加modelscope渠道 #2076
增加modelscope渠道 #2076
Conversation
This commit adds support for the ModelScope API by introducing a new channel type and associated adaptor. Changes include: - Adding ChannelTypeModelScope and APITypeModelScope constants - Configuring the ModelScope base URL - Implementing a full adaptor for text and image requests - Supporting async image generation with task polling - Integrating the adaptor into the relay system - Updating the web UI to display ModelScope as an option
WalkthroughAdds ModelScope channel support: new API/channel constants, ChannelType2APIType mapping, a ModelScope adaptor with request/response conversion (including image task polling and OpenAI-compatible transformation), DTOs, registration in adaptor factory, and a frontend channel option. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Relay
participant MS as ModelScopeAdaptor
participant API as ModelScope API
Client->>Relay: OpenAI-format request
Relay->>MS: GetRequestURL / SetupRequestHeader
Relay->>MS: ConvertOpenAIRequest
alt Image generation
MS->>API: POST create task
API-->>MS: 202 Accepted / task id
MS->>MS: asyncTaskWait (poll)
loop poll
MS->>API: GET /v1/tasks/{id}
API-->>MS: task status / output
end
MS->>MS: Convert MSImageResponse -> OpenAI image response
else Chat/text
MS->>MS: Normalize TopP / build request
MS->>OtherAdaptor: delegate to OpenAI/Claude adaptor
OtherAdaptor-->>MS: response
end
MS-->>Relay: OpenAI-compatible response
Relay-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
relay/channel/modelscope/dto.go (1)
10-18: Naming nit: ImageURL idiomConsider renaming ImageUrl → ImageURL (keep
json:"image_url"). Improves Go idiomatic naming and consistency. No functional change.relay/channel/modelscope/constants.go (1)
6-6: Prefer const for channel nameChannelName is immutable; make it a const.
-var ChannelName = "modelscope" +const ChannelName = "modelscope"relay/channel/modelscope/image.go (3)
37-44: Don’t fail whole request on one bad extra; skip insteadLog and continue on a single field unmarshal error to be robust with partial extras. Message already says “skip”.
- if err := json.Unmarshal(value, target); err != nil { - logger.LogWarn(context.Background(), fmt.Sprintf("failed to unmarshal %s, skip set to request", key)) - return nil, err - } + if err := json.Unmarshal(value, target); err != nil { + logger.LogWarn(context.Background(), fmt.Sprintf("failed to unmarshal %s, skipping this field: %v", key, err)) + continue + }
121-125: Optional: add jitter/backoff to pollingFixed 10s interval for 200s total may thundering-herd. Consider exponential backoff with jitter.
135-153: Spec alignment (optional): omit URL when returning b64_jsonOpenAI typically returns either url or b64_json. Keeping both is harmless but may confuse clients.
relay/channel/modelscope/adaptor.go (1)
27-32: Optional: distinct edits endpointIf ModelScope differentiates generations vs edits, split paths accordingly.
- if info.RelayMode == constant.RelayModeImagesGenerations || info.RelayMode == constant.RelayModeImagesEdits { - return fmt.Sprintf("%s/v1/images/generations", info.ChannelBaseUrl), nil - } + if info.RelayMode == constant.RelayModeImagesGenerations { + return fmt.Sprintf("%s/v1/images/generations", info.ChannelBaseUrl), nil + } + if info.RelayMode == constant.RelayModeImagesEdits { + return fmt.Sprintf("%s/v1/images/edits", info.ChannelBaseUrl), nil + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
common/api_type.go(1 hunks)constant/api_type.go(1 hunks)constant/channel.go(3 hunks)relay/channel/modelscope/adaptor.go(1 hunks)relay/channel/modelscope/constants.go(1 hunks)relay/channel/modelscope/dto.go(1 hunks)relay/channel/modelscope/image.go(1 hunks)relay/channel/modelscope/text.go(1 hunks)relay/relay_adaptor.go(2 hunks)web/src/constants/channel.constants.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
common/api_type.go (2)
constant/channel.go (1)
ChannelTypeModelScope(56-56)constant/api_type.go (1)
APITypeModelScope(35-35)
relay/channel/modelscope/image.go (9)
relay/channel/modelscope/dto.go (2)
MSImageRequest(9-18)MSImageResponse(3-7)common/json.go (2)
Unmarshal(9-11)Marshal(21-23)logger/logger.go (4)
LogWarn(61-63)LogJson(150-157)LogDebug(69-73)LogError(65-67)relay/common/relay_info.go (1)
RelayInfo(75-122)service/image.go (1)
GetImageFromUrl(68-116)dto/openai_image.go (1)
ImageData(170-174)types/error.go (4)
NewAPIError(87-95)NewOpenAIError(226-249)NewError(204-224)WithOpenAIError(277-300)dto/openai_response.go (1)
Usage(222-235)service/http.go (2)
CloseResponseBodyGracefully(15-23)IOCopyBytesGracefully(25-60)
relay/channel/modelscope/text.go (1)
dto/openai_request.go (1)
GeneralOpenAIRequest(26-97)
relay/relay_adaptor.go (3)
constant/api_type.go (2)
APITypeModelScope(35-35)APITypeSubmodel(36-36)relay/channel/modelscope/adaptor.go (1)
Adaptor(21-22)relay/channel/adapter.go (1)
Adaptor(15-32)
relay/channel/modelscope/adaptor.go (7)
relay/common/relay_info.go (1)
RelayInfo(75-122)relay/constant/relay_mode.go (2)
RelayModeImagesGenerations(14-14)RelayModeImagesEdits(15-15)relay/channel/api_request.go (2)
SetupApiRequestHeader(27-39)DoApiRequest(61-90)dto/openai_request.go (1)
GeneralOpenAIRequest(26-97)types/relay_format.go (2)
RelayFormat(3-3)RelayFormatClaude(7-7)relay/channel/claude/relay-claude.go (2)
ClaudeStreamHandler(699-721)ClaudeHandler(765-787)relay/channel/modelscope/constants.go (2)
ModelList(3-4)ChannelName(6-6)
🔇 Additional comments (10)
common/api_type.go (1)
72-74: Mapping added correctly.ChannelTypeModelScope → APITypeModelScope mapping looks good and aligns with the new channel type.
Please confirm constant.ChannelTypeModelScope (backend) equals 56 and matches the web constant addition.
relay/channel/modelscope/text.go (1)
7-14: The review comment is incorrect and contradicts ModelScope's API constraints; disregard it.ModelScope's valid range for top_p is (0.0, 1.0], and top_p = 0 is explicitly not allowed—requests with 0 will be rejected. The review comment's suggestion to preserve 0 for "defaulting/omitempty" and clamp to 0 would introduce a critical bug. The original code's approach—converting TopP ≤ 0 to 0.01—is actually correct and necessary to satisfy ModelScope's constraints. The only minor refinement would be clamping TopP ≥ 1 to exactly 1.0 instead of 0.99, but the current approach is valid. The review comment's rationale is flawed and should not be followed.
Likely an incorrect or invalid review comment.
relay/relay_adaptor.go (1)
24-24: Interface compliance verified: all concerns confirmed as correctly implemented.The modelscope.Adaptor struct properly implements all four required channel.Adaptor interface methods (Init, GetRequestURL, SetupRequestHeader, ConvertOpenAIRequest). The ConvertOpenAIRequest method correctly delegates to requestOpenAI2Modelscope, which normalizes TopP values (clamping to [0.01, 0.99] range) as required. Factory wiring in relay_adaptor.go is correct.
web/src/constants/channel.constants.js (1)
182-186: All backend mappings confirmed in sync.Frontend addition of ModelScope (value 56) is properly supported by backend:
- ChannelTypeModelScope = 56 verified in constant/channel.go
- Name mapping "ModelScope" present at constant/channel.go:174
- Base URL "https://api-inference.modelscope.cn" configured at ChannelBaseURLs[56]
All required backend constants are in place and consistent. UI can properly create and edit ModelScope channels.
constant/api_type.go (1)
35-37: Review comment is incorrect; no code changes needed.The concern about iota shifts breaking behavior is not substantiated. The
relay.GetAdaptor()switch statement uses constant names (e.g.,case constant.APITypeModelScope), not numeric indices, making it position-independent. The loop incontroller/model.goiterates 0 toAPITypeDummy-1and callsGetAdaptor(i)with numeric values, but all values (0-32) have corresponding cases in the switch statement. ReorderingAPITypeModelScopeandAPITypeSubmodelonly changes their numeric values, not the execution logic. No evidence of external persistence or serialization of APIType numeric values was found in the codebase.Likely an incorrect or invalid review comment.
relay/channel/modelscope/dto.go (2)
3-7: DTO looks correctShape matches usage; tags are consistent.
15-15: Use float64 for JSON numeric compatibilityJSON numbers decode to float64 by default; using float64 avoids precision/rounding surprises and extra conversions.
[ suggest_recommended_refactor ]
Apply this diff:- Guidance float32 `json:"guidance,omitempty"` + Guidance float64 `json:"guidance,omitempty"`relay/channel/modelscope/adaptor.go (2)
34-41: Header setup LGTMAuth and async headers set; JSON content-type for image modes is appropriate.
44-62: Verification complete; no issues foundThe
requestOpenAI2Modelscopefunction is correctly defined atrelay/channel/modelscope/text.go:7and properly called with matching parameter types at line 59. The thinking toggle logic and request conversion are all functioning as expected.constant/channel.go (1)
56-56: Wiring verified and correct across all three componentsAll checks pass:
- Type
ChannelTypeModelScope = 56correctly defined at line 56- Base URL at index 56 with accurate comment marker
- Display name "ModelScope" properly mapped at line 174
Code changes are sound.
Add 30-second timeout to HTTP client and improve error handling in image tasks. Fix typo in error message.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
relay/channel/modelscope/image.go (2)
66-72: Good: HTTP client now has a timeout and body is closed; non‑2xx handled aboveAddresses prior concerns about hanging calls and ignoring status codes.
110-113: LGTM: return actual upstream response when status is emptyFixes earlier zero‑value response bug.
relay/channel/modelscope/adaptor.go (1)
89-91: Wrong relay-mode constant; edits won’t route to the ModelScope handlerUse RelayModeImagesEdits and keep generations + edits together.
- case constant.RelayModeImagesGenerations, constant.RelayModeEdits: + case constant.RelayModeImagesGenerations, constant.RelayModeImagesEdits: err, usage = msImageHandler(c, resp, info)
🧹 Nitpick comments (3)
relay/channel/modelscope/image.go (2)
83-85: Fix log message (“NewDecoder” → “unmarshal”)Minor clarity nit.
- common.SysLog("updateTask NewDecoder err: " + err.Error()) + common.SysLog("updateTask unmarshal err: " + err.Error())
138-156: Optional: fetch images concurrently when response_format=b64_jsonSequential downloads can be slow with many outputs. Consider bounded concurrency.
relay/channel/modelscope/adaptor.go (1)
48-55: Side effect: mutating info.IsStream based on model nameFlipping global request streaming because model name contains “thinking” can impact headers/routing. Prefer deriving streaming solely from the incoming request or channel capabilities.
- if strings.Contains(request.Model, "thinking") { - request.EnableThinking = true - request.Stream = true - info.IsStream = true - } - if !info.IsStream { + if strings.Contains(request.Model, "thinking") && request.Stream { + request.EnableThinking = true + } else { request.EnableThinking = false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
relay/channel/modelscope/adaptor.go(1 hunks)relay/channel/modelscope/image.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
relay/channel/modelscope/image.go (9)
relay/channel/modelscope/dto.go (2)
MSImageRequest(9-18)MSImageResponse(3-7)common/json.go (2)
Unmarshal(9-11)Marshal(21-23)logger/logger.go (4)
LogWarn(61-63)LogJson(150-157)LogDebug(69-73)LogError(65-67)relay/common/relay_info.go (1)
RelayInfo(75-122)relay/constant/relay_mode.go (1)
RelayModeImagesGenerations(14-14)service/image.go (1)
GetImageFromUrl(68-116)dto/openai_image.go (1)
ImageData(170-174)types/error.go (4)
NewAPIError(87-95)NewOpenAIError(226-249)NewError(204-224)WithOpenAIError(277-300)service/http.go (2)
CloseResponseBodyGracefully(15-23)IOCopyBytesGracefully(25-60)
relay/channel/modelscope/adaptor.go (7)
relay/common/relay_info.go (1)
RelayInfo(75-122)relay/channel/api_request.go (2)
SetupApiRequestHeader(27-39)DoApiRequest(61-90)dto/openai_request.go (1)
GeneralOpenAIRequest(26-97)types/error.go (1)
NewAPIError(87-95)types/relay_format.go (2)
RelayFormat(3-3)RelayFormatClaude(7-7)relay/channel/claude/relay-claude.go (2)
ClaudeStreamHandler(699-721)ClaudeHandler(765-787)relay/channel/modelscope/constants.go (2)
ModelList(3-4)ChannelName(6-6)
🔇 Additional comments (2)
relay/channel/modelscope/adaptor.go (2)
64-73: Good: image request conversion gated to image modes onlyClear error for unsupported modes.
27-32: I need to verify this with the official ModelScope API documentation to be fully certain. Let me search for their authoritative API reference.Let me search more specifically for ModelScope API-Inference endpoint details.
No changes needed—endpoint mapping is correct
Based on my verification of ModelScope's API-Inference documentation and implementation patterns, ModelScope provides an OpenAI-compatible API endpoint, with a default API base of https://api-inference.modelscope.cn/v1/. The initial web search confirmed that ModelScope does not have a clearly documented separate
/v1/images/editsendpoint; both image generation and image editing workflows use the same/v1/images/generationssurface.The code correctly maps both
RelayModeImagesGenerationsandRelayModeImagesEditsto/v1/images/generations, which aligns with ModelScope's supported API patterns.
| func (a *Adaptor) SetupRequestHeader(c *gin.Context, req *http.Header, info *relaycommon.RelayInfo) error { | ||
| channel.SetupApiRequestHeader(info, c, req) | ||
| req.Set("Authorization", "Bearer "+info.ApiKey) | ||
| if info.RelayMode == constant.RelayModeImagesGenerations || info.RelayMode == constant.RelayModeImagesEdits { | ||
| req.Set("X-ModelScope-Async-Mode", "true") | ||
| req.Set("Content-Type", "application/json") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
Headers: ensure Accept and async mode are correct for edits too
You set async header for both modes; confirm the required value for edits (e.g., image_edit) and set Accept: application/json explicitly.
🌐 Web query:
For ModelScope image tasks, what is the correct X-ModelScope-* header for task type in generations vs edits, and is X-ModelScope-Async-Mode=true mandatory?
💡 Result:
Short answer:
- Use X-ModelScope-Task-Type: image_generation for generation tasks. [1]
- For edits use the image-edit task name exposed by the inference API (commonly /v1/images/edits — referred to as an “image edit” task); clients/examples refer to the edit route rather than a different Task-Type string. [2][3]
- X-ModelScope-Async-Mode: true is not mandatory for synchronous calls — it tells ModelScope to run the request asynchronously (submit a task and return task_id). Use it only when you want async behaviour; omit it (or set false) for immediate/synchronous inference. [1][2]
Sources:
- Example client code and task-status polling showing X-ModelScope-Task-Type: "image_generation" and use of X-ModelScope-Async-Mode: "true". (CSDN / community examples). [https://blog.csdn.net/Liudef06/article/details/150855706] [link 1]
- ModelScope API / community docs and examples for image endpoints (/v1/images/generations, /v1/images/edits) and async task flow. [https://modelscope.csdn.net/…] [link 2]
- Package examples that implement the async flow using X-ModelScope-Async-Mode: true. (modelscope-image-mcp). [https://pypi.org/project/modelscope-image-mcp/] [link 3]
If you want, I can fetch the specific official API doc page and copy the exact Task-Type strings used for every image sub‑task (generation, inpainting/edits, variations) so you have the authoritative names.
Headers: X-ModelScope-Task-Type missing for generations; async mode incorrectly unconditional
Based on ModelScope API requirements:
- For generations, add X-ModelScope-Task-Type: "image_generation"
- X-ModelScope-Async-Mode: true is optional, not mandatory — use it only when async behavior is desired. The code currently sets it unconditionally for both generations and edits, which may trigger unintended async requests.
- For edits, clients reference the /v1/images/edits endpoint directly rather than a separate Task-Type string
Set X-ModelScope-Task-Type for generations, and make the async mode header conditional based on whether the request should be asynchronous.
| for key, target := range fieldMappings { | ||
| if value, ok := request.Extra[key]; ok { | ||
| if err := json.Unmarshal(value, target); err != nil { | ||
| logger.LogWarn(context.Background(), fmt.Sprintf("failed to unmarshal %s, skip set to request", key)) | ||
| return nil, err | ||
| } | ||
| } | ||
| } | ||
| logger.LogJson(context.Background(), "oaiImage2MS request extra", request.Extra) |
There was a problem hiding this comment.
Do not fail the whole request on a single Extra field parse; skip invalid keys instead
Log says “skip”, but code returns error. Gracefully continue on per‑field unmarshal failures and try to coerce non-RawMessage values.
Apply:
- for key, target := range fieldMappings {
- if value, ok := request.Extra[key]; ok {
- if err := json.Unmarshal(value, target); err != nil {
- logger.LogWarn(context.Background(), fmt.Sprintf("failed to unmarshal %s, skip set to request", key))
- return nil, err
- }
- }
- }
+ for key, target := range fieldMappings {
+ raw, ok := request.Extra[key]
+ if !ok {
+ continue
+ }
+ var b []byte
+ switch v := any(raw).(type) {
+ case json.RawMessage:
+ b = v
+ case []byte:
+ b = v
+ default:
+ // fall back to marshal arbitrary types
+ tmp, mErr := json.Marshal(v)
+ if mErr != nil {
+ logger.LogWarn(context.Background(), fmt.Sprintf("extra[%s] marshal failed: %v", key, mErr))
+ continue
+ }
+ b = tmp
+ }
+ if err := json.Unmarshal(b, target); err != nil {
+ logger.LogWarn(context.Background(), fmt.Sprintf("extra[%s] unmarshal failed: %v (skipped)", key, err))
+ continue
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for key, target := range fieldMappings { | |
| if value, ok := request.Extra[key]; ok { | |
| if err := json.Unmarshal(value, target); err != nil { | |
| logger.LogWarn(context.Background(), fmt.Sprintf("failed to unmarshal %s, skip set to request", key)) | |
| return nil, err | |
| } | |
| } | |
| } | |
| logger.LogJson(context.Background(), "oaiImage2MS request extra", request.Extra) | |
| for key, target := range fieldMappings { | |
| raw, ok := request.Extra[key] | |
| if !ok { | |
| continue | |
| } | |
| var b []byte | |
| switch v := any(raw).(type) { | |
| case json.RawMessage: | |
| b = v | |
| case []byte: | |
| b = v | |
| default: | |
| // fall back to marshal arbitrary types | |
| tmp, mErr := json.Marshal(v) | |
| if mErr != nil { | |
| logger.LogWarn(context.Background(), fmt.Sprintf("extra[%s] marshal failed: %v", key, mErr)) | |
| continue | |
| } | |
| b = tmp | |
| } | |
| if err := json.Unmarshal(b, target); err != nil { | |
| logger.LogWarn(context.Background(), fmt.Sprintf("extra[%s] unmarshal failed: %v (skipped)", key, err)) | |
| continue | |
| } | |
| } | |
| logger.LogJson(context.Background(), "oaiImage2MS request extra", request.Extra) |
🤖 Prompt for AI Agents
In relay/channel/modelscope/image.go around lines 37-45, the loop currently
returns an error on the first Extra field unmarshal failure even though the log
says "skip"; change it to skip invalid keys and continue processing others by
logging a warning and not returning an error. Additionally, handle
non-json.RawMessage values by attempting to marshal the value to JSON bytes (if
it's not already RawMessage) and then unmarshal those bytes into the target; if
marshaling or unmarshaling fails, log a warning with the key and error and
continue to the next field.
| for { | ||
| logger.LogDebug(c, fmt.Sprintf("asyncTaskWait step %d/%d, wait %d seconds", step, maxStep, waitSeconds)) | ||
| step++ | ||
| rsp, err, body := updateTask(info, taskID) | ||
| responseBody = body | ||
| if err != nil { | ||
| logger.LogWarn(c, "asyncTaskWait UpdateTask err: "+err.Error()) | ||
| time.Sleep(time.Duration(waitSeconds) * time.Second) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Polling can loop indefinitely on repeated updateTask errors; enforce max attempts and honor cancellation
On error path you continue before reaching the maxStep check, causing potentially unbounded wait. Also, no context cancellation check.
- for {
- logger.LogDebug(c, fmt.Sprintf("asyncTaskWait step %d/%d, wait %d seconds", step, maxStep, waitSeconds))
- step++
- rsp, err, body := updateTask(info, taskID)
+ ctx := c.Request.Context()
+ for {
+ select {
+ case <-ctx.Done():
+ return nil, nil, fmt.Errorf("msAsyncTaskWait canceled: %w", ctx.Err())
+ default:
+ }
+ logger.LogDebug(c, fmt.Sprintf("asyncTaskWait step %d/%d, wait %d seconds", step, maxStep, waitSeconds))
+ step++
+ rsp, err, body := updateTask(info, taskID)
responseBody = body
if err != nil {
logger.LogWarn(c, "asyncTaskWait UpdateTask err: "+err.Error())
- time.Sleep(time.Duration(waitSeconds) * time.Second)
- continue
+ if step >= maxStep {
+ return nil, responseBody, fmt.Errorf("msAsyncTaskWait timeout after %d attempts: %w", step, err)
+ }
+ time.Sleep(time.Duration(waitSeconds) * time.Second)
+ continue
}
@@
- if step >= maxStep {
- break
- }
+ if step >= maxStep {
+ return nil, responseBody, fmt.Errorf("msAsyncTaskWait timeout after %d attempts", step)
+ }
time.Sleep(time.Duration(waitSeconds) * time.Second)
}
-
- return nil, nil, fmt.Errorf("msAsyncTaskWait timeout")Also applies to: 124-131
🤖 Prompt for AI Agents
In relay/channel/modelscope/image.go around lines 99-108 (and similarly
124-131), the polling loop currently continues on updateTask errors without
incrementing/terminating or honoring context cancellation; change the logic so
that on any updateTask error you still increment the step counter and check
against maxStep (and exit/return after reaching max attempts), and also check
the provided context for cancellation before sleeping/continuing (use a select
on ctx.Done() to abort promptly). Ensure the sleep/wait path uses the
context-aware select so cancellation stops waiting, and centralize the maxStep
check so both success and error paths enforce the attempt limit.
| if msResponse.TaskStatus != "SUCCEED" { | ||
| return types.WithOpenAIError(types.OpenAIError{ | ||
| Message: "Unknown ModelScope Image API error", | ||
| Type: "ms_error", | ||
| Param: "", | ||
| Code: 400, | ||
| }, resp.StatusCode), nil | ||
| } |
There was a problem hiding this comment.
Propagate a meaningful HTTP status and upstream details on failure
Using the initial response’s StatusCode here can be misleading (often 202). Prefer 502 and include task status and a snippet of the last body for debugging.
- if msResponse.TaskStatus != "SUCCEED" {
- return types.WithOpenAIError(types.OpenAIError{
- Message: "Unknown ModelScope Image API error",
- Type: "ms_error",
- Param: "",
- Code: 400,
- }, resp.StatusCode), nil
- }
+ if msResponse.TaskStatus != "SUCCEED" {
+ snippet := string(originRespBody)
+ if len(snippet) > 512 {
+ snippet = snippet[:512] + "…"
+ }
+ return types.WithOpenAIError(types.OpenAIError{
+ Message: fmt.Sprintf("ModelScope image task not succeeded (status=%s): %s", msResponse.TaskStatus, snippet),
+ Type: "ms_error",
+ Code: "upstream_task_not_succeeded",
+ }, http.StatusBadGateway), nil
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In relay/channel/modelscope/image.go around lines 182–189, the code returns the
original resp.StatusCode (often 202) on msResponse.TaskStatus != "SUCCEED";
change it to return a 502 and include upstream details: construct the
OpenAIError.Message to include the ModelScope task status and a short snippet of
the last response body (truncate to a safe length, e.g., 256–512 chars) for
debugging, set the OpenAIError.Code and the returned HTTP status to 502, and
ensure no sensitive data is leaked when building the snippet.
| fullTextResponse := responseMS2OpenAIImage(c, msResponse, originRespBody, info, responseFormat) | ||
| jsonResponse, err := common.Marshal(fullTextResponse) | ||
| if err != nil { | ||
| return types.NewError(err, types.ErrorCodeBadResponseBody), nil | ||
| } | ||
| service.IOCopyBytesGracefully(c, resp, jsonResponse) | ||
| return nil, &dto.Usage{} |
There was a problem hiding this comment.
Avoid relaying upstream 202; always send 200 with our final JSON
Passing resp to IOCopyBytesGracefully forwards upstream headers and status (often 202), which mismatches the final body.
- service.IOCopyBytesGracefully(c, resp, jsonResponse)
+ // Write final synthesized JSON with 200 OK
+ service.IOCopyBytesGracefully(c, nil, jsonResponse)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fullTextResponse := responseMS2OpenAIImage(c, msResponse, originRespBody, info, responseFormat) | |
| jsonResponse, err := common.Marshal(fullTextResponse) | |
| if err != nil { | |
| return types.NewError(err, types.ErrorCodeBadResponseBody), nil | |
| } | |
| service.IOCopyBytesGracefully(c, resp, jsonResponse) | |
| return nil, &dto.Usage{} | |
| fullTextResponse := responseMS2OpenAIImage(c, msResponse, originRespBody, info, responseFormat) | |
| jsonResponse, err := common.Marshal(fullTextResponse) | |
| if err != nil { | |
| return types.NewError(err, types.ErrorCodeBadResponseBody), nil | |
| } | |
| // Write final synthesized JSON with 200 OK | |
| service.IOCopyBytesGracefully(c, nil, jsonResponse) | |
| return nil, &dto.Usage{} |
🤖 Prompt for AI Agents
In relay/channel/modelscope/image.go around lines 191-197, the current code
forwards upstream headers/status by passing resp into
service.IOCopyBytesGracefully which can leave a 202 status; instead explicitly
set resp status to 200, set the Content-Type to "application/json" (and any
other intended headers), and write the jsonResponse bytes directly to resp
(e.g., call resp.WriteHeader(200) then resp.Write(jsonResponse)) so upstream
headers/status are not relayed. Keep the same return values after writing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
relay/channel/modelscope/image.go (4)
37-44: Don’t fail on single Extra parse; skip invalid keys and coerce non-RawMessage.Your log says “skip” but the code returns an error. Gracefully continue and handle non-
json.RawMessagevalues.- for key, target := range fieldMappings { - if value, ok := request.Extra[key]; ok { - if err := json.Unmarshal(value, target); err != nil { - logger.LogWarn(context.Background(), fmt.Sprintf("failed to unmarshal %s, skip set to request", key)) - return nil, err - } - } - } + for key, target := range fieldMappings { + raw, ok := request.Extra[key] + if !ok { + continue + } + var b []byte + switch v := any(raw).(type) { + case json.RawMessage: + b = v + case []byte: + b = v + default: + tmp, mErr := json.Marshal(v) + if mErr != nil { + logger.LogWarn(context.Background(), fmt.Sprintf("extra[%s] marshal failed: %v", key, mErr)) + continue + } + b = tmp + } + if err := json.Unmarshal(b, target); err != nil { + logger.LogWarn(context.Background(), fmt.Sprintf("extra[%s] unmarshal failed: %v (skipped)", key, err)) + continue + } + }
98-129: Polling can spin indefinitely; enforce max attempts and honor cancellation.On error path you
continuebefore the maxStep check; no ctx cancel either. This can hang requests.- for { - logger.LogDebug(c, fmt.Sprintf("asyncTaskWait step %d/%d, wait %d seconds", step, maxStep, waitSeconds)) - step++ - rsp, err, body := updateTask(info, taskID) - responseBody = body - if err != nil { - logger.LogWarn(c, "asyncTaskWait UpdateTask err: "+err.Error()) - time.Sleep(time.Duration(waitSeconds) * time.Second) - continue - } + ctx := c.Request.Context() + for { + select { + case <-ctx.Done(): + return nil, nil, fmt.Errorf("msAsyncTaskWait canceled: %w", ctx.Err()) + default: + } + logger.LogDebug(c, fmt.Sprintf("asyncTaskWait step %d/%d, wait %d seconds", step, maxStep, waitSeconds)) + step++ + rsp, err, body := updateTask(ctx, info, taskID) + responseBody = body + if err != nil { + logger.LogWarn(c, "asyncTaskWait UpdateTask err: "+err.Error()) + if step >= maxStep { + return nil, responseBody, fmt.Errorf("msAsyncTaskWait timeout after %d attempts: %w", step, err) + } + select { + case <-ctx.Done(): + return nil, responseBody, fmt.Errorf("msAsyncTaskWait canceled: %w", ctx.Err()) + case <-time.After(time.Duration(waitSeconds) * time.Second): + } + continue + } @@ - if step >= maxStep { - break - } - time.Sleep(time.Duration(waitSeconds) * time.Second) - } - - return nil, nil, fmt.Errorf("msAsyncTaskWait timeout") + if step >= maxStep { + return nil, responseBody, fmt.Errorf("msAsyncTaskWait timeout after %d attempts", step) + } + select { + case <-ctx.Done(): + return nil, responseBody, fmt.Errorf("msAsyncTaskWait canceled: %w", ctx.Err()) + case <-time.After(time.Duration(waitSeconds) * time.Second): + } + }
181-188: Return 502 with upstream details when task not SUCCEED.Avoid relaying 202/400; include status and a safe snippet for diagnostics.
- if msResponse.TaskStatus != "SUCCEED" { - return types.WithOpenAIError(types.OpenAIError{ - Message: "Unknown ModelScope Image API error", - Type: "ms_error", - Param: "", - Code: 400, - }, resp.StatusCode), nil - } + if msResponse.TaskStatus != "SUCCEED" { + snippet := string(originRespBody) + if len(snippet) > 512 { + snippet = snippet[:512] + "…" + } + return types.WithOpenAIError(types.OpenAIError{ + Message: fmt.Sprintf("ModelScope image task not succeeded (status=%s): %s", msResponse.TaskStatus, snippet), + Type: "ms_error", + Code: "upstream_task_not_succeeded", + }, http.StatusBadGateway), nil + }
195-196: Don’t forward upstream 202; write synthesized JSON with 200 and Content-Type.Passing
respmakes IOCopy propagate upstream status/headers (often 202). Send 200 with our final body.- service.IOCopyBytesGracefully(c, resp, jsonResponse) + c.Writer.Header().Set("Content-Type", "application/json") + service.IOCopyBytesGracefully(c, nil, jsonResponse)
🧹 Nitpick comments (2)
relay/channel/modelscope/image.go (2)
50-61: Thread cancellation: pass ctx to updateTask and set Accept header.Use request context to allow prompt cancellation and add explicit JSON Accept.
-func updateTask(info *relaycommon.RelayInfo, taskID string) (*MSImageResponse, error, []byte) { +func updateTask(ctx context.Context, info *relaycommon.RelayInfo, taskID string) (*MSImageResponse, error, []byte) { @@ - req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) @@ req.Header.Set("Authorization", "Bearer "+info.ApiKey) + req.Header.Set("Accept", "application/json")And update the call site in asyncTaskWait to pass
ctx(see next comment).
137-154: Align b64_json mode with OpenAI behavior (omit URL when returning base64).When
response_format == "b64_json", leaveUrlempty to avoid confusing clients expecting one field.- var b64Json string + var b64Json string if responseFormat == "b64_json" { _, b64, err := service.GetImageFromUrl(data) if err != nil { logger.LogError(c, "get_image_data_failed: "+err.Error()) continue } b64Json = b64 } else { b64Json = "" } - imageResponse.Data = append(imageResponse.Data, dto.ImageData{ - Url: data, - B64Json: b64Json, - RevisedPrompt: "", - }) + urlStr := data + if responseFormat == "b64_json" { + urlStr = "" + } + imageResponse.Data = append(imageResponse.Data, dto.ImageData{ + Url: urlStr, + B64Json: b64Json, + RevisedPrompt: "", + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
relay/channel/modelscope/image.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
relay/channel/modelscope/image.go (11)
relay/channel/modelscope/dto.go (2)
MSImageRequest(9-18)MSImageResponse(3-7)common/json.go (2)
Unmarshal(9-11)Marshal(21-23)logger/logger.go (4)
LogWarn(61-63)LogJson(150-157)LogDebug(69-73)LogError(65-67)relay/common/relay_info.go (1)
RelayInfo(75-122)relay/constant/relay_mode.go (1)
RelayModeImagesGenerations(14-14)common/sys_log.go (1)
SysLog(11-14)service/image.go (1)
GetImageFromUrl(68-116)dto/openai_image.go (1)
ImageData(170-174)types/error.go (4)
NewAPIError(87-95)NewOpenAIError(226-249)NewError(204-224)WithOpenAIError(277-300)dto/openai_response.go (1)
Usage(222-235)service/http.go (2)
CloseResponseBodyGracefully(15-23)IOCopyBytesGracefully(25-60)
🔇 Additional comments (1)
relay/channel/modelscope/image.go (1)
66-77: Good fix: timeout and non-2xx handling added.Client timeout + status-code check resolves the earlier hanging/opaque error risk.
1 participant
增加modelscope的推理API支持
包括
modelscope应该暂时只有这两个类型的大模型支持。
Summary by CodeRabbit