Refactor how py-script are executed, kill scriptQueue store, introduce pyExec by antocuni · Pull Request #881 · pyscript/pyscript

antocuni · 2022-10-22T19:01:44Z

Yet another refactoring to untangle the old mess.
Highlights:

base.ts, pyscript.ts and pyrepl.ts were a tangled mess of code, in which each of them interacted with the others in non-obvious ways. Now PyScript is no longer a subclass of BaseEvalElement and it is much simpler. I removed code for handling the attributes std-out and std-err because they are no longer needed with the new display() logic.

The logic for executing python code is now in pyexec.ts: so we are decoupling the process of "finding" the python code (handled by the py-script web component) and the logic to actually execute it. This has many advantages, including the fact that it will be more easily usable by other components (e.g. pyrepl). Also, note that it's called pyexec and not pyeval: in the vast majority of cases in Python you have statements to execute, and almost never expressions to evaluate.

I killed the last remaining global store, scriptQueue 🎉. As a bonus effect, now we automatically do the correct thing when a <py-script> tag is dynamically added to the DOM (I added a test for it). I did not remove svelte from packages.json, because I don't fully understand the implications: there are various options which mention svelte in rollup.js and tsconfig.json, so it's probably better to kill it in its own PR.

pyexec.ts is also responsible of handling the default target for display() and correct handling/visualization of exceptions. I fixed/improved/added display/output tests in the process.
I also found a problem though, see issue #878, so I improved the test and marked it as xfail.

I removed BaseEvalElement as the superclass of most components. Now the only class which inherits from it is PyRepl. In a follow-up PR, I plan to merge them into a single class and do more cleanup.

During the refactoring, I killed guidGenerator: now instead of generating random py-* IDs which are very hard to read for humans, we generated py-internal-X IDs, where X is 0, 1, 2, 3, etc. This makes writing tests and debugging much easier.

I improved a lot our test machinery: it turns out that PR #829 broke the ability to use/view sourcemaps inside the playwright browser (at least on my machine).
For some reason chromium is unable to find sourcemaps if you use playwrights internal routing. So I reintroduced the http_server fixture which was removed by that PR, and added a pytest option --no-fake-server to use it instead, useful for debugging. By default we are still using the fakeserver though (which is faster and parallelizable).

Similarly, I added --dev which implies --headed and also automatically open chrome dev tools.

…comment which explains some of the problems of the current approach. But I don't want to tackled it in this PR

When using this option, you use a real http server instead of using playwright's internal routing. There are cases in which it's useful, in particular: 1) if you want to test the page on different browser 2) if you want to use the chrome dev tools, because apparently when using the fake server chromium is unable to locate the sourcemaps This commit reintroduces some of the code which was killed by PR #829.

…ly open the devtools panel

…easier to test and to use in the debugger for humans

…eEvalElement. This contains a bit of mechnical copy&paste but I will simplify the code in the next commits

…at we can execute a very simple python code, no need to check the behavior of display()

… using a queue of scripts, we immediately run the python code inside connectedCallback. This has also the advantage that if we dynamically add a <py-script> tag to the DOM, the python code is automatically executed, as the new test demonstrates (the test would have been failed before this commit)

…t>, and the relevant test

…queue

…level because you would need to mock the whole Runtime, and you would end up testing the mock instead of something actually useful. PyScript is battle-tested a lot by integration tests

…out multiple display. It is about implicit display() target, so move it it the correct class and improve the test to be less dependent on the exact <div> id

tedpatrick

Goodbye stores!
No issues.

JeffersGlass · 2022-10-23T19:38:28Z

-                this.errorElement = this.outputElement;
-            }
-        }
+export function make_PyScript(runtime: Runtime) {


Just curious - what's the advantage of using a function that defines the class rather than just the class definition?

the fact that I can pass runtime from the outside. Note that runtime is used from connectedCallback, and since this is a method called by the browser, we don't really have any other way to pass it explicitly.
The alternative was to use a global variable as it was before, but we saw that it caused way too many problems.

JeffersGlass · 2022-10-23T19:40:07Z

+}
+
+
+function displayPyException(err: any, errElem: HTMLElement) {


Love this as a start - having a unified way to intercept and handle errors will be nice, but really good to have this behavior in one place.

yes. The next step will be to have an unified place where to display errors when there is no other obvious place (e.g. if you raise an exception from an event handler).

…queue

marimeireles

Cool PR! :)

marimeireles · 2022-10-24T10:21:01Z

    config: AppConfig;
    loader: PyLoader;
    runtime: Runtime;
+    PyScript: any; // XXX would be nice to have a more precise type for the class itself


[question] Why do we have PyScript here instead of pyscript does this mean something different?

It stands for the PyScript object I guess... and the other ones are function names?

to, this is the PyScript class, which now is created at runtime:

pyscript/pyscriptjs/src/components/pyscript.ts

Lines 8 to 11 in f9194cc

export function make_PyScript(runtime: Runtime) {

class PyScript extends HTMLElement {

so basically, it's the equivalent of what formerly was import { PyScript } from './components/pyscript'

marimeireles · 2022-10-24T11:09:03Z

+
+
+// XXX this is used by base.ts but should be removed once we complete the refactoring
+export async function pyExecDontHandleErrors(runtime: Runtime, pysrc: string, out: HTMLElement)


[suggestion/question] I don't understand this name. Could you please explain it to me?
This piece of code is evaluating the strings coming from the front-end into python code. Should it be renamed?
Why aren't we catching errors here? Why keep this on base.ts?

I don't understand this name. Could you please explain it to me?
pyExec executes python code inside a try/catch:

pyscript/pyscriptjs/src/pyexec.ts

Lines 14 to 23 in f9194cc

try {

await runtime.run(pysrc);

}

catch (err) {

// XXX: currently we display exceptions in the same position as

// the output. But we probably need a better way to do that,

// e.g. allowing plugins to intercept exceptions and display them

// in a configurable way.

displayPyException(err, outElem);

}

pyExecDontHandleErrors does the same but without the try/catch: this means that if a python-level exception is raised, it will be propagated outside (and thus needs to be intercepted by someone else).

Why aren't we catching errors here? Why keep this on base.ts?

as the comment says, the plan is to kill it eventually and refactor base.ts/pyrepl.ts to use pyExec instead. But the old code was too messy and I didn't want to touch too many things at once, it's what I plan to do next.

marimeireles · 2022-10-24T11:18:03Z

+let _uniqueIdCounter = 0;
+export function ensureUniqueId(el: HTMLElement) {
+    if (el.id === "")
+        el.id = "py-internal-" + _uniqueIdCounter++;


I like this =)
I think next step is to actually try to remove all of these ids.

yes but it's not completely easy because Element.write relies on it. But yes, we should stop passing ids around and start passing real HTMLElement objects instead.

marimeireles · 2022-10-24T12:19:01Z

Thanks @antocuni! ^-^

antocuni added 30 commits October 21, 2022 15:36

move the code for registering ES modules into main.ts, and add a big …

cbedd43

…comment which explains some of the problems of the current approach. But I don't want to tackled it in this PR

start to move the logic for executing python code into pyExec

667ed3e

kill unused code

f31274a

remove the <string> cast, it's not necessary

507466a

kill the support for std-err: it was unused and it will be refactored

f8888d3

kill support for std-out as well

b1cd5a2

kill kill kill

1134c84

add a pytest option --dev which is like --headed but also automatical…

b3fc5bc

…ly open the devtools panel

typo

179befd

improve this test and mark it as xfail, see issue #878

14c6537

add a failing test

33b03a4

kill more unneeded code and adapt the tests accordingly

9f75fc8

kill guidGenerator and use sequential numbers instead, they are much …

591534c

…easier to test and to use in the debugger for humans

refactor pyExec to take an HTMLElement instead of an id

e2f4b8f

make PyScript a standalone class and remove the intheritance from Bas…

c8ba605

…eEvalElement. This contains a bit of mechnical copy&paste but I will simplify the code in the next commits

simplify this test. With test_pyscript_hello we just want to check th…

76280c4

…at we can execute a very simple python code, no need to check the behavior of display()

add a test for <py-script src=...> and implement the functionality

dca2ffb

we no longer need the shadow root for <py-script>

69d03aa

add the logic to display exceptions which are raised inside <py-scrip…

4999566

…t>, and the relevant test

Merge remote-tracking branch 'origin/main' into antocuni/kill-script-…

6cd5178

…queue

kill this test, addToOutput was killed

50c872f

kill pyscript.test.ts: it doesn't make sense to unit-test it at this …

eaacc21

…level because you would need to mock the whole Runtime, and you would end up testing the mock instead of something actually useful. PyScript is battle-tested a lot by integration tests

test_multiple_async_display is not really about async and it's not ab…

d3c68ea

…out multiple display. It is about implicit display() target, so move it it the correct class and improve the test to be less dependent on the exact <div> id

PyLoader doesn't have to inherit from BaseEvalElement

afdf355

py-title doesn't have to inherit from BaseEvalElement

b3d43d6

PyButton doesn't have to inherit from BaseEvalElement

ce1104b

PyInputBox doesn't have to inherit from BaseEvalElement

e345ed5

fix the PyTitle tests

5d46042

antocuni added 2 commits October 23, 2022 00:49

fix the pyinputbox and pybutton unit tests

f999e62

add a comment

ecac29a

antocuni mentioned this pull request Oct 22, 2022

Add more type definitions #882

Merged

antocuni changed the title ~~WIP: refactor how py-script are executed, kill scriptQueue store, introduce pyExec~~ Refactor how py-script are executed, kill scriptQueue store, introduce pyExec Oct 23, 2022

antocuni marked this pull request as ready for review October 23, 2022 17:44

antocuni requested review from JeffersGlass, madhur-tandon and tedpatrick October 23, 2022 17:44

tedpatrick approved these changes Oct 23, 2022

View reviewed changes

madhur-tandon reviewed Oct 23, 2022

View reviewed changes

Comment thread pyscriptjs/tests/unit/pytitle.test.ts Outdated

madhur-tandon approved these changes Oct 23, 2022

View reviewed changes

JeffersGlass reviewed Oct 23, 2022

View reviewed changes

JeffersGlass approved these changes Oct 23, 2022

View reviewed changes

JeffersGlass mentioned this pull request Oct 23, 2022

display() sends the output to the wrong tag #878

Closed

antocuni added 2 commits October 23, 2022 22:51

remove outdate comment

3eb07da

Merge remote-tracking branch 'origin/main' into antocuni/kill-script-…

b796d33

…queue

antocuni merged commit f9194cc into main Oct 23, 2022

antocuni deleted the antocuni/kill-script-queue branch October 23, 2022 21:31

This was referenced Oct 24, 2022

py-repl creating duplicate ids #864

Closed

Remove all remaining references to svelte #883

Closed

marimeireles reviewed Oct 24, 2022

View reviewed changes

JeffersGlass mentioned this pull request Nov 10, 2022

Evaluate dynamically added <py-script> tag #690

Closed

JeffersGlass mentioned this pull request Jan 11, 2023

Add REPL plugin hooks; Add output, output-mode, stderr attributes #1106

Merged

8 tasks

		}


		function displayPyException(err: any, errElem: HTMLElement) {

	export function make_PyScript(runtime: Runtime) {

	class PyScript extends HTMLElement {



		// XXX this is used by base.ts but should be removed once we complete the refactoring
		export async function pyExecDontHandleErrors(runtime: Runtime, pysrc: string, out: HTMLElement)

	try {
	await runtime.run(pysrc);
	}
	catch (err) {
	// XXX: currently we display exceptions in the same position as
	// the output. But we probably need a better way to do that,
	// e.g. allowing plugins to intercept exceptions and display them
	// in a configurable way.
	displayPyException(err, outElem);
	}

Conversation

antocuni commented Oct 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tedpatrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marimeireles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antocuni Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marimeireles commented Oct 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

antocuni commented Oct 22, 2022 •

edited

Loading

antocuni Oct 24, 2022 •

edited

Loading