Use the new pyodide feature for better tracebacks, as soon as it's available

[antocuni]

I'm opening this issue mostly as a reminder, so that we don't forget.
In the next days a new pyodide release will be available. The new release will contain this feature:
pyodide/pyodide#3993

Now runPython accepts a new argument filename: if it's used, the python tracebacks will look much nicer and will contain references to the actual source code written by the user, so we should just always use it.

[WebReflection]

beside the need to update to latest interpreter, every interpreter already accept any argument with their own run or runAsync API so in that regard there's nothing to do here, if I am not mistaken ... see https://github.com/pyscript/polyscript/blob/main/esm/interpreter/_python.js#L10-L26

[WebReflection]

Reading that page ... this is really a PyScript enhancement, eventually, and imho definitively not a release blocker.

I've also no idea how we're supposed to define the HTML line when every kind of bundler might shrink or change that layout so that in production that'd be filename: "index.html", lineno: 1 all over the place but I don't know how to relate an element line in a parsed page beside using an indexOf over an outerHTML that will likely misleady because we also need to set the style display block at distance and everyone else can set other attributes and stuff so this feature looks like not super useful for the Web, or really pointless as soon as any class, attribute, content, is changed (we also cleanup the textContent of <py-script> element so I really struggle to see what is this about or how this improve anything for us).

[WebReflection]

last blocker to me:

IMO, this is not a big problem. I think people who use this option should know what they are doing and should not reuse the filename.

I think I personally don't know what we're doing as we can't map a node into a line in any HTML document ... the live DOM is not a 1:1 relation to what's served to the user.

edit we could try to snapshot each time before execution but I don't see that granting better results ... in short, we can use that feature, I don't know who would benefit from it ... let's try once it's out though, maybe I am missing something.

[WebReflection]

P.S. these questions are likely not relevant for the src case where we do have a file, I am just a bit skeptical about the index.html presented use case there ... although code should not be minified (if in scripts, if it's in <py-script> I wouldn't have a clue) so maybe it will work out.

[antocuni]

it's not necessary to have a precise line number: if we can compute it, that's good, if we cannot, we can always use a fake number and that's ok as well.
E.g. in this case:

<py-script>
    print('foo')
</py-script>

<py-script>
    print('bar')
</py-script>

<py-script>
    print('baz')
</py-script>

I think it's perfectly fine to use fake names such as index.html-1, index.html-2 and index.html-3, each starting a line 1.
Or, if our <script> tag have an id, we could even use index.html#id-of-the-script. Literally anything would work.

By doing this you get wrong line numbers in tracebacks, but at least you get source code, which IMHO is the most important thing.

To explain better what I mean, consider this code:

        <script type="py">
          def foo():
              bar()

          def bar():
              0/0

          foo()
        </script>

Currently you get this traceback:

Note the three lines which says File "<exec>": these are in theory the three lines showing the user code, but pyodide cannot because it doesn't have a filename.
By giving a filename (even if it's completely fake), pyodide will be able to produce a better traceback, something like this:

Traceback (most recent call last):
  File "index.html-1", line 7, in <module>
    foo()
  File "index.html-1", line 2, in foo
    bar()
  File "index.html-1", line 5, in bar
    0/0
ZeroDivisionError: division by zero

[WebReflection]

Happy to experiment once the feature is out. The relation code -> file becomes even more weird with worker either as attribute or as external file as these will be passed as Blob content (entry point) ... all nodes have a target and/or automatic ID so maybe we an use worker-X there too ... we'll see what we can do when we can use this feature.

[WebReflection]

Btw, this is still not a polyscript issue as polyscript doesn't care about what's passed along run or runAsync so we better move this issue to pyscript: pyscript/pyscript#1701

[antocuni]

Btw, this is still not a polyscript issue as polyscript doesn't care about what's passed along run or runAsync so we better move this issue to pyscript: pyscript/pyscript#1701

I still think it should be a polyscript feature, because it's useful even with raw pyodide: so, if you enable it in polyscript, everyone using <script type="pyodide"> would benefit for it.

That said, your call. As long as we have this in pyscript, I'm happy :)

[WebReflection]

polyscript enable any argument to be passed ... to all interpreters ... that's it. It passes anything we want to pass to pyodide already, so that <script type="pyodide"> will already benefit. There's nothing to do in here for any future change to any interpreter, unless they break the contract of passing a string as first argument. This should stay closed.

[WebReflection]

On a second though, if you think that dance should be done in here as Pyodide only thing, then that might be case we add Pyodide only features but code will branch out in ugly ways ... although if custom scripts are handled differently, that's an architecture change to think though with the interpreters folders ... reopening until I have something to test, so far this all feel too theoretical and there's no action I can take until Pyodide is out with its latest (yes, I could use the alpha/beta version, but that cannot land in here so I don't want to break previous stable code until that's out).

[antocuni]

On a second though, if you think that dance should be done in here as Pyodide only thing, then that might be case we add Pyodide only features

yes, this is what I meant, sorry if I wasn't clear enough.
But again, I don't have any strong opinion about whether this should be in polyscript or not.
As a pyodide user, I see only advantages in using the new options and I would like to have it by default without having to do anything, but if this makes the code too complicated, maybe it's not so important.

[WebReflection]

it is the right thing to do when interpreters support new features, stuff happens in here ... it's just the branching code that is not nice because potentially any interpreter could add its own extra feature and it'll be nuts to branch all those cases beside the run or runAsync offered abstraction ... but if we can confine those cases within these utilities, it should be all good.

[bugzpodder]

@antocuni if you put a package that cannot be loaded via pyodide.loadPackage into loadPyodide's option.packages it will throw an error. eg try putting seaborn and it will not be happy

edit: this comment is on completely wrong issue..

[WebReflection]

btw ... we have a blocker for 0.24.0 and we need to wait for the next release, apparently:
pyodide/pyodide#4153 (comment)

[WebReflection]

Update ... in this MR #81 we tackled a different, more relevant, issue: all hooks before the code would make the line number completely banana/irrelevant ... and that got fixed.

As of today, that was the only complain about the error we had because indeed if the stack trace was saying line 28 and that was just the first line of user's code after our stdlib injection, there really was an issue.

Now that such issue has been solved, I'd like to understand how we can also use filename at least in Pyodide and here is my thinking:

• if the code is in a file, use that file name (whole URL? just basename?)
• if the code is inline, don't use a filename ... I'll explain why in a sec ...

With this case:

<script type="py">
error 1
</script>
<script type="py">
error 2
</script>

It makes no sense to have error in index.html at line 1 because that's not what index.html contains at line 1 and code is simply evaluated out of a specific script and, if there's no disambiguation among scripts, it also makes no sense to state error in <script type="py"> as more than one script like that could be on the page.

On top of that:

<script type="py" worker>
error 1
</script>
<script type="py" worker>
error 2
</script>

Now the code doesn't even run in index.html ... it gets a dedicated Object URL that means nothing to our users so that I think, until or unless we find a better solution, the exec indication that code was evaluated from the page works better than a misleading index.html or a generic hex Object URL generated ad-hoc but transparent for users.

This would also kinda hint/promote to use external files instead of trashing Python all over the HTML page, which is, I think, a good hint and a better way forward.

Thoughts?

[ntoll]

👋 A quick summary of my thoughts:

• Clear error messages are important. It's not fun to have the wrong line numbers etc.
• Some aspects of the context in which PyScript runs make this a challenge (inline Python code in index.html or inline code running on a worker etc... as @WebReflection illustrated above).
• I'm not sure there is a technical solution to this, except that we encourage folks to put all their code into files and so tracebacks/line numbers etc... have a concrete context in which to work.
• Bonus: what about MicroPython..? 😉

[WebReflection]

Thanks @ntoll , I think we can close this until we have a plan and solutions around the issues I've rised.

Maybe having index.html at line X is what @antocuni wanted all along, worker or not, but that breaks easily when a worker, as example, adds a listener and the error is in that listener at line X and the listener triggered lazily after user action ... I think right now we are in a better place, after #81 landing, so I'd like to stop thinking about this issue and move forward with other more relevant issues (because as previously mentioned, nobody complaind about the line in the index, complains arrived because we prepended evaluated code without splitting its evaluation ... and that's solved now).