-
Notifications
You must be signed in to change notification settings - Fork 36
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
Update functioncall.go #28
Conversation
support array
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
=======================================
Coverage 79.48% 79.48%
=======================================
Files 6 6
Lines 312 312
=======================================
Hits 248 248
Misses 45 45
Partials 19 19 ☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here. |
@Naist4869 Thank you always for your catch! |
functioncall/functioncall.go
Outdated
for _, p := range params { | ||
if p.Required { | ||
required = append(required, p.Name) | ||
} | ||
props[p.Name] = p | ||
if p.Type == "array" && p.Items != nil { |
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.
This kind of condition handling should be inside MarshalJSON
of Param
itself.
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.
func (p Param) MarshalJSON() ([]byte, error)
"We don't need to change this method; using the default is the best."
"If you have changed func (p Param) MarshalJSON() ([]byte, error), then to implement the original functionality you'll need:
var propMap map[string]any
if err := json.Unmarshal(propData, &propMap); err != nil {
return nil, err
}
return json.Marshal(propMap)
functioncall/functioncall.go
Outdated
schema := map[string]any{ | ||
"type": "array", | ||
"items": p.Items, | ||
"required": required, |
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.
I'm not sure why this required
needs to be here. This required
is supposed to contain all names of required
params within this Params
, not a Param
. Could you give me more guidance here?
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.
Relatedly, do we need if p.Items != nil
condition above?
If we don't need any special operation for array
such as this required
, I think omitempty
for Items
is enough.
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.
I'm not sure why this
required
needs to be here. Thisrequired
is supposed to contain all names ofrequired
params within thisParams
, not aParam
. Could you give me more guidance here?我不确定为什么这个required
需要在这里。这个required
应该包含Params
中required
参数的所有名称,而不是Param
。您能给我更多指导吗?
https://community.openai.com/t/how-to-calculate-the-tokens-when-using-function-call/266573/27
https://github.com/hmarr/openai-chat-tokens
{
messages: [{ role: "user", content: "hello" }],
functions: [
{
name: "get_recipe",
parameters: {
type: "object",
required: ["ingredients", "instructions", "time_to_cook"],
properties: {
ingredients: {
type: "array",
items: {
type: "object",
required: ["name", "unit", "amount"],
properties: {
name: {
type: "string",
},
unit: {
enum: ["grams", "ml", "cups", "pieces", "teaspoons"],
type: "string",
},
amount: {
type: "number",
},
},
},
},
instructions: {
type: "array",
items: {
type: "string",
},
description: "Steps to prepare the recipe (no numbering)",
},
time_to_cook: {
type: "number",
description: "Total time to prepare the recipe in minutes",
},
},
},
},
],
tokens: 106,
},
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.
Relatedly, do we need
if p.Items != nil
condition above? If we don't need any special operation forarray
such as thisrequired
, I thinkomitempty
forItems
is enough.
This is just my coding habit. Using only 'if p.Type == "array"' is also acceptable
functioncall/functioncall.go
Outdated
} | ||
props[p.Name] = schema | ||
} else { | ||
props[p.Name] = p |
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.
When we have MarshalJSON
of Param
, we don't need this if-else switching but simply props[p.Name] = p
, IMO.
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.
It's Recursion
func (p *Param) MarshalJSON() ([]byte, error) {
type Alias Param // Alias not impl MarshalJSON, so not recursion
return json.Marshal(&struct {
*Alias
}{
Alias: (*Alias)(p),
})
}
I tried what you suggested. Please help me verify. |
…or better type specificity.
Now I've got your point. Thank you very much. I'll take time to look in. |
Thank you very much. I might add some change later but thank you for this great catch. |
support array