-
Notifications
You must be signed in to change notification settings - Fork 127
Add openai feedback support #942
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
Merged
umaannamalai
merged 11 commits into
develop-openai-instrumentation
from
add-openai-feedback
Nov 6, 2023
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1123c05
Add get_ai_message_ids & message id capturing
hmstepanek 4619c90
Add tests
hmstepanek bd25144
Merge branch 'develop-openai-instrumentation' into add-openai-feedback
mergify[bot] 0bc5a64
Remove generator
hmstepanek 06761de
Add tests for conversation id unset
hmstepanek de0b469
Merge branch 'develop-openai-instrumentation' into add-openai-feedback
mergify[bot] 694449c
Merge branch 'add-openai-feedback' of github.com:newrelic/newrelic-py…
hmstepanek 5e90108
Add error code to mocked responses
hmstepanek 86eb710
Merge branch 'develop-openai-instrumentation' into add-openai-feedback
mergify[bot] 1a745b2
Remove bedrock tests
hmstepanek a0079fb
Merge branch 'develop-openai-instrumentation' into add-openai-feedback
umaannamalai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,211 @@ | ||
# Copyright 2010 New Relic, Inc. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import openai | ||
from testing_support.fixtures import reset_core_stats_engine | ||
|
||
from newrelic.api.background_task import background_task | ||
from newrelic.api.ml_model import get_ai_message_ids | ||
from newrelic.api.transaction import add_custom_attribute, current_transaction | ||
|
||
_test_openai_chat_completion_messages_1 = ( | ||
{"role": "system", "content": "You are a scientist."}, | ||
{"role": "user", "content": "What is 212 degrees Fahrenheit converted to Celsius?"}, | ||
) | ||
_test_openai_chat_completion_messages_2 = ( | ||
{"role": "system", "content": "You are a mathematician."}, | ||
{"role": "user", "content": "What is 1 plus 2?"}, | ||
) | ||
expected_message_ids_1 = [ | ||
{ | ||
"conversation_id": "my-awesome-id", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059ccd", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTemv-0", | ||
}, | ||
{ | ||
"conversation_id": "my-awesome-id", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059ccd", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTemv-1", | ||
}, | ||
{ | ||
"conversation_id": "my-awesome-id", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059ccd", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTemv-2", | ||
}, | ||
] | ||
|
||
expected_message_ids_1_no_conversation_id = [ | ||
{ | ||
"conversation_id": "", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059ccd", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTemv-0", | ||
}, | ||
{ | ||
"conversation_id": "", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059ccd", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTemv-1", | ||
}, | ||
{ | ||
"conversation_id": "", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059ccd", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTemv-2", | ||
}, | ||
] | ||
expected_message_ids_2 = [ | ||
{ | ||
"conversation_id": "my-awesome-id", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059aad", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTeat-0", | ||
}, | ||
{ | ||
"conversation_id": "my-awesome-id", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059aad", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTeat-1", | ||
}, | ||
{ | ||
"conversation_id": "my-awesome-id", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059aad", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTeat-2", | ||
}, | ||
] | ||
expected_message_ids_2_no_conversation_id = [ | ||
{ | ||
"conversation_id": "", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059aad", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTeat-0", | ||
}, | ||
{ | ||
"conversation_id": "", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059aad", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTeat-1", | ||
}, | ||
{ | ||
"conversation_id": "", | ||
"request_id": "49dbbffbd3c3f4612aa48def69059aad", | ||
"message_id": "chatcmpl-87sb95K4EF2nuJRcTs43Tm9ntTeat-2", | ||
}, | ||
] | ||
|
||
|
||
@reset_core_stats_engine() | ||
@background_task() | ||
def test_get_ai_message_ids_when_nr_message_ids_not_set(): | ||
message_ids = get_ai_message_ids("request-id-1") | ||
assert message_ids == [] | ||
|
||
|
||
@reset_core_stats_engine() | ||
def test_get_ai_message_ids_outside_transaction(): | ||
message_ids = get_ai_message_ids("request-id-1") | ||
assert message_ids == [] | ||
|
||
|
||
@reset_core_stats_engine() | ||
@background_task() | ||
def test_get_ai_message_ids_mulitple_async(loop, set_trace_info): | ||
set_trace_info() | ||
add_custom_attribute("conversation_id", "my-awesome-id") | ||
|
||
async def _run(): | ||
res1 = await openai.ChatCompletion.acreate( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_1, temperature=0.7, max_tokens=100 | ||
) | ||
res2 = await openai.ChatCompletion.acreate( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_2, temperature=0.7, max_tokens=100 | ||
) | ||
return [res1, res2] | ||
|
||
results = loop.run_until_complete(_run()) | ||
|
||
message_ids = [m for m in get_ai_message_ids(results[0].id)] | ||
assert message_ids == expected_message_ids_1 | ||
|
||
message_ids = [m for m in get_ai_message_ids(results[1].id)] | ||
assert message_ids == expected_message_ids_2 | ||
|
||
# Make sure we aren't causing a memory leak. | ||
transaction = current_transaction() | ||
assert not transaction._nr_message_ids | ||
|
||
|
||
@reset_core_stats_engine() | ||
@background_task() | ||
def test_get_ai_message_ids_mulitple_async_no_conversation_id(loop, set_trace_info): | ||
set_trace_info() | ||
|
||
async def _run(): | ||
res1 = await openai.ChatCompletion.acreate( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_1, temperature=0.7, max_tokens=100 | ||
) | ||
res2 = await openai.ChatCompletion.acreate( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_2, temperature=0.7, max_tokens=100 | ||
) | ||
return [res1, res2] | ||
|
||
results = loop.run_until_complete(_run()) | ||
|
||
message_ids = [m for m in get_ai_message_ids(results[0].id)] | ||
assert message_ids == expected_message_ids_1_no_conversation_id | ||
|
||
message_ids = [m for m in get_ai_message_ids(results[1].id)] | ||
assert message_ids == expected_message_ids_2_no_conversation_id | ||
|
||
# Make sure we aren't causing a memory leak. | ||
transaction = current_transaction() | ||
assert not transaction._nr_message_ids | ||
|
||
|
||
@reset_core_stats_engine() | ||
@background_task() | ||
def test_get_ai_message_ids_mulitple_sync(set_trace_info): | ||
set_trace_info() | ||
add_custom_attribute("conversation_id", "my-awesome-id") | ||
|
||
results = openai.ChatCompletion.create( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_1, temperature=0.7, max_tokens=100 | ||
) | ||
message_ids = [m for m in get_ai_message_ids(results.id)] | ||
assert message_ids == expected_message_ids_1 | ||
|
||
results = openai.ChatCompletion.create( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_2, temperature=0.7, max_tokens=100 | ||
) | ||
message_ids = [m for m in get_ai_message_ids(results.id)] | ||
assert message_ids == expected_message_ids_2 | ||
|
||
# Make sure we aren't causing a memory leak. | ||
transaction = current_transaction() | ||
assert not transaction._nr_message_ids | ||
|
||
|
||
@reset_core_stats_engine() | ||
@background_task() | ||
def test_get_ai_message_ids_mulitple_sync_no_conversation_id(set_trace_info): | ||
set_trace_info() | ||
|
||
results = openai.ChatCompletion.create( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_1, temperature=0.7, max_tokens=100 | ||
) | ||
message_ids = [m for m in get_ai_message_ids(results.id)] | ||
assert message_ids == expected_message_ids_1_no_conversation_id | ||
|
||
results = openai.ChatCompletion.create( | ||
model="gpt-3.5-turbo", messages=_test_openai_chat_completion_messages_2, temperature=0.7, max_tokens=100 | ||
) | ||
message_ids = [m for m in get_ai_message_ids(results.id)] | ||
assert message_ids == expected_message_ids_2_no_conversation_id | ||
|
||
# Make sure we aren't causing a memory leak. | ||
transaction = current_transaction() | ||
assert not transaction._nr_message_ids |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 may be worth testing some cases here where one or some combo of these values are
None
. At a minimum, it's likely conversation ID will be empty in many casesThere 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 added some tests for conversation id being unset. I was originally gonna add more tests into this anticipating bedrock and the response_id being unset but I thought it was better to wait on those until we actually get to bedrock.