[BB-9845] Coaching AI XBlock #20
Conversation
pkulkark
force-pushed
the
pooja/bb-9845-changes
branch
6 times, most recently
from
215bd87 to
3811ed0
Compare
pkulkark
changed the base branch from
artur/shortanswer-with-coach
to
pooja/bb-9936-changes
There was a problem hiding this comment.
@pkulkark I left several comments - please see inline.
Also, please update the PR description to add testing instructions, link to the ticket, etc.
Thanks.
pkulkark
force-pushed
the
pooja/bb-9845-changes
branch
2 times, most recently
from
0e2014f to
5004a17
Compare
pkulkark
changed the base branch from
pooja/bb-9936-changes
to
artur/all-changes
pkulkark
changed the base branch from
artur/all-changes
to
pooja/bb-9936-changes
pkulkark
force-pushed
the
pooja/bb-9845-changes
branch
8 times, most recently
from
2791b22 to
f0b7eb0
Compare
pkulkark
changed the base branch from
pooja/bb-9936-changes
to
artur/all-changes
pkulkark
changed the base branch from
artur/all-changes
to
pooja/bb-9936-changes
pkulkark
changed the base branch from
pooja/bb-9936-changes
to
artur/all-changes
pkulkark
changed the base branch from
artur/all-changes
to
pooja/bb-9936-changes
pkulkark
force-pushed
the
pooja/bb-9845-changes
branch
from
d593056 to
34a8a52
Compare
pkulkark
changed the base branch from
pooja/bb-9936-changes
to
artur/all-changes
pkulkark
force-pushed
the
pooja/bb-9845-changes
branch
from
e121a81 to
b6bc8cb
Compare
Also remove normalization for evaluation_criteria
pkulkark
force-pushed
the
pooja/bb-9845-changes
branch
from
b6bc8cb to
a33db98
Compare
samuelallan72
left a comment
samuelallan72
left a comment
There was a problem hiding this comment.
In general, this works fine, and I think ok to merge considering we're planning some follow up refactoring.
I left some suggestions and comments though, mostly focusing on documentation since the fields are complex. Please consider which can be addressed now, or which can be left for future refactoring; up to your discretion.
👍
- I tested this: tested this on the sandbox
- I read through the code
- I checked for accessibility issues
- Includes documentation
| display_name=_("Main character prompt"), | ||
| help=_( | ||
| "Defines how the main character (left pane) behaves. " | ||
| "You can use Jinja variables: character_data, scenario_data." |
There was a problem hiding this comment.
character_data is not documented anywhere for the user. We should document the available fields and an example somewhere.
| conversation_format = String( | ||
| display_name=_("Conversation format template"), | ||
| help=_( | ||
| "Template used to format the conversation, appended to all prompts" |
There was a problem hiding this comment.
Please document this more. The format is confusing to me - it seems to be jinja templated xml?
| default=SAMPLE_CHARACTER_PROMPT, | ||
| ) | ||
|
|
||
| conversation_format = String( |
There was a problem hiding this comment.
Some settings like conversation_format and message_content_tag are not editable. Why are they settings then? Can we simplify the code by making them constants?
Co-authored-by: Samuel Allan <[email protected]>
2 participants
Description
This PR implements the Coaching AI XBlock as per the designs and discussions.
Supporting information
OpenCraft Internal Jira task: BB-9845
Testing instructions
coach_ai_evalin the advanced modules list in Studio and create a new Coach AI XBlock component.