Skip to content
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

Teacher tool: first pass of nested validators #9862

Merged
merged 11 commits into from
Feb 14, 2024
Merged
12 changes: 12 additions & 0 deletions common-docs/teachertool/catalog-shared.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@
"use": "variable_accessed",
"template": "At least one variable is accessed",
"docPath": "/teachertool"
},
{
"id": "8E6F9A92-2D22-4BB0-A77A-BD7490D3CEF7",
"use": "variable_set_random",
"template": "A custom variable's value is set to a random number",
"docPath": "/teachertool"
},
{
"id": "E8DAD360-F4AB-4E6B-9981-C14BDEE1295B",
"use": "device_random_used",
"template": "The math random number generator block is used",
"docPath": "/teachertool"
}
]
}
27 changes: 27 additions & 0 deletions common-docs/teachertool/validator-plans-shared.json
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,33 @@
}
}
]
},
{
".desc": "A custom variable's value is set to a random number",
"name": "variable_set_random",
"threshold": 1,
"checks": [
{
"validator": "blocksExist",
"blockCounts": {
"variables_set": 1
},
"childValidatorPlans": ["device_random_used"]
}
]
},
{
".desc": "Random number block used in project",
"name": "device_random_used",
"threshold": 1,
"checks": [
{
"validator": "blocksExist",
"blockCounts": {
"device_random": 1
}
}
]
}
]
}
1 change: 1 addition & 0 deletions localtypings/pxteditor.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ declare namespace pxt.editor {
export interface EditorMessageRunEvalRequest extends EditorMessageRequest {
action: "runeval";
validatorPlan: pxt.blocks.ValidatorPlan;
planBank: pxt.blocks.ValidatorPlan[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small naming suggestion for clarity.

Suggested change
planBank: pxt.blocks.ValidatorPlan[];
planLib: pxt.blocks.ValidatorPlan[];

}

export interface EditorMessageRenderBlocksResponse {
Expand Down
1 change: 1 addition & 0 deletions localtypings/validatorPlan.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ declare namespace pxt.blocks {
// Each type of validation will need to implement its own ValidatorCheck based on this.
export interface ValidatorCheckBase {
validator: string;
childValidatorPlans?: string[]
}

// Inputs for "Blocks Exist" validation.
Expand Down
61 changes: 37 additions & 24 deletions pxteditor/code-validation/runValidatorPlanAsync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,48 @@ import { validateBlocksInSetExist } from "./validateBlocksInSetExist";
import { validateBlockCommentsExist } from "./validateCommentsExist";
import { validateSpecificBlockCommentsExist } from "./validateSpecificBlockCommentsExist";

const maxConcurrentChecks = 4;

export async function runValidatorPlanAsync(usedBlocks: Blockly.Block[], plan: pxt.blocks.ValidatorPlan): Promise<boolean> {
// Each plan can have multiple checks it needs to run.
// Run all of them in parallel, and then check if the number of successes is greater than the specified threshold.
// TBD if it's faster to run in parallel without short-circuiting once the threshold is reached, or if it's faster to run sequentially and short-circuit.
export function runValidatorPlanAsync(usedBlocks: Blockly.Block[], plan: pxt.blocks.ValidatorPlan, planBank: pxt.blocks.ValidatorPlan[]): boolean {
const startTime = Date.now();
let checksSucceeded = 0;
let successfulBlocks: Blockly.Block[] = [];

const checkRuns = pxt.Util.promisePoolAsync(maxConcurrentChecks, plan.checks, async (check: pxt.blocks.ValidatorCheckBase): Promise<boolean> => {
for (const check of plan.checks) {
let checkPassed = false;
switch (check.validator) {
case "blocksExist":
return runBlocksExistValidation(usedBlocks, check as pxt.blocks.BlocksExistValidatorCheck);
[successfulBlocks, checkPassed] = [...runBlocksExistValidation(usedBlocks, check as pxt.blocks.BlocksExistValidatorCheck)];
break;
case "blockCommentsExist":
return runValidateBlockCommentsExist(usedBlocks, check as pxt.blocks.BlockCommentsExistValidatorCheck);
checkPassed = runValidateBlockCommentsExist(usedBlocks, check as pxt.blocks.BlockCommentsExistValidatorCheck);
break;
case "specificBlockCommentsExist":
return runValidateSpecificBlockCommentsExist(usedBlocks, check as pxt.blocks.SpecificBlockCommentsExistValidatorCheck);
checkPassed = runValidateSpecificBlockCommentsExist(usedBlocks, check as pxt.blocks.SpecificBlockCommentsExistValidatorCheck);
break;
case "blocksInSetExist":
return runBlocksInSetExistValidation(usedBlocks, check as pxt.blocks.BlocksInSetExistValidatorCheck);
[successfulBlocks, checkPassed] = [...runBlocksInSetExistValidation(usedBlocks, check as pxt.blocks.BlocksInSetExistValidatorCheck)];
break;
default:
pxt.debug(`Unrecognized validator: ${check.validator}`);
return false;
checkPassed = false;
break;
}
});

const results = await checkRuns;
const successCount = results.filter((r) => r).length;
const passed = successCount >= plan.threshold;
if (checkPassed && check.childValidatorPlans) {
for (const planName of check.childValidatorPlans) {
let timesPassed = 0;
for (const parentBlock of successfulBlocks) {
const blocksToUse = parentBlock.getChildren(true);
const childPlan = planBank.find((plan) => plan.name === planName);
const childPassed = runValidatorPlanAsync(blocksToUse, childPlan, planBank);
timesPassed += childPassed ? 1 : 0;
}
checkPassed = timesPassed > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug here. In the course of iterating over child validator plans, checkPassed can become false then true again with no consequences. Only the last-assigned value of checkPassed is used when computing checksSucceeded below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good catch. I think "anding" this will do the trick..

If we have any of the children validators fail, then the whole validator plan should fail, so, if at any moment, checkPassed is false, then that false value should be maintained.

}
}
checksSucceeded += checkPassed ? 1 : 0;
}

const passed = checksSucceeded >= plan.threshold;

pxt.tickEvent("validation.evaluation_complete", {
plan: plan.name,
Expand All @@ -42,13 +57,11 @@ export async function runValidatorPlanAsync(usedBlocks: Blockly.Block[], plan: p
return passed;
}

function runBlocksExistValidation(usedBlocks: Blockly.Block[], inputs: pxt.blocks.BlocksExistValidatorCheck): boolean {
function runBlocksExistValidation(usedBlocks: Blockly.Block[], inputs: pxt.blocks.BlocksExistValidatorCheck): [Blockly.Block[], boolean] {
const blockResults = validateBlocksExist({ usedBlocks, requiredBlockCounts: inputs.blockCounts });
const success =
blockResults.disabledBlocks.length === 0 &&
blockResults.missingBlocks.length === 0 &&
blockResults.insufficientBlocks.length === 0;
return success;
const blockId = Object.keys(inputs.blockCounts)[0];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has several array accesses without range checks. Are these arrays guaranteed to be non-empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For blockId in particular, if Object.keys(inputs.blockCounts) returned nothing, then the validation would be essentially pointless. With this array access, I'm getting the block that we're checking exists for that validator run.

So, for example, if you have this validator:

    {
        ".desc": "A custom variable's value is set to a random number",
        "name": "variable_set_random",
        "threshold": 1,
        "checks": [
            {
                "validator": "blocksExist",
                "blockCounts": {
                    "variables_set": 1
                },
                "childValidatorPlans": ["device_random_used"]
            }
        ]
    }

The block that we're actually wanting to check for is variables_set, and you can only know this in this function by looking in the JSON. For tutorial validation, you can have multiple different blocks in the blockCounts object, but for our validation, we should only ever have one entry in the blockCounts object. Either way, if Object.keys(inputs.blockCounts) was empty at the point that we do an array access in this function, validation would be essentially pointless. However, you're right in that I should check whether the array returned from Object.keys() has length before accessing.

const successfulBlocks = blockResults.successfulBlocks.length ? blockResults.successfulBlocks[0][blockId] : [];
return [successfulBlocks, blockResults.passed];
}

function runValidateBlockCommentsExist(usedBlocks: Blockly.Block[], inputs: pxt.blocks.BlockCommentsExistValidatorCheck): boolean {
Expand All @@ -61,7 +74,7 @@ function runValidateSpecificBlockCommentsExist(usedBlocks: Blockly.Block[], inpu
return blockResults.passed;
}

function runBlocksInSetExistValidation(usedBlocks: Blockly.Block[], inputs: pxt.blocks.BlocksInSetExistValidatorCheck): boolean {
function runBlocksInSetExistValidation(usedBlocks: Blockly.Block[], inputs: pxt.blocks.BlocksInSetExistValidatorCheck): [Blockly.Block[], boolean] {
const blockResults = validateBlocksInSetExist({ usedBlocks, blockIdsToCheck: inputs.blocks, count: inputs.count });
return blockResults.passed;
return [blockResults.successfulBlocks, blockResults.passed];
}
15 changes: 15 additions & 0 deletions pxteditor/code-validation/validateBlocksExist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,25 @@ export function validateBlocksExist({ usedBlocks, requiredBlockCounts }: {
missingBlocks: string[],
disabledBlocks: string[],
insufficientBlocks: string[],
successfulBlocks: pxt.Map<Blockly.Block[]>[],
passed: boolean
} {
let missingBlocks: string[] = [];
let disabledBlocks: string[] = [];
let insufficientBlocks: string[] = [];
let successfulBlocks: pxt.Map<Blockly.Block[]>[] = [];
const userBlocksEnabledByType = usedBlocks?.reduce((acc: pxt.Map<number>, block) => {
acc[block.type] = (acc[block.type] || 0) + (block.isEnabled() ? 1 : 0);
return acc;
}, {});

for (const [requiredBlockId, requiredCount] of Object.entries(requiredBlockCounts || {})) {
const countForBlock = userBlocksEnabledByType[requiredBlockId];
const passedBlocks = usedBlocks.filter((block) => block.type === requiredBlockId);
if (passedBlocks.length > 0) {
successfulBlocks.push({ [requiredBlockId]: passedBlocks})
}

if (countForBlock === undefined) {
// user did not use a specific block
missingBlocks.push(requiredBlockId);
Expand All @@ -29,9 +37,16 @@ export function validateBlocksExist({ usedBlocks, requiredBlockCounts }: {
}
}

const passed =
missingBlocks.length === 0 &&
disabledBlocks.length === 0 &&
insufficientBlocks.length === 0;

return {
missingBlocks,
disabledBlocks,
insufficientBlocks,
successfulBlocks,
passed
}
}
3 changes: 2 additions & 1 deletion pxteditor/editorcontroller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,11 @@ export function bindEditorMessages(getEditorAsync: () => Promise<IProjectView>)
case "runeval": {
const evalmsg = data as pxt.editor.EditorMessageRunEvalRequest;
const plan = evalmsg.validatorPlan;
const planBank = evalmsg.planBank;
return Promise.resolve()
.then(() => {
const blocks = projectView.getBlocks();
return runValidatorPlanAsync(blocks, plan)})
return runValidatorPlanAsync(blocks, plan, planBank)})
.then (results => {
resp = { result: results };
});
Expand Down
4 changes: 3 additions & 1 deletion teachertool/src/services/makecodeEditorService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ export async function setHighContrastAsync(on: boolean) {
}

export async function runValidatorPlanAsync(
validatorPlan: pxt.blocks.ValidatorPlan
validatorPlan: pxt.blocks.ValidatorPlan,
planBank: pxt.blocks.ValidatorPlan[]
): Promise<pxt.blocks.EvaluationResult | undefined> {
let evalResults = undefined;

Expand All @@ -101,6 +102,7 @@ export async function runValidatorPlanAsync(
type: "pxteditor",
action: "runeval",
validatorPlan: validatorPlan,
planBank: planBank
} as pxt.editor.EditorMessageRunEvalRequest);
const result = response as pxt.editor.EditorMessageResponse;
validateResponse(result, true); // Throws on failure
Expand Down
9 changes: 8 additions & 1 deletion teachertool/src/transforms/runEvaluateAsync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,21 @@ export async function runEvaluateAsync() {
new Promise(async resolve => {
dispatch(Actions.setEvalResult(criteriaInstance.instanceId, CriteriaEvaluationResult.InProgress));

const loadedValidatorPlans = teacherTool.validatorPlans;
if (!loadedValidatorPlans) {
logError(ErrorCode.validatorPlansNotFound, "Attempting to evaluate criteria without any plans");
dispatch(Actions.clearEvalResult(criteriaInstance.instanceId));
return resolve(false);
}

const plan = generateValidatorPlan(criteriaInstance);

if (!plan) {
dispatch(Actions.clearEvalResult(criteriaInstance.instanceId));
return resolve(false);
}

const planResult = await runValidatorPlanAsync(plan);
const planResult = await runValidatorPlanAsync(plan, loadedValidatorPlans);

if (planResult) {
dispatch(
Expand Down
1 change: 1 addition & 0 deletions teachertool/src/types/errorCode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ export enum ErrorCode {
unableToReadRubricFile = "unableToReadRubricFile",
localStorageReadError = "localStorageReadError",
localStorageWriteError = "localStorageWriteError",
validatorPlansNotFound = "validatorPlansNotFound"
}