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

Support openai>=1.1.2 #1260

Merged

Conversation

jieguangzhou
Copy link
Collaborator

@jieguangzhou jieguangzhou commented Nov 9, 2023

Description

#1261

Related Issues

Checklist

  • Is this code covered by new or existing unit tests or integration tests?
  • Did you run make test successfully?
  • Do new classes, functions, methods and parameters all have docstrings?
  • Were existing docstrings updated, if necessary?
  • Was external documentation updated, if necessary?

Additional Notes or Comments

@jieguangzhou jieguangzhou force-pushed the impv/openai-new-version branch 4 times, most recently from 18d8719 to 4702796 Compare November 9, 2023 17:05
Copy link
Contributor

@thejumpman2323 thejumpman2323 left a comment

Choose a reason for hiding this comment

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

some small removal

superduperdb/ext/openai/model.py Outdated Show resolved Hide resolved
superduperdb/ext/openai/model.py Outdated Show resolved Hide resolved
@jieguangzhou jieguangzhou force-pushed the impv/openai-new-version branch from 4702796 to 4b08b0d Compare November 10, 2023 08:31
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (34830a7) 80.33% compared to head (4b08b0d) 80.65%.
Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1260      +/-   ##
==========================================
+ Coverage   80.33%   80.65%   +0.31%     
==========================================
  Files          95       97       +2     
  Lines        6602     6689      +87     
==========================================
+ Hits         5304     5395      +91     
+ Misses       1298     1294       -4     
Files Coverage Δ
superduperdb/__init__.py 100.00% <100.00%> (ø)
superduperdb/base/artifact.py 94.87% <100.00%> (+0.20%) ⬆️
superduperdb/base/config.py 100.00% <ø> (ø)
superduperdb/base/datalayer.py 71.40% <100.00%> (+3.23%) ⬆️
superduperdb/components/model.py 93.66% <100.00%> (+11.00%) ⬆️
superduperdb/ext/torch/model.py 76.50% <ø> (-0.31%) ⬇️
superduperdb/misc/serialization.py 85.86% <100.00%> (+0.92%) ⬆️
superduperdb/misc/superduper.py 76.25% <ø> (ø)
superduperdb/base/build.py 65.85% <66.66%> (-1.65%) ⬇️
superduperdb/base/logger.py 86.95% <80.00%> (-3.05%) ⬇️
... and 4 more

... and 10 files with indirect coverage changes

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

f'{CASSETTE_DIR}/test_batch_translate_async.yaml',
filter_headers=['authorization'],
)
@vcr.use_cassette()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need this meta-data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created

vcr = vcr.VCR(
     path_transformer=lambda x: x + '.yaml',
     match_on=('method', 'path'),
     filter_headers=['authorization'],
     cassette_library_dir=CASSETTE_DIR,
     before_record_request=before_record_request,
     before_record_response=before_record_response,
 )

to handle this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha -- gotcha -- I missed that. This is nice.

Comment on lines +28 to +38
# monkey patch vcr to make it work with upload binary data
def _make_vcr_request(httpx_request, **kwargs):
from vcr.request import Request as VcrRequest

try:
body = httpx_request.read().decode("utf-8")
except UnicodeDecodeError:
body = str(httpx_request.read())
uri = str(httpx_request.url)
headers = dict(httpx_request.headers)
return VcrRequest(httpx_request.method, uri, body, headers)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the latest version, when vcr decodes the uploaded binary file, an error occurs. We need to patch it here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will get a error on decode for the binary data

'''
VCR filter function to only record the PNG signature in the response.

This is necessary because the response is a PNG which can be quite large.
'''
if 'body' not in response:
Copy link
Collaborator Author

@jieguangzhou jieguangzhou Nov 10, 2023

Choose a reason for hiding this comment

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

In the latest version, some responses lack the body field, which will cause an error in the following logic.

@jieguangzhou jieguangzhou merged commit 07787bb into superduper-io:main Nov 10, 2023
2 checks passed
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.

fix the openai code for the new breaking changes in the recent release
4 participants