Skip to content
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

Several improvements / fixes for functions #320

Merged
merged 11 commits into from
Jul 25, 2023
Merged

Conversation

rzubek
Copy link
Contributor

@rzubek rzubek commented Jul 7, 2023

New: function arguments can now be typed arrays, in addition to string, number, etc. Addresses issue #304 and part of issue #317. See AddArrayParameters().

Fix: error messages coming from the server caused exceptions to be thrown during deserialization; this is because the message field was coming back as an array but the deserializer expected a string.

Merged: PR #312. This code builds on PR 312 so it was incorporated.

kayhantolga and others added 6 commits June 22, 2023 02:43
…ray, instead of a string, for example:

{  "error": {
    "message": [
      "Invalid schema for function 'get_n_day_weather_forecast': In context=('properties', 'format'), array schema missing items"
    ],
    "type": "invalid_request_error",
    "param": null,
    "code": null
  }
}

This causes the JSON deserializer to choke.

There doesn't seem to be an official API documentation for error messages. However, since they appear to be only used for user feedback, I'd propose changing their type to be an unspecified `object`.
@retendo
Copy link

retendo commented Jul 20, 2023

Would be great to have something like this merged 👍

Question:
Wouldn't it be more helpful if items is just this:
public FunctionParameters? Items { get; set; }

Edit:
That wouldn't solve it either as this would not allow arrays of enum values. I guess we could add Enum to FunctionParameters as well.

@remixtedi
Copy link

When are you planning to merge this?

@rzubek
Copy link
Contributor Author

rzubek commented Jul 22, 2023

When are you planning to merge this?

If you're asking me, it's not my repo, I don't have any permissions here. This is waiting for @kayhantolga to review and merge.

@kayhantolga
Copy link
Member

kayhantolga commented Jul 24, 2023

Hi
this is a great improvement, Thanks! Could you please update the code to target https://github.com/betalgo/openai/tree/feature/functionCall branch?
I moved some of the files and made some changes.

Also, I left a comment, please check that.

@@ -13,7 +13,7 @@ public record Error
{
[JsonPropertyName("code")] public string? Code { get; set; }

[JsonPropertyName("message")] public string? Message { get; set; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Do you have any sample responses that show why the change is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please see the sample response in the description of commit 80bde47

It came back as an array of strings (rather than a string) which caused the JSON serializer to blow up

@kayhantolga kayhantolga added the question Further information is requested label Jul 24, 2023
@kayhantolga kayhantolga added this to the 7.2.0 milestone Jul 25, 2023
@kayhantolga
Copy link
Member

@rzubek do you think you can do the changes anytime soon? I want to include your changes into 7.2.0

@rzubek
Copy link
Contributor Author

rzubek commented Jul 25, 2023

Hi @kayhantolga,

This PR is ready to go into dev, I don't think it's waiting for any more changes? Unless I missed something.

Can you merge it to dev first, and then merge in your refactor from PR #334? The refactor is going to cause merge conflicts, and you're by far the best person to resolve them.

@kayhantolga kayhantolga merged commit b0ab261 into betalgo:dev Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants