54 unified response #55
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #55 +/- ##
===========================================
+ Coverage 67.52% 69.56% +2.04%
===========================================
Files 8 8
Lines 234 276 +42
===========================================
+ Hits 158 192 +34
- Misses 65 71 +6
- Partials 11 13 +2 ☔ View full report in Codecov by Sentry. |
|
|
||
| var tokenCount schemas.TokenCount | ||
|
|
||
| message := responseJSON["choices"].([]interface{})[0].(map[string]interface{})["message"].(map[string]interface{}) |
There was a problem hiding this comment.
@mkrueger12 looking at this code I feel like it's worth defining OpenAI Chat Response schema, map the response into it and then remap into the unified chat response schema
There was a problem hiding this comment.
@roma-glushko I can do that. What advantage does it provide?
There was a problem hiding this comment.
@mkrueger12 That would make the implementation more readable at very least and it would give an understanding what the actual provider response might look like, what information it contains in a declarative way 👀
In this specific case, expression like responseJSON["choices"].([]interface{})[0].(map[string]interface{}) means that you are sure that the response has the choices key where you also are sure there is at least one item e.g. [0]. But is this always a case? Because if not I think this code will fail with an exception trying to access that info but in fact getting nil.
If you had a validation via the defined OpenAI chat response, you would ensure that the response has all needed fields which would simply the code here.
Something like that.
|
|
||
| response = schemas.UnifiedChatResponse{ | ||
| ID: responseJSON["id"].(string), | ||
| Created: float64(time.Now().Unix()), |
| Router string `json:"router,omitempty"` | ||
| Model string `json:"model,omitempty"` | ||
| Cached bool `json:"cached,omitempty"` | ||
| ProviderResponse ProviderResponse `json:"provider_response,omitempty"` |
There was a problem hiding this comment.
Do you think ProviderResponse -> ModelResponse would be a good idea?
Updated the Unified schema per the GEP
Updated the doChatRequest func to marshal openai response into the unified response
Tested