-
Notifications
You must be signed in to change notification settings - Fork 359
Move the Khanmigo data functions to Perseus Core for Server Side Scoring initiative #3091
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
base: main
Are you sure you want to change the base?
Conversation
…re for Server Side Scoring initiative This PR moves getAnswersFromWidget, injectWidgets, and a handful of helper functions to Perseus Core so that they are available for the Perseus service. We will be creating a new API endpoint to call these functions, so that we can support Khanmigo's move to keeping answerful data server-side. It also created the new getPerseusAIData function, which will be used for the new API Endpoint. Issue: LEMS-3766 Test plan: Tests pass
🗄️ Schema Change: No Changes ✅ |
…ons to Perseus Core for Server Side Scoring initiative
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +48 B (+0.01%) Total Size: 499 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (fbcbdbd) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3091If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3091If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3091 |
| expect(content).toEqual("Content with a radio\nchoice 1\nchoice 2"); | ||
| }); | ||
|
|
||
| it("should inject radio widget into the content with randomization note", () => { |
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.
We could solve this randomization note eventually, but it would require us to pass in the shuffle seed data and translated strings - so we'll look into that at some point in the future.
… widget, after discovering we're missing a few others.
| @@ -0,0 +1,629 @@ | |||
| import {keys} from "@khanacademy/wonder-stuff-core"; | |||
|
|
|||
| import type { | |||
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.
These were directly moved from extract-perseus-data.
| return context; | ||
| } | ||
|
|
||
| /** |
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.
This is the new function for our AI to use.
handeyeco
left a comment
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.
My understanding is that this mostly just moving things which is good to me!
We added getPerseusAIData. Would it be worth adding a simple test for that? (Or maybe there is one and I didn't see it)
@handeyeco Yes! There's some tests in the new test file, but I think github likes to hide it until you explicitly expand it. |
Summary:
This PR moves
getAnswersFromWidget,injectWidgets, and a handful of helper functions to Perseus Core so that they are available for the Perseus service. We will be creating a new API endpoint to call these functions, so that we can support Khanmigo's move to keeping answerful data server-side.It also created the new
getPerseusAIDatafunction, which will be used for the new API Endpoint.I'm happy to take any direction on the name, as "getPromptJSON" didn't feel the most descriptive or correct to myself or the AI team. I don't feel like I've nailed the perfect name either, but it seemed clearer at least.
Issue: LEMS-3766
Test plan: