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

Lab2: add source validator for labs with JSON sources #58445

Merged
merged 3 commits into from May 8, 2024

Conversation

sanchitmalhotra126
Copy link
Contributor

Quick convenience fix; in local development, we're running into a lot of AI Chat levels that share channel IDs with old labs that have incompatible sources, and have to manually go into S3 to delete the old sources. For other labs (Blockly/Python lab), the response validators will let us fall back to empty sources, so this adds similar behavior for AI Chat and any other labs that use JSON sources; if the JSON cannot be parsed, we'll fall back to empty sources.

Testing story

Tested with AI chat levels in the gen-ai-foundations-draft script.

Copy link
Contributor

@bencodeorg bencodeorg left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

try {
JSON.parse(responseToValidate.source as string);
} catch (e) {
throw new ValidationError('Error parsing JSON: ' + e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you're following the existing pattern, but for my understanding, where does this get caught? I can't seem to trace it properly 🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries this code is a bit tricky to follow 🙂 this ValidationError is eventually caught by ProjectManager when it attempts to load sources for the project:

if (error instanceof ValidationError) {
this.metricsReporter.logWarning(
`Error validating sources (${error.message}). Defaulting to empty sources.`
);

It takes care of defaulting to empty sources and thereby not passing the invalid sources to labs.

Copy link
Contributor

Choose a reason for hiding this comment

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

There it is! Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. Where are sources assigned {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, they're just never assigned (they stay undefined from

let sources: ProjectSources | undefined;

Copy link
Contributor

@fisher-alice fisher-alice left a comment

Choose a reason for hiding this comment

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

🙏 Thank you for doing this - I was so happy when I saw this PR title pop up in notifications!

@@ -46,6 +46,21 @@ const PythonSourceResponseValidator: ResponseValidator<
return sourceValidatorHelper(response, pythonValidator);
};

// Validator for labs that use JSON sources
const JsonSourceResponseValidator: ResponseValidator<
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: technically, Blocky sources are also json sources I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, will update the comment to specify

try {
JSON.parse(responseToValidate.source as string);
} catch (e) {
throw new ValidationError('Error parsing JSON: ' + e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. Where are sources assigned {}?

Copy link
Collaborator

@levadadenys levadadenys left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this!

@sanchitmalhotra126 sanchitmalhotra126 merged commit 315a6c4 into staging May 8, 2024
2 checks passed
@sanchitmalhotra126 sanchitmalhotra126 deleted the sanchit/lab2-json-sources branch May 8, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants