feat: integrate system prompt #47
Conversation
alangsto
force-pushed
the
alangsto/integrate_prompt_template
branch
from
d7f7add to
a23aa4e
Compare
| @@ -45,3 +45,10 @@ def block_get_children(block): | |||
| # pylint: disable=import-error, import-outside-toplevel | |||
| from openedx.core.lib.graph_traversals import get_children | |||
| return get_children(block) | |||
|
|
|||
|
|
|||
| def get_cache_course_run_data(course_run_id, fields): | |||
There was a problem hiding this comment.
I want to leave some documentation about what we're doing here and why - that we need to get the course ID and not the course run ID, and that this is the easiest place to get it from the platform. I think this could be a potentially unclear choice to future readers. I feel like an ADR might be overkill, so what do you think about leaving a brief comment about why we're using the catalog cache?
There was a problem hiding this comment.
Ah, I see you left a comment in utils.py::get_course_key 👍 . I think it might make a little more sense as a comment closer to the actual code that uses the catalog utility method, but thank you for including that comment!
learning_assistant/utils.py
Outdated
| @@ -47,7 +56,37 @@ def get_reduced_message_list(system_list, message_list): | |||
| return new_message_list | |||
|
|
|||
|
|
|||
| def get_chat_response(system_list, message_list): | |||
| def get_course_key(courserun_id): | |||
There was a problem hiding this comment.
Nit: There's a push to use "course key" or "course run key" when referring to instances of the CourseKey class and to use course id" or "course run id" when referring to the string representations of those keys in order to be more clear about the type of the data. I think it could be more accurate to title this get_course_id`, but let me know if you disagree.
| total_system_tokens = sum(_estimated_message_tokens(system_message['content']) for system_message in system_list) | ||
| total_system_tokens = ( | ||
| _estimated_message_tokens(prompt_template) | ||
| + _estimated_message_tokens('.' * 40) # average number of characters per course name is 40 |
There was a problem hiding this comment.
Could you leave a comment about why we're estimating these and including them in the count, please?
alangsto
force-pushed
the
alangsto/integrate_prompt_template
branch
from
a23aa4e to
7b0b615
Compare
learning_assistant/api.py
Outdated
| """ | ||
| unit_content = '' | ||
| if unit_usage_key: | ||
| content_length, unit_content = get_block_content(request, user_id, course_id, unit_usage_key) |
There was a problem hiding this comment.
Do we want to log anything about content length here? It might be a useful way to estimate how long each piece of content is.
There was a problem hiding this comment.
That's a good idea to get a sense of the length. I can see us needing that information in the future.
alangsto
force-pushed
the
alangsto/integrate_prompt_template
branch
3 times, most recently
from
0e5907b to
fb27c83
Compare
|
This PR should not be merged until https://2u-internal.atlassian.net/browse/COSMO-144 is completed. |
alangsto
force-pushed
the
alangsto/integrate_prompt_template
branch
from
233826d to
d0d1a45
Compare
COSMO-117
Replace use of course specific prompts with a prompt template stored in a django setting.