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

Fix broken implementation AssistantModify implementation #685

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

qhenkart
Copy link
Contributor

@qhenkart qhenkart commented Mar 14, 2024

This resolves #684
both reverts the changes in #669
and modifies the code to actually match the intention of the #669

Currently the SDK does not match the API and requires all of the existing tools to be provided in every request to avoid an API error or deleting all of the tools. Please see the linked issue for repro steps and use cases including step by step cURL requests to demonstrate the existing functionality of the API

As specified in the inline documentation
A custom marshaller was required here to handle the various use cases of the API request with respect to GO-isms and to avoid breaking changes:

If Tools is nil (or undefined), the field is omitted from the JSON. > this reflects the functionality of the API and reverts the breaking change introduced in #669

If Tools is an empty slice, it's included in the JSON as an empty array ([]). > this matches the intention of #669 by allowing the ability to remove all Tools, something that was broken previously and the linked PR attempted to resolve
If Tools is populated, it's included in the JSON with the elements > and thusly replaces the Tools. This functionality existed already and simply is maintained in this PR

This is important to note because in Golang, omitempty will omit both a nil (uninitialized) slice as well as an empty slice. To provide support to allow both a non-existant slice, and an empty slice, and a populated slice and never a nil slice (see the linked issue to see the API error if it receives nil). A custom Marshaller and aliast was required

Additionally I wrapped the tests block into Run blocks to be able to isolate tests and remove variable shadowing. I also added explicit unit tests for this functionality, and the run blocks were necessary to be able to properly isolate them for testing and development.

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.46%. Comparing base (699f397) to head (5bfa45b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   98.44%   98.46%   +0.01%     
==========================================
  Files          24       24              
  Lines        1353     1366      +13     
==========================================
+ Hits         1332     1345      +13     
  Misses         15       15              
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@sashabaranov sashabaranov left a comment

Choose a reason for hiding this comment

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

Thank you for such a thorough PR!

We might apply a similar approach to a long-standing temperature issue #9

@sashabaranov sashabaranov merged commit 0925563 into sashabaranov:master Mar 15, 2024
3 checks passed
grulex pushed a commit to grulex/go-openai that referenced this pull request Mar 15, 2024
…v#685)

* add custom marshaller, documentation and isolate tests

* fix linter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recent PR breaks requests to modify Assistant (breaks our production deployment)
2 participants