feat: gate use of v2 endpoint with waffle flag #152
Conversation
|
|
||
|
|
||
| def chat_history_enabled(course_key): | ||
| def v2_endpoint_enabled(): |
There was a problem hiding this comment.
I did not make this a course waffle flag as I don't believe we need a course by course roll out.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
| completion_endpoint = getattr(settings, 'CHAT_COMPLETION_API', None) | ||
| completion_endpoint_key = getattr(settings, 'CHAT_COMPLETION_API_KEY', None) | ||
| if completion_endpoint and completion_endpoint_key: | ||
| headers = {'Content-Type': 'application/json', 'x-api-key': completion_endpoint_key} |
There was a problem hiding this comment.
Question: is there no longer the need for an api key with the new endpoint?
There was a problem hiding this comment.
There's not a need for an api key with the current endpoint! It was added at one point because the Xpert platform team was going to require it, but then it was never enforced.
| } | ||
|
|
||
| if v2_endpoint_enabled(): | ||
| response_body = { | ||
| 'client_id': getattr(settings, 'CHAT_COMPLETION_CLIENT_ID', 'edx_olc_la'), |
There was a problem hiding this comment.
Question: what is the 'edx_olc_la'? Is there a reason why this is hardcoded rather than added as a setting?
There was a problem hiding this comment.
It is added as a setting, the string is just the default if no setting is found
| @@ -126,18 +145,18 @@ def test_message_list_reduced(self): | |||
| """ | |||
| # pass in copy of list, as it is modified as part of the reduction | |||
| reduced_message_list = get_reduced_message_list(self.prompt_template, self.message_list) | |||
| self.assertEqual(len(reduced_message_list), 2) | |||
| self.assertEqual(len(reduced_message_list), 1) | |||
There was a problem hiding this comment.
Question: might be missing this in the implementation but can you clarify why the length goes down by 1 for this assert and the assert below?
There was a problem hiding this comment.
I modified what get_reduced_message_list returns, so that it doesn't include the system prompt.
| @@ -14,6 +14,10 @@ Change Log | |||
| Unreleased | |||
| ********** | |||
|
|
|||
| 4.6.4 - 2025-01-06 | |||
| ****************** | |||
| * Gate use of the Xpert platform v2 endpoint with a waffle flag. | |||
There was a problem hiding this comment.
Nit: would using the v2 endpoint justify considering this a minor release than a patch? So 4.7.0 instead of 4.6.4? Sort of a nit but I wasn't sure how big of a change this would represent api-wise.
There was a problem hiding this comment.
Oh fair point! It's not breaking, but I could see a minor release being more appropriate.
| system_message = {'role': 'system', 'content': prompt_template} | ||
|
|
||
| return [system_message] + new_message_list | ||
| return new_message_list |
There was a problem hiding this comment.
Why did you decide to remove the system message?
Is this a change that is going to affect edX Xpert LA UI?
There was a problem hiding this comment.
I modified this function to allow for the two different request bodies in the v1 vs v2 version of the endpoint. The system message is still included in the request (see
learning-assistant/learning_assistant/utils.py
Lines 61 to 70 in 5c82f55
COSMO-620
The Xpert platform team has released a new v2 endpoint for processing messages. This PR gates use of the v2 endpoint by a new waffle flag.
I also cleaned up some code related to an unused API key and an unused waffle flag related to course content.