Upgrade workers #112

winniederidder · 2022-03-30T09:57:47Z

This PR upgrades the workers to make use of the recently published packages Comsync and pyodide-worker-runner.
It also adds useful refactoring parts from #109 that are not strictly tied into debugging but made integrating it more feasible.
This should increase the general flexibility.

Support was added for the following:
sleeping
interrupting with restarting
clicking run before the load is finished (is awaited during run instead if needed)
generating a tar with dependencies to pre-load, allowing internal Python code to use proper packages instead of a raw string
Running code is refactored to be done within a separate component contained within the Papyros object

Closes #66
Closes #38
Closes #86

src/CodeRunner.ts

src/BackendEvent.ts

src/workers/python/PythonWorker.worker.ts

src/util/Util.ts

src/workers/python/init.py

alexmojaki · 2022-03-31T20:46:35Z

src/workers/python/init.py

            elif event_type == "input":
                return cb("input", data["prompt"], contentType="text/plain")
+            elif event_type == "sleep":
+                return cb("sleep", str(data["seconds"]*1000), contentType="text/integer")


I suggest using makeRunnerCallback from pyodide-worker-runner and moving this event translation logic to JS.

I'm still unsure what the best approach is. The makeRunnerCallback method expects an array of callbacks, which becomes quite unwieldy. I moved to a PubSub system to prevent needing to pass many callbacks. I would think that adding a catch-all to makeRunnerCallback would make it a bit more user-friendly for this specific case, but I wouldn't call this part of my code stable yet.

You should only need an object with two callbacks for the two event types input and output. The output part types aren't looked at.

Here, I meant that makeRunnerCallback expects an object with at least 2 callbacks and then extra callbacks per possible type. I have an error callback, the interrupt just in case, and potentially a debugging callback which makes this unwieldy to pass around. If more types get added, this also poses issues for makeRunnerCallback. The translation in Python is not exactly ideal either, so I'll think about a hybrid approach to clean this up.

It sounds like it would be better if makeRunnerCallback didn't try to be so helpful with other event types and just let you provide a function to act as a default. That way you could just let other events pass through to your own system.

A catch-all would indeed be more practical in my opinion.

src/workers/python/init.py

alexmojaki · 2022-04-06T14:42:51Z

src/CodeRunner.ts

+        this.setState(RunState.Stopping);
+        BackendManager.publish({
+            type: BackendEventType.End,
+            data: "User cancelled run", contentType: "text/plain"
+        });
+        await this.backend.then(b => b.interrupt());


This is still assuming that nothing will catch the KeyboardInterrupt. You can only do this if call throws InterruptError. Treat client.interrupt as a suggestion/request.

The goal is to fully interrupt. Ideally, some method should exist in synClient to force-interrupt (maybe a configurable timeout that terminates the worker if the interrupt was not successful?) Having to interpret client.interrupt as a suggestion is counter-intuitive to me.

alexmojaki · 2022-04-06T14:43:57Z

src/CodeRunner.ts

+     */
+    async setProgrammingLanguage(programmingLanguage: ProgrammingLanguage): Promise<void> {
+        if (this.programmingLanguage !== programmingLanguage) { // Expensive, so ensure it is needed
+            await this.backend.then(b => b.interrupt());


If you want to be sure here then use client.terminate(), but then the client must be deleted and not reused.

alexmojaki · 2022-04-06T14:50:18Z

src/CodeRunner.ts

+            const end = new Date().getTime();
+            this.setState(RunState.Ready, t("Papyros.finished", { time: (end - start) / 1000 }));
+            if (terminated) {
+                await this.start();


comsync already restarts the worker after terminating. Also, you can end up here if you interrupt/terminate the client for the sake of switching programming language.

Restarting the worker is not enough, as the launch method must also be called. This is done in start, so that should be fine. For the second case, I don't think anything will go wrong if start is called twice.

alexmojaki · 2022-04-06T14:56:28Z

src/workers/javascript/JavaScriptWorker.worker.ts

@@ -121,7 +120,8 @@ class JavaScriptWorker extends Backend {
        return ret;
    }

-    override _runCodeInternal(code: string): Promise<any> {
+    override runCode(extras: SyncExtras, code: string): Promise<any> {
+        this.extras = extras;


I assume that input is still supported in JS. What about sleeping?

Sleeping doesn't really seem to be a thing in JS. What exists is setTimeout, which is non-blocking. In order to properly support that, I think I would have to store the timers internally and await them before giving control back to the user. Our focus is currently on Python, so I don't plan on changing this yet.

What exists is setTimeout, which is non-blocking. In order to properly support that, I think I would have to store the timers internally and await them before giving control back to the user.

What exactly are you saying? Using setTimeout as usual doesn't work properly? Or do you mean making setTimeout blocking?

You could add a custom sleep function using extras.syncSleep(), which currently just means running the callback with the sleep event.

What I meant is that a blocking sleep call does not exist in JavaScript by default. It can be emulated with Promises and awaiting them, but I haven't played around a lot with async/await in the JavaScript runner.
setTimeout is non-blocking and works, but these calls are not "awaited", so if you write eg

setTimeout(() => console.log("Timer"), 5000)

then the run will be finished immediately and the log statement will appear later in the actual console instead of the output field. This is because of the global overrides being done at the start and being cleared at the end of a run. That's why some awaiting mechanism should be added for these timeouts.

src/Backend.ts

alexmojaki · 2022-04-06T15:11:10Z

src/workers/python/papyros/papyros.py

+                    if typ in ["stderr", "traceback", "syntax_error"]:
+                        cb("error", part["text"], contentType=part.get("contentType"))
+                    elif typ in ["input", "input_prompt"]:
+                        # Do not display values entered by user for input
+                        continue
+                    else:
+                        cb("output", part["text"], contentType=part.get("contentType"))


I think you should embrace the python_runner output part types approach. You're sending stderr and stdout to separate event types and code paths, but they're almost exactly the same thing, just different colours. Meanwhile you're combining stderr and tracebacks which get handled completely differently. Ultimately these are all just different types of output which all get rendered to the same output area, even if it's in different ways.

These things are a bit too tied to Python, so they're not easily generalised to other languages (e.g. due to presence of snake_case). With the eventing system, I think it's nice to be able to only listen to error messages instead of getting all output. Finally, I don't think our target group will often write to stderr, so the focus is on the tracebacks as they will be the main reason for error displaying. The current translations seem fine to me.

src/workers/python/papyros/util.py

winniederidder added 16 commits March 30, 2022 10:58

Refactor python part for flexibility

07900ef

Rename BackendEvent and refactor workers

ba46118

Implement pubsub system for events

b7082a4

Rename and refactor running code

d5f34ea

Cleanup to initializing input handlers

59094a2

Reduce constructor params

4ee6906

Fix typo

2fa34d8

Refactor to use coderunner

83752a9

Add last parts of refactor

46f6476

Move channel to BackendManager

beda633

New packages

eae89ce

Send input in the right way

ceff1e3

Fix configure input

de96039

Use SyncClient

a5e824e

Use syncExpose

6c08493

Use pyodideExpose

29ea11f

alexmojaki reviewed Mar 31, 2022

View reviewed changes

src/CodeRunner.ts Outdated Show resolved Hide resolved

alexmojaki reviewed Mar 31, 2022

View reviewed changes

src/BackendEvent.ts Outdated Show resolved Hide resolved

alexmojaki reviewed Mar 31, 2022

View reviewed changes

src/workers/python/PythonWorker.worker.ts Outdated Show resolved Hide resolved

winniederidder added 5 commits March 31, 2022 17:21

Add generic methods to easily expose methods for SyncClient

9b7e688

Docs for new methods

c0d380c

Delay autocompletion until loaded

5b22cf0

Add support for sleep

f523551

First version of proper interrupts

b6996ae

alexmojaki reviewed Mar 31, 2022

View reviewed changes

src/util/Util.ts Show resolved Hide resolved

alexmojaki reviewed Mar 31, 2022

View reviewed changes

src/workers/python/init.py Outdated Show resolved Hide resolved

alexmojaki reviewed Mar 31, 2022

View reviewed changes

src/workers/python/init.py Outdated Show resolved Hide resolved

alexmojaki reviewed Mar 31, 2022

View reviewed changes

src/workers/python/init.py Outdated Show resolved Hide resolved

winniederidder added the deploy This PR should be deployed. label Mar 31, 2022

winniederidder had a problem deploying to production April 5, 2022 20:40 Failure

Update build script to make tar accessible

d6e84e1

winniederidder added deploy This PR should be deployed. and removed deploy This PR should be deployed. labels Apr 5, 2022

winniederidder had a problem deploying to production April 5, 2022 20:47 Failure

Add tar to github

f172a4e

winniederidder added deploy This PR should be deployed. and removed deploy This PR should be deployed. labels Apr 5, 2022

winniederidder temporarily deployed to production April 5, 2022 20:55 Inactive

github-actions bot removed the deploy This PR should be deployed. label Apr 5, 2022

alexmojaki reviewed Apr 6, 2022

View reviewed changes

src/Backend.ts Outdated Show resolved Hide resolved

alexmojaki reviewed Apr 6, 2022

View reviewed changes

src/workers/python/papyros/util.py Outdated Show resolved Hide resolved

winniederidder added 3 commits April 7, 2022 14:17

Fix to_py and exposing

9bd3032

Fix build

6b82b40

Better resizing

b272fc8

winniederidder changed the base branch from issue40/code-completion to main April 14, 2022 08:42

bmesuere approved these changes Apr 14, 2022

View reviewed changes

Merge branch 'main' into upgrade-workers

cc563df

winniederidder added the deploy This PR should be deployed. label Apr 14, 2022

winniederidder temporarily deployed to production April 14, 2022 09:03 Inactive

github-actions bot removed the deploy This PR should be deployed. label Apr 14, 2022

winniederidder merged commit c35f8f1 into main Apr 14, 2022

winniederidder deleted the upgrade-workers branch April 14, 2022 09:16

winniederidder mentioned this pull request Apr 14, 2022

Enable run button faster after switching language #28

Closed

alexmojaki mentioned this pull request Apr 30, 2022

Pass all 'other' event types to single function in makeRunnerCallback alexmojaki/pyodide-worker-runner#10

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade workers #112

Upgrade workers #112

winniederidder commented Mar 30, 2022 •

edited

Loading

alexmojaki Mar 31, 2022

winniederidder Mar 31, 2022

alexmojaki Mar 31, 2022

winniederidder Apr 2, 2022

alexmojaki Apr 6, 2022

winniederidder Apr 6, 2022

alexmojaki Apr 6, 2022

winniederidder Apr 7, 2022

alexmojaki Apr 6, 2022

alexmojaki Apr 6, 2022

winniederidder Apr 7, 2022

alexmojaki Apr 6, 2022

winniederidder Apr 6, 2022

alexmojaki Apr 6, 2022

winniederidder Apr 7, 2022

alexmojaki Apr 6, 2022

winniederidder Apr 7, 2022

Upgrade workers #112

Upgrade workers #112

Conversation

winniederidder commented Mar 30, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winniederidder commented Mar 30, 2022 •

edited

Loading