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

Feat: ErrorWidget as fallback when widgets models or views fail. #2960

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

maartenbreddels
Copy link
Member

This makes sure that we always render something, with a descriptive
error message. This makes it easier for users to diagnose or report
issues.

JupyterLab screenshot:

image

Note that in all cases, the Button and Hbox are rendered.

This makes sure that we always render something, with a descriptive
error message. This makes is easier for users to diagnose or report
issues.
@maartenbreddels
Copy link
Member Author

This replaces voila-dashboards/voila#667 but is a better version because we can distinguish between loading models and views.

packages/base/src/errorwidget.ts Outdated Show resolved Hide resolved
packages/base/src/errorwidget.ts Outdated Show resolved Hide resolved
packages/base/src/errorwidget.ts Show resolved Hide resolved
@vidartf
Copy link
Member

vidartf commented Sep 17, 2020

So previously we used to use the various other mimetypes in the bundle to present an error message (but we stopped doing that?). Would you mind mentioning why this is better?

@martinRenou
Copy link
Member

So previously we used to use the various other mimetypes in the bundle to present an error message

I am really interested in this. Was it really what was happening? I feel like the widget mimetype was always rendered, only that this mimetype was showing Model not found or similar errors when the widget manager fails to render.
It would be really nice in ipympl to have a fallback image mimetype when the widget manager fails to render, but it does not seem to work.

@vidartf
Copy link
Member

vidartf commented Sep 17, 2020

Context in #2007. We seemed to agree then that the best long term solution was "a short HTML entry with a link to an entry in the docs describing the possible causes of seeing the message". Doing something smarter on the JS side might also be a good thing, but I think there is also value in having a shared/similar way of doing things for various different errors that can occur. I.e. I don't think this PR will solve the error message if the user forgot to install the jupyter widget manager (or the install is broken).

tl;dr: not opposed to this, but if another solution solves more problems, we should at least compare them.

@vidartf
Copy link
Member

vidartf commented Sep 17, 2020

Ugh, it seems the notebook issue that was preventing us from using html mimetype is still open: jupyter/notebook#2980

Xref: #2024 that is tracking the followup to that.

@maartenbreddels
Copy link
Member Author

Would you mind mentioning why this is better?

The error in this PR give more information because they are aware of the issue (missing Model, missing View). The mimetype fallback msg will only give a general error msg, and not a stacktrace.

The errors given now in Jupyter Lab are:
image

and classical:
image

@SylvainCorlay
Copy link
Member

I would like to get this in for 8.0 if possible.

moduleVersion
) as Promise<typeof WidgetModel>;
await promise;
return promise;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to await the promise before returning it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to catch a possible exception

@SylvainCorlay SylvainCorlay added this to the 8.0 milestone Feb 5, 2021

export class ErrorWidgetView extends DOMWidgetView {
render() {
console.log('render');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct

@davidbrochart
Copy link
Member

@maartenbreddels is it supposed to work with classical notebook? It prints:

Failed to load widget 'DOMWidgetModel' from module 'badwidget', error:
Error: Script error for "badwidget"
http://requirejs.org/docs/errors.html#scripterror
    at makeError (http://localhost:8888/static/components/requirejs/require.js?v=d37b48bb2137faa0ab98157e240c084dd5b1b5e74911723aa1d1f04c928c2a03dedf922d049e4815f7e5a369faa2e6b6a1000aae958b7953b5cc60411154f593:168:17)
    at HTMLScriptElement.onScriptError (http://localhost:8888/static/components/requirejs/require.js?v=d37b48bb2137faa0ab98157e240c084dd5b1b5e74911723aa1d1f04c928c2a03dedf922d049e4815f7e5a369faa2e6b6a1000aae958b7953b5cc60411154f593:1735:36)

@maartenbreddels
Copy link
Member Author

I think I have tested this, is this the js console?

@davidbrochart
Copy link
Member

No, the cell output.

@maartenbreddels
Copy link
Member Author

That should be exactly what it does, or do you refer to the short stack trace?
Looking at voila-dashboards/voila#667 it seems to do the same as what I got.

@davidbrochart
Copy link
Member

Yes, I was wondering why we don't have the same stack trace as in jlab.

@maartenbreddels
Copy link
Member Author

might be due to the transpiling target, I think in the notebook we still use something older (ES5?) and we lose the stack trace, but not 100% sure.

@davidbrochart
Copy link
Member

OK, thanks.
I tried your PR on jlab3 by rebasing it on #3086, it works fine.

@jasongrout
Copy link
Member

We're pushing forward to ipywidgets 8. Is there someone that wants to either champion this for 8.0 very soon, or can we move it to 8.1?

@jasongrout jasongrout modified the milestones: 8.0, 8.1 Aug 10, 2021
@jasongrout
Copy link
Member

Moving to 8.1 since a champion has not come forth.

@trungleduc
Copy link
Contributor

Hello @maartenbreddels, I'm trying to finish this PR. How do you think about this behavior of failback widget?

ipy.mp4

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

Successfully merging this pull request may close these issues.

7 participants