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

[Python Lab] add support for matplotlib and small updates #57782

Merged
merged 5 commits into from
Apr 8, 2024

Conversation

molly-moen
Copy link
Contributor

@molly-moen molly-moen commented Apr 3, 2024

Warning!!

The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.

Summary

The work in this PR came out of investigating if we could get matplotlib working in Python Lab. matplotlib is a supported pyodide package, but there are some issues with getting it working in a web worker (see this issue for details). As a simple workaround we can patch the matplotlib show function to print a base64 string, then show said string in an image element. This may prove to be a little fragile; I filed a follow up task to productionize this.

This work required a couple things that caused me to do a few refactors/improvements more generally to Python Lab. These are:

  • update the console log storage in redux to have different log types, image, system out, system in (not used yet), and system message (things we send, such as start/end messages).
  • Clean up and update how we wrap the user's code with our custom patches. Previously this was all done in pyodideWorkerManager. Now I have moved the existing flush system out patch (which is appended to all student code) and the new matplotlib patch into a separate file, and created a utils file to handle wrapping the user's code.
  • Always load pyodide on page load. Now that we are patching matplotlib, we would be importing numpy and matplotlib on first run if we continued to lazy load. matplotlib takes a few seconds to load, so starting that process early makes the first run faster. We can do this now because we only load the component if we are on a Python Lab level, as opposed to previously when it was loaded for all lab2 levels.

This is what it looks like to run a program that outputs two graphs
Screenshot 2024-04-03 at 2 34 41 PM

Links

Testing story

Tested locally. Python Lab is isolated code and not in use, so this is low risk.

Follow-up work

  • CT-475 is a placeholder for productionizing this work.

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@molly-moen molly-moen requested a review from a team April 3, 2024 22:12
@@ -102,7 +102,22 @@ const PythonEditor: React.FunctionComponent = () => {
<div>
Output:
{codeOutput.map((outputLine, index) => {
return <div key={index}>{outputLine}</div>;
if (outputLine.type === 'img') {
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking nit: Should we eventually move these types to a constants file somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to get refactored completely so I think for now I'll leave as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but yes, they should eventually be constants :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, I figured! As I typed it I knew I was just making the "obligatory seeing plaintext in a condition" comment 😂 Sounds good to me!

return (
<img
key={index}
src={`data:image/png;base64,${outputLine.contents}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice solution!

os.fsync(sys.stdout.fileno())
`;

// TODO: Should we always patch matplotlib? Or can we be smarter about when to patch it?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some comment about this included in the task to productionize the patch? If not, can we add a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a link to the ticket here!

Copy link
Contributor

@ebeastlake ebeastlake left a comment

Choose a reason for hiding this comment

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

Nice work! 🎉

@molly-moen molly-moen mentioned this pull request Apr 8, 2024
8 tasks
@molly-moen molly-moen merged commit 0fcdaa8 into staging Apr 8, 2024
2 checks passed
@molly-moen molly-moen deleted the molly/python-lab-visualizations branch April 8, 2024 23:46
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.

2 participants