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

[next] Improved error reporting #1596

Open
antocuni opened this issue Jul 14, 2023 · 2 comments
Open

[next] Improved error reporting #1596

antocuni opened this issue Jul 14, 2023 · 2 comments
Assignees

Comments

@antocuni
Copy link
Contributor

antocuni commented Jul 14, 2023

One of the area in which pyscript-next is not yet on feature parity with pyscript-classic is error reporting. The goal of this issue is to summarize what are the features that we want in order to improve that.

I noticed that when we talk about "pyscript devs" or "users" it's easy to get confused because there is some ambiguity, so let's start by giving some definitions:

  • pyscript devs: the people who develops pyscript: i.e. us :)
  • pyscript users: the people who develops apps with pyscript.
  • final users: the people using the apps developed by pyscript users.

About errors, we have:

  • internal errors: errors caused by a bug in pyscript. Ideally, these should not exist (but we are not in an ideal world, so we all know they will :))
  • user errors: errors caused by pyscript users not doing things properly: e.g., Python-level exceptions, wrong URLs, wrong config files, etc.
  • app errors: errors caused by the final users when using the app, such as clicking on the wrong button, putting letters when a number is expected, etc. This kind of error is explicitly excluded from this conversation.

The goal of "improved error reporting" is to make life easier to pyscript users. It is not about making things nicer for final users (apart very indirectly because if pyscript users can work better, they can make better apps).


Rationale and current status in pyscript-classic

Many of the design decisions which I took in pyscript-classic w.r.t. error reporting are based on the following assumptions and goals:

  • assume that the Chrome Dev Tools don't exist. Many pyscript users are not experienced developers and they might have never heard of dev tools. Moreover, on mobile devices (and maybe tablets) dev tools might not exist at all
  • assume that pyscript users do not read messages printed to the console. This derives directly from the point above.
  • user errors should be displayed in the DOM
  • user warnings should be displayed in the DOM. This is annoying for final users, but it's the job of pyscript users to silence them.
  • user errors and warnings should ideally include a link to the relevant docs, if applicable
  • errors should never pass silently: for example, if there is a key in the config file which is unknown, we should assume that it's a typo and report it accordingly (either via a warning or an error)
  • unless explicitly silenced: pyscript users should have a way to customize and/or turn off the default behavior. For example, "display errors in the DOM" should be the default, but they should be able to change this.

In pyscript-classic, the behavior above is implemented in the following way:

  • throwing subclasses of UserError: if we detect something wrong, we can just throw new UserError() and be sure that it will be displayed to the user in the most appropriate way. Example:

    throw new UserError(
    ErrorCode.BAD_CONFIG,
    `The config supplied: ${configText} is an invalid TOML and cannot be parsed`,
    );

  • UserErrors are handled here:

    // Error handling logic: if during the execution we encounter an error
    // which is ultimate responsibility of the user (e.g.: syntax error in the
    // config, file not found in fetch, etc.), we can throw UserError(). It is
    // responsibility of main() to catch it and show it to the user in a
    // proper way (e.g. by using a banner at the top of the page).
    async main() {
    try {
    await this._realMain();
    } catch (error) {
    await this._handleUserErrorMaybe(error);
    }
    }

  • Whenever we run Python code, we try/catch errors and display it accordingly:

    try {
    return await interpreter.run(pysrc, outElem.id);
    } catch (e) {
    const err = e as Error;
    // 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);
    return { result: undefined };
    }

Note that even pyscript-classic doesn't always obey to these rules, but it should be considered a bug.
For example, there are a few cases in pyscript-classic in which we incorrectly use console.warn to emit a warning (which is not seen by anyone in 99% of the cases):

js.console.warn(
"WARNING: This plugin is still in a very experimental phase and will likely change"
" and potentially break in the future releases. Use it with caution."
)

def write(element_id, value, append=False, exec_id=0):
"""Writes value to the element with id "element_id"""
Element(element_id).write(value=value, append=append)
js.console.warn(
dedent(
"""PyScript Deprecation Warning: PyScript.write is
marked as deprecated and will be removed sometime soon. Please, use
Element(<id>).write instead."""
)
)

All of the behaviors described above are fully tested by integration tests. For example:

def test_python_exception(self):
self.pyscript_run(
"""
<py-script>
print('hello pyscript')
raise Exception('this is an error')
</py-script>
"""
)
assert "hello pyscript" in self.console.log.lines
self.check_py_errors("Exception: this is an error")
#
# check that we sent the traceback to the console
tb_lines = self.console.error.lines[-1].splitlines()
assert tb_lines[0] == "[pyexec] Python exception:"
assert tb_lines[1] == "Traceback (most recent call last):"
assert tb_lines[-1] == "Exception: this is an error"
#
# check that we show the traceback in the page. Note that here we
# display the "raw" python traceback, without the "[pyexec] Python
# exception:" line (which is useful in the console, but not for the
# user)
pre = self.page.locator("py-script > pre")
tb_lines = pre.inner_text().splitlines()
assert tb_lines[0] == "Traceback (most recent call last):"
assert tb_lines[-1] == "Exception: this is an error"

For internal errors, we don't do anything special, AFAIK: i.e., they are normal JS exceptions which are just displayed in the console. (Not saying that this is the right thing to do, just explaining what is the current behavior.

The exact styling for errors and warnings is not set in stone, of course. Ideally, it should be customizable and configurable in various ways (e.g., by having a "notification icon" which displays errors when clicked), but we never managed to implement that.


pyscript-next

I believe that we should follow the same rationale and goals for pyscript-next, and we should implement all the feature described above.
This includes e.g. errors which happens inside workers, which should somehow passed to the main thread and displayed in the DOM.

What belongs to polyscript and what to pyscript is unclear to me. I am happy with whatever decision as long as the pyscript users see the behavior above.

We could also reconsider what to do for internal errors: it might be a good idea to show them very explicitly, so that it's easier for people to open a bug report to pyscript (instead of just saying "my app doesn't work" or "pyscript doesn't work").

@WebReflection
Copy link
Contributor

WebReflection commented Jul 18, 2023

What belongs to polyscript and what to pyscript is unclear to me.

polyscript won't follow any pyscript convention but it should provide a way to create custom types (i.e. pyscript) able to do everything you mentioned.

In particular, I think polyscript should simply allow a custom stderr and be sure that any error passes through it, signaling somehow the error type, but it shouldn't be polyscript responsibility to present that error on the page as any custom type might want to present errors however they like/prefer, including pyscript.

This would decouple polyscript from the presentation layer completely, still allowing all sort of customization for pyscript and others ... does this make sense?

P.S. because of how Workers technically work, there's zero point in creating UserError and other similar classes as classes are lost in a postMessage related dance, so we can't take any concrete advantage from that ... the custom type should receive however an error able to disambiguate the type but I haven't figured out what'd be the best way to do so ... maybe a type field, among message and stack should be enough.

@antocuni
Copy link
Contributor Author

[cut]

This would decouple polyscript from the presentation layer completely, still allowing all sort of customization for pyscript and others ... does this make sense?

yes, sounds good to me!

P.S. because of how Workers technically work, there's zero point in creating UserError and other similar classes as classes are lost in a postMessage related dance, so we can't take any concrete advantage from that ...

right, I think that we had the same problem in pyscript-classic using synclink.

the custom type should receive however an error able to disambiguate the type but I haven't figured out what'd be the best way to do so ... maybe a type field, among message and stack should be enough.

yes, or maybe a field isUserError = true. Anyway, the concrete details are not so important I think, as long as we are able to disambiguate the two.

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

No branches or pull requests

2 participants