fix: correct params in llm api call#223
Conversation
Move parameter into body and rename to align with expectation for Xpert API Services.
Following recommendation from Copilot: https://github.com/edx/learning-assistant/pull/222/changes#r3100416910
There was a problem hiding this comment.
Pull request overview
This PR corrects how the conversation history identifier is sent to the LLM backend by removing the outdated Conversation-History-ID header usage and instead including the identifier in the request body for the v2 API format, aligning with current Xpert API Services behavior.
Changes:
- Remove
Conversation-History-IDfrom request headers. - Add
conversation_idto the v2 request body and update request-body construction to accept a conversation id. - Update conversation-id seed format to a stable JSON array string for deterministic UUID generation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
learning_assistant/utils.py |
Moves conversation identifier from headers into the v2 body (conversation_id) and updates deterministic ID seeding format. |
tests/test_utils.py |
Updates request-structure assertions to reflect header removal and new v2 body field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if v2_endpoint_enabled(): | ||
| response_body = { | ||
| 'client_id': getattr(settings, 'CHAT_COMPLETION_CLIENT_ID', 'edx_olc_la'), | ||
| 'system_message': prompt_template, | ||
| 'messages': messages, | ||
| 'external_id': str(user_id), | ||
| 'conversation_id': convo_id, |
There was a problem hiding this comment.
get_chat_response() selects the endpoint using v2_endpoint_enabled(), but create_request_body() also calls v2_endpoint_enabled() internally to choose the payload shape. If the waffle flag value changes between these calls (or behaves differently per call), the code can send a v2 payload to the v1 endpoint (or vice versa). Consider computing the v2-enabled boolean once in get_chat_response() and passing it into create_request_body() so endpoint selection and payload structure stay consistent.
| conversation_history_id = create_conversation_history_id(user_id, course_run_id) | ||
| body = create_request_body(prompt_template, message_list, user_id, conversation_history_id) |
There was a problem hiding this comment.
create_request_body() now requires convo_id, but the v1 request body ignores it. This forces get_chat_response() to always compute a conversation id even when the v2 endpoint is disabled, and makes create_request_body() harder to reuse correctly. Consider making convo_id optional (e.g., defaulting to None) and only generating/passing it when building the v2 payload.
| mock_convo_id_seed = json.dumps([str(self.user_id), str(self.course_id)], separators=(',', ':')) | ||
| mock_convo_hist_id = str(uuid.uuid5(uuid.NAMESPACE_URL, str(mock_convo_id_seed))) |
There was a problem hiding this comment.
This test re-implements the conversation-id generation logic inline (json.dumps(...) + uuid.uuid5(...)). To avoid future drift from create_conversation_history_id() (which is part of what you’re validating), consider importing and calling create_conversation_history_id(self.user_id, self.course_id) here instead of duplicating the algorithm.
During validation of #222 in production, it was found that the
Conversation-History-IDhad been implemented based on outdated documentation. This change moves that value to the correct location based on the implementation within Xpert API Services today.