<rant>
During the infinite discussion in #769 we decided to use a "dynamic" implicit target for display(), which as far as I know can be only implemented with a global variable.
I am still convinced that this approach is very fragile and it will create tons of problems and nasty and subtle bugs.
I predict this is the first of many 🔮
We are still in time to change our minds.
</rant>
I found this problem during an unrelated refactoring. I think I know what happens but I don't want to solve it now, so I'm writing an issue to make sure we don't forget.
Consider this test:
self.pyscript_run(
"""
<py-script>
display('hello 1')
</py-script>
<p>hello 2</p>
<py-script>
display('hello 3')
</py-script>
"""
)
It displays hello 2, hello 1, hello 3:

I think the bug was introduce by PR #749 but the roots of the bug are much deeper than this and shows some criticality on our code base.
To start with, the current test that we have is suboptimal:
|
def test_consecutive_display(self): |
|
self.pyscript_run( |
|
""" |
|
<py-script> |
|
display('hello 1') |
|
</py-script> |
|
<py-script> |
|
display('hello 2') |
|
</py-script> |
|
""" |
|
) |
|
# need to improve this to get the first/second input |
|
# instead of just searching for it in the page |
|
inner_html = self.page.content() |
|
first_pattern = r'<div id="py-.*?-2">hello 1</div>' |
|
assert re.search(first_pattern, inner_html) |
|
second_pattern = r'<div id="py-.*?-3">hello 2</div>' |
|
assert re.search(second_pattern, inner_html) |
|
|
|
assert first_pattern is not second_pattern |
it seems to test for the right thing, but because of the mess that we do when generating outputs, the test passes "by chance". The problem is that to display outputs we create <div>s inside <div>s inside <div>s, each with a random ID, which makes everything very hard to test. We should improve testability of such a simple thing. This will hopefully be solved as part of the PR I'm working on.
The other problem is much deeper. I think that the root cause is here:
|
try { |
|
<string>await runtime.run(`set_current_display_target(target_id="${this.id}")`); |
|
<string>await runtime.run(source); |
|
} finally { |
|
<string>await runtime.run(`set_current_display_target(target_id=None)`); |
|
} |
The thing to note is that Runtime.run is async, so the three calls are not executed in strict order. I think that what happens is roughly the following:
main.ts starts to execute the first py-script. We run set_current_display_target(FIRST_TAG), but since it's async we immediately interrupt the execution
main.ts starts to execute the second py-scritp. We run set_current_display_target(SECOND_TAG).
- at some point in the future we execute the rest of the
run(), but now the "current display target" is always SECOND_TAG.
The proper fix is to ensure that the three calls are done atomically, but it's very hard to to it reliably.
A quick&dirty fix is probably to concatenate the three strings into one big string, but it's a very slipper slope. It might work right now because we execute everything in the global namespace (and we shouldn't) and it will break as soon as we stop doing it (e.g. to implement custom namespaces).
Another "maybe fix" is to use the famous ContextVar, but AFAIK it's a python-only thing. I don't really know how it interacts with the JS event loop and I think we should be very cautious before using it without fully understanding how it works in pyodide.
Another possible fix is to use synchronous calls to run: this probably solves the problem, but it breaks all py-script tags which uses await at top level. FWIW, this is one more data point which indicates that maybe they should not be allowed.
Note that currently we don't have any way to run pyodide code synchronously, which should probably kill Runtime.run and introduce Runtime.runSync and Runtime.runAsync.
<rant>During the infinite discussion in #769 we decided to use a "dynamic" implicit target for
display(), which as far as I know can be only implemented with a global variable.I am still convinced that this approach is very fragile and it will create tons of problems and nasty and subtle bugs.
I predict this is the first of many 🔮
We are still in time to change our minds.
</rant>I found this problem during an unrelated refactoring. I think I know what happens but I don't want to solve it now, so I'm writing an issue to make sure we don't forget.
Consider this test:
It displays

hello 2, hello 1, hello 3:I think the bug was introduce by PR #749 but the roots of the bug are much deeper than this and shows some criticality on our code base.
To start with, the current test that we have is suboptimal:
pyscript/pyscriptjs/tests/integration/test_02_output.py
Lines 19 to 38 in aa85f5f
it seems to test for the right thing, but because of the mess that we do when generating outputs, the test passes "by chance". The problem is that to display outputs we create
<div>s inside<div>s inside<div>s, each with a random ID, which makes everything very hard to test. We should improve testability of such a simple thing. This will hopefully be solved as part of the PR I'm working on.The other problem is much deeper. I think that the root cause is here:
pyscript/pyscriptjs/src/components/base.ts
Lines 119 to 124 in aa85f5f
The thing to note is that
Runtime.runisasync, so the three calls are not executed in strict order. I think that what happens is roughly the following:main.tsstarts to execute the first py-script. We runset_current_display_target(FIRST_TAG), but since it'sasyncwe immediately interrupt the executionmain.tsstarts to execute the second py-scritp. We runset_current_display_target(SECOND_TAG).run(), but now the "current display target" is alwaysSECOND_TAG.The proper fix is to ensure that the three calls are done atomically, but it's very hard to to it reliably.
A quick&dirty fix is probably to concatenate the three strings into one big string, but it's a very slipper slope. It might work right now because we execute everything in the global namespace (and we shouldn't) and it will break as soon as we stop doing it (e.g. to implement custom namespaces).
Another "maybe fix" is to use the famous
ContextVar, but AFAIK it's a python-only thing. I don't really know how it interacts with the JS event loop and I think we should be very cautious before using it without fully understanding how it works in pyodide.Another possible fix is to use synchronous calls to
run: this probably solves the problem, but it breaks allpy-scripttags which usesawaitat top level. FWIW, this is one more data point which indicates that maybe they should not be allowed.Note that currently we don't have any way to run pyodide code synchronously, which should probably kill
Runtime.runand introduceRuntime.runSyncandRuntime.runAsync.