Move main Scorer and Validator to PerseusScore #2165

handeyeco · 2025-01-28T21:23:28Z

Summary:

You don't have to look at this alone. I would be happy to give you a tour.

I'm sorry, I didn't see a way to break this up into pieces that could be moved independent of one another.

It's honestly not a lot of changes, just changes that needed to happen while moving things.

The main idea:

The original goal of the PR was to move scorePerseusItem
That required moving logic for upgrading widgets
Upgrading required the alignment moves
Since PerseusCore has upgrade logic and PerseusScore has scoring logic, they both needed registries
- We're discussing merging the two packages to reduce the maintenance burden
Since this stuff has to run on the server and the server can't be connected to React, I removed logic that was stuck onto WidgetExports

That's pretty much it: moved scoring logic, moved stuff scoring logic depended on, updated imports, and cleaned up WidgetExports.

Issue: LEMS-2737

Test plan:

Widgets can be upgraded
Widgets can be validated
Widgets can be scored

Virtually no user-facing changes should occur.

github-actions · 2025-01-28T21:28:13Z

Size Change: -765 B (-0.05%)

Total Size: 1.48 MB

Filename	Size	Change
`packages/perseus-core/dist/es/index.js`	46 kB	+2.48 kB (+5.71%)	🔍
`packages/perseus-editor/dist/es/index.js`	688 kB	+14 B (0%)
`packages/perseus-score/dist/es/index.js`	115 kB	+2.29 kB (+2.03%)
`packages/perseus/dist/es/index.js`	376 kB	-5.55 kB (-1.45%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	86.8 kB
`packages/math-input/dist/es/index.js`	77.6 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	5.82 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

handeyeco · 2025-01-28T21:25:53Z

dev/flipbook.tsx

@@ -324,7 +324,6 @@ function GradableRenderer(props: QuestionRendererProps) {
                                const score = scorePerseusItem(
                                    question,
                                    rendererRef.current.getUserInputMap(),
-                                    mockStrings,


Now all validators and scorers should be sending error codes instead of error messages, so we don't need strings anymore.

handeyeco · 2025-01-28T21:26:21Z

packages/perseus-core/src/widgets/graded-group-set/index.ts

    name: "graded-group-set",
    defaultWidgetOptions,
 };

-export default GradedGroupSetWidgetLogic;
+export default gradedGroupSetWidgetLogic;


This were just mistakes from previous PRs.

handeyeco · 2025-01-28T21:26:46Z

packages/perseus-core/src/widgets/image/index.ts

@@ -26,6 +26,8 @@ const defaultWidgetOptions: ImageDefaultWidgetOptions = {
 const imageWidgetLogic: WidgetLogic = {
    name: "image",
    defaultWidgetOptions,
+    supportedAlignments: ["block", "full-width"],
+    defaultAlignment: "block",


Alignment is used during the upgrade process, that's why I moved all of these.

handeyeco · 2025-01-28T21:27:32Z

packages/perseus-core/src/widgets/upgrade.ts

+
+    // Unversioned widgets (pre-July 2014) are all implicitly 0.0
+    const initialVersion = oldWidgetInfo.version || {major: 0, minor: 0};
+    const latestVersion = getCurrentVersion(type);


This is a change, basically just adding a helper here.

handeyeco · 2025-01-28T21:27:45Z

packages/perseus-core/src/widgets/upgrade.ts

+    // some point, and we would like to not break when that happens).
+    let newEditorOptions = _.clone(oldWidgetInfo.options) || {};
+
+    const upgradePropsMap = getWidgetOptionsUpgrades(type);


This is a change, basically just adding a helper here.

handeyeco · 2025-01-28T21:42:38Z

packages/perseus-score/src/widgets/widget-registry.ts

+    return widgets[name]?.scorer ?? null;
+};
+
+registerWidget(


Jeremy and I are talking about merging PerseusScore and PerseusCore so there's only one registry for the logic (along with a registry for the editors and a registry for the UI). In the meantime...

handeyeco · 2025-01-28T21:43:53Z

packages/perseus/src/types.ts

@@ -569,9 +538,6 @@ export type WidgetExports<
     * This key defaults to `{major: 0, minor: 0}` if not provided.
     */
    version?: Version;
-    supportedAlignments?: ReadonlyArray<Alignment>;
-    defaultAlignment?: Alignment;
-    getDefaultAlignment?: () => Alignment;


I got rid of getDefaultAlignment. It didn't seem like any of the components used it?

You're right! There's a shared getDefaultAlignment that's used, but nothing calls this function on individual widgets.

handeyeco · 2025-01-28T21:44:59Z

packages/perseus/src/widgets/explanation/explanation.test.ts

-        it("should always return 0 points", async () => {
-            const result = ExplanationWidgetExports?.scorer?.();
-
-            expect(result).toHaveBeenAnsweredCorrectly({


idk, this seemed to be more of a test for noopScorer which has its own tests.

Agreed. Doesn't actually seem like a useful test.

handeyeco · 2025-01-28T21:45:43Z

packages/perseus/src/widgets/group/group.test.tsx

-                  "countChoices": undefined,
-                  "deselectEnabled": undefined,
+                  "countChoices": false,
+                  "deselectEnabled": false,


Okay, this is worth pointing out. I have no idea why this changed and I can't figure it out. To me it seemed that these should have been false though? So IDK.

Does this test end up upgrading the widget options ... and in doing so now seat some defaults that it didn't before? I'll pull the branch and look a bit more..

Yep. That's it. The Renderer upgrades the widget options and now that you've pulled the defaultOptions from the editors, they're actually being applied during the upgrade. Previously, we didn't register the widget editors at all for these tests, so minor version upgrades never changed anything! I think your change is correct (or at least restores the original intention of this upgrade code).

handeyeco · 2025-01-28T21:47:17Z

packages/perseus-score/src/widgets/widget-registry.ts

+registerWidget("deprecated-standin", () => scoreNoop(1) as any);
+registerWidget("measurer", () => scoreNoop(1) as any);
+
+registerWidget("definition", scoreNoop as any);


We need to get rid of scoreNoop but I didn't feel like now was the right moment.

… logic to PerseusScore

github-actions · 2025-01-28T22:36:21Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (852109c) and published it to npm. You
can install it using the tag PR2165.

Example:

yarn add @khanacademy/perseus@PR2165

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR2165

handeyeco · 2025-01-29T15:48:46Z

packages/perseus-score/src/score.test.ts

+    PerseusWidgetsMap,
+} from "@khanacademy/perseus-core";
+
+describe("flattenScores", () => {


Most of this is copied as-is except for the removal of strings and the refactor I pointed out.

handeyeco · 2025-01-29T15:49:59Z

packages/perseus-score/src/score.ts

@@ -0,0 +1,186 @@
+// TODO: combine scorePerseusItem with scoreWidgetsFunctional
+
+import {


This logic is mostly copied as-is except the removal of strings

handeyeco · 2025-01-29T15:52:13Z

packages/perseus-score/src/widgets/expression/score-expression.test.ts


 describe("scoreExpression", () => {
-    it("should be correctly answerable if validation passes", function () {


Removed tests related to validation from scoring tests

handeyeco · 2025-01-29T15:54:52Z

packages/perseus/src/util/scoring.test.ts

@@ -16,118 +16,3 @@ describe("isCorrect", () => {
        expect(isCorrect(score)).toBe(false);
    });
 });
-
-describe("flattenScores", () => {


handeyeco · 2025-01-29T15:55:08Z

packages/perseus/src/util/scoring.ts

-    );
-}
-
-export function flattenScores(widgetScoreMap: {


handeyeco · 2025-01-29T15:55:38Z

packages/perseus/src/widgets.ts

- * Supported alignments are given as an array of strings in the exports of
- * a widget's module.
- */
-export const getSupportedAlignments = (


jeremywiebe

Yahoo! I made it through. Some assorted comments for you, but this looks good to me.

jeremywiebe · 2025-01-29T16:43:43Z

packages/perseus-core/src/index.ts

 export type * from "./widgets/logic-export.types";

+export * as WidgetLogic from "./widgets/widget-registry";


The naming difference here feels like perhaps this should be exported as WidgetRegistry. :)

I renamed the export and file to CoreWidgetRegistry/core-widget-registry

jeremywiebe · 2025-01-30T17:26:10Z

packages/perseus-core/src/widgets/upgrade.ts

+                throw new PerseusError(
+                    "No upgrade found for widget. Cannot render.",
+                    Errors.Internal,
+                    {
+                        metadata: {
+                            type,
+                            fromMajorVersion: nextVersion - 1,
+                            toMajorVersion: nextVersion,
+                        },
+                    },
+                );


I think it's reasonable to throw here. Perhaps we could include information from the widget options that might help us trace which content is causing this? Maybe just add JSON.serialize(oldWidgetInfo) to metadata here?

jeremywiebe · 2025-01-30T17:27:36Z

packages/perseus-core/src/widgets/upgrade.ts

+    return _.extend({}, oldWidgetInfo, {
+        // maintain other info, like type
+        // After upgrading we guarantee that the version is up-to-date
+        version: latestVersion,
+        // Default graded to true (so null/undefined becomes true):
+        graded: oldWidgetInfo.graded != null ? oldWidgetInfo.graded : true,
+        alignment: alignment,
+        static: widgetStatic,
+        options: newEditorOptions,
+    });


Unrelated to the changes you're making, but this feels like a really easy improvement:

Suggested change

return _.extend({}, oldWidgetInfo, {

// maintain other info, like type

// After upgrading we guarantee that the version is up-to-date

version: latestVersion,

// Default graded to true (so null/undefined becomes true):

graded: oldWidgetInfo.graded != null ? oldWidgetInfo.graded : true,

alignment: alignment,

static: widgetStatic,

options: newEditorOptions,

});

return {

...oldWidgetInfo,

// maintain other info, like type

// After upgrading we guarantee that the version is up-to-date

version: latestVersion,

// Default graded to true (so null/undefined becomes true):

graded: oldWidgetInfo.graded != null ? oldWidgetInfo.graded : true,

alignment: alignment,

static: widgetStatic,

options: newEditorOptions,

};

Done, but I ran into a hairy type issue so I had to cast as any

jeremywiebe · 2025-01-30T17:29:22Z

packages/perseus-core/src/widgets/upgrade.ts

+                // TODO: why does widget have no type?
+                // We don't want to derive type from widget ID
+                // see: LEMS-1845
+                newValues.type = widgetId.split(" ")[0];


I'd be tempted to start throwing in the face of a missing widget type. Right now, we limp along and gloss over this data this is quite broken (and I suspect oooooold).

I can make this change, but I wasn't sure if you were asking me to or not. I personally would like to avoid making this PR more risky than it already is, but I don't feel strongly about it either way. I agree that it seems low risk to throw here.

jeremywiebe · 2025-01-30T17:30:38Z

packages/perseus-core/src/widgets/upgrade.ts

+    // select box. If the widget only supports one alignment, the
+    // alignment value will likely just end up as "default".
+    if (alignment == null || alignment === "default") {
+        alignment = getSupportedAlignments(type)[0];


Cool! The array access makes me a bit paranoid. Could we add a safety mechanism in getSupportedAlignments to throw if there are no supported alignments? Fully aware that this is extremely paranoid and you might just want to land this as is. :)

jeremywiebe · 2025-01-30T17:48:21Z

packages/perseus/src/widgets/group/group.test.tsx

-                  "countChoices": undefined,
-                  "deselectEnabled": undefined,
+                  "countChoices": false,
+                  "deselectEnabled": false,


Does this test end up upgrading the widget options ... and in doing so now seat some defaults that it didn't before? I'll pull the branch and look a bit more..

jeremywiebe · 2025-01-30T18:29:11Z

packages/perseus/src/widgets/group/group.test.tsx

-                  "countChoices": undefined,
-                  "deselectEnabled": undefined,
+                  "countChoices": false,
+                  "deselectEnabled": false,


Yep. That's it. The Renderer upgrades the widget options and now that you've pulled the defaultOptions from the editors, they're actually being applied during the upgrade. Previously, we didn't register the widget editors at all for these tests, so minor version upgrades never changed anything! I think your change is correct (or at least restores the original intention of this upgrade code).

jeremywiebe · 2025-01-30T18:31:25Z

packages/perseus/src/widgets/explanation/explanation.test.ts

-        it("should always return 0 points", async () => {
-            const result = ExplanationWidgetExports?.scorer?.();
-
-            expect(result).toHaveBeenAnsweredCorrectly({


Agreed. Doesn't actually seem like a useful test.

jeremywiebe · 2025-01-30T18:33:52Z

packages/perseus/src/types.ts

@@ -569,9 +538,6 @@ export type WidgetExports<
     * This key defaults to `{major: 0, minor: 0}` if not provided.
     */
    version?: Version;
-    supportedAlignments?: ReadonlyArray<Alignment>;
-    defaultAlignment?: Alignment;
-    getDefaultAlignment?: () => Alignment;


You're right! There's a shared getDefaultAlignment that's used, but nothing calls this function on individual widgets.

jeremywiebe · 2025-01-30T18:37:05Z

packages/perseus-score/src/widgets/mock-widget/mock-widget-validation.types.ts

+    value: string;
+};
+
+export type PerseusMockWidgetUserInput = {


I'm tempted to pull it into its own package (it could be a private package that's used anywhere in our Perseus repo, but it'd be structured as a real package to help guide us). I don't think this is a rush but might be something to try to see how it affects other organization (and move us to better support external widgets).

handeyeco added 13 commits January 28, 2025 10:16

[LEMS-2737/move-scoring-logic] WIP: start the move

8aa4028

[LEMS-2737/move-scoring-logic] WIP: scoring registry

b25fb6c

[LEMS-2737/move-scoring-logic] WIP: handle noop scorer

d2387fc

[LEMS-2737/move-scoring-logic] WIP: move group scorer

3523c8f

[LEMS-2737/move-scoring-logic] WIP: move alignments

8fc194e

[LEMS-2737/move-scoring-logic] WIP: Perseus Core registry

15c03e5

[LEMS-2737/move-scoring-logic] WIP: getSupportedAlignments

39efebe

[LEMS-2737/move-scoring-logic] WIP: add registry helpers

0934f34

[LEMS-2737/move-scoring-logic] WIP: mock widget

f764d86

[LEMS-2737/move-scoring-logic] WIP: lint/tsc happy, tests sad

5348013

[LEMS-2737/move-scoring-logic] WIP: fix some tests

14450db

[LEMS-2737/move-scoring-logic] fix tests

38ea7b6

[LEMS-2737/move-scoring-logic] convert Log to Error

de0e4ae

handeyeco self-assigned this Jan 28, 2025

handeyeco changed the title ~~[LEMS-2737/move-scoring-logic] WIP: lint/tsc happy, tests sad~~ Move main Scorer and Validator to PerseusScore Jan 28, 2025

[LEMS-2737/move-scoring-logic] remove validation checks in scorers

e894520

handeyeco commented Jan 28, 2025

View reviewed changes

handeyeco added 2 commits January 28, 2025 15:58

[LEMS-2737/move-scoring-logic] fix test util file placement

7d73db2

[LEMS-2737/move-scoring-logic] docs(changeset): Move scorePerseusItem…

f231ad4

… logic to PerseusScore

handeyeco marked this pull request as ready for review January 28, 2025 22:46

handeyeco requested review from a team January 28, 2025 22:46

handeyeco commented Jan 29, 2025

View reviewed changes

handeyeco added 3 commits January 29, 2025 09:56

[LEMS-2737/move-scoring-logic] more cleanup

4216e8a

[LEMS-2737/move-scoring-logic] fix conflicts

d7f2607

[LEMS-2737/move-scoring-logic] add grapher to core registry

83080a4

jeremywiebe approved these changes Jan 30, 2025

View reviewed changes

[LEMS-2737/move-scoring-logic] rename WidgetLogic to CoreWidgetRegistry

34055a0

handeyeco added 5 commits January 30, 2025 13:38

[LEMS-2737/move-scoring-logic] add oldWidgetInfo to error metadata

19a5d84

[LEMS-2737/move-scoring-logic] secure getSupportedAlignments a little

d9a6699

[LEMS-2737/move-scoring-logic] remove underscore extends

488f663

[LEMS-2737/move-scoring-logic] cleanup getSupportedAlignments more

a916f59

[LEMS-2737/move-scoring-logic] cleanup getDefaultAlignment

852109c

		@@ -0,0 +1,186 @@
		// TODO: combine scorePerseusItem with scoreWidgetsFunctional

		import {


		describe("scoreExpression", () => {
		it("should be correctly answerable if validation passes", function () {

		export type * from "./widgets/logic-export.types";

		export * as WidgetLogic from "./widgets/widget-registry";

Move main Scorer and Validator to PerseusScore #2165

Are you sure you want to change the base?

Move main Scorer and Validator to PerseusScore #2165

Conversation

handeyeco commented Jan 28, 2025 • edited Loading

Summary:

Test plan:

github-actions bot commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2025 • edited Loading

npm Snapshot: Published

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handeyeco commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited

Loading