-
Notifications
You must be signed in to change notification settings - Fork 102
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
Add support for openai v1 completions #1006
Add support for openai v1 completions #1006
Conversation
ad8daf4
to
8da4227
Compare
🦙 MegaLinter status: ❌ ERROR
See detailed report in MegaLinter reports |
008e0ba
to
1df5b05
Compare
1df5b05
to
f8b3703
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-ai-limited-preview #1006 +/- ##
=============================================================
Coverage ? 82.10%
=============================================================
Files ? 191
Lines ? 20132
Branches ? 3495
=============================================================
Hits ? 16530
Misses ? 2598
Partials ? 1004 ☔ View full report in Codecov by Sentry. |
newrelic/hooks/mlmodel_openai.py
Outdated
# param, etc in the error response, they are not populated on the exception | ||
# object so grab them from the response object instead. | ||
content = getattr(response, "content", b"{}") | ||
response = json_decode(content.decode("utf-8")).get("error", {}) |
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 looks like the status code and other notice error attributes may be available on exc.body
exc.message
.
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've changed it to use the body. The exc.message is not the message we are looking for. 😂 It includes the json inside of it which is the same as the default message that we would grab without overriding it so I'm using the message from the body instead still.
body = getattr(exc, "body", {}) | ||
notice_error_attributes = { | ||
"http.statusCode": getattr(exc, "status_code", "") or "", | ||
"error.message": body.get("message", "") or "", |
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.
Should we check if body has anything in it to avoid NoneType errors? The body attribute would only be on OpenAI errors and if we hit a standard error like TypeError this may not be populated?
{"role": "system", "content": "You are a scientist."}, | ||
{"role": "user", "content": "What is 212 degrees Fahrenheit converted to Celsius?"}, | ||
) | ||
|
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.
In v0, we also had the test case of no API key being supplied at all - it may be good to add that here so we're consistent with tests cases across versions.
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 just throws the same error as the api_key = "DEADBEEF" case except it's None so I figured we didn't really need to test for that as it's not different.
7b98c51
into
develop-ai-limited-preview
Overview
Adds support for openai v1 completions