Skip to content

Commit fbdbd00

Browse files
ErrorWidget as fallback when widgets models or views fail (#3304)
* Feat: ErrorWidget as fallback when widgets models or views fail. This makes sure that we always render something, with a descriptive error message. This makes is easier for users to diagnose or report issues. * remove log statements * make it work in the classical notebook * Rebase to latest master * Use unlink icon for broken widgets * Show/hide error stack on wdiget click * Update fallback widget style * Add tests for base manager * Lint code * Update error messages * Add more safety belts * Add more check * Update suggestions from reviewer * Rename `SVG_ICON` to `BROKEN_FILE_SVG_ICON` Co-authored-by: Maarten A. Breddels <[email protected]>
1 parent 713df6e commit fbdbd00

File tree

10 files changed

+348
-28
lines changed

10 files changed

+348
-28
lines changed

packages/base-manager/src/manager-base.ts

Lines changed: 89 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Distributed under the terms of the Modified BSD License.
33

44
import * as services from '@jupyterlab/services';
5+
import * as widgets from '@jupyter-widgets/base';
56

67
import { JSONObject, PartialJSONObject } from '@lumino/coreutils';
78

@@ -128,10 +129,12 @@ export abstract class ManagerBase implements IWidgetManager {
128129
const id = uuid();
129130
const viewPromise = (model.state_change = model.state_change.then(
130131
async () => {
132+
const _view_name = model.get('_view_name');
133+
const _view_module = model.get('_view_module');
131134
try {
132-
const ViewType = (await this.loadClass(
133-
model.get('_view_name'),
134-
model.get('_view_module'),
135+
const ViewType = (await this.loadViewClass(
136+
_view_name,
137+
_view_module,
135138
model.get('_view_module_version')
136139
)) as typeof WidgetView;
137140
const view = new ViewType({
@@ -153,7 +156,16 @@ export abstract class ManagerBase implements IWidgetManager {
153156
console.error(
154157
`Could not create a view for model id ${model.model_id}`
155158
);
156-
throw e;
159+
const msg = `Failed to create view for '${_view_name}' from module '${_view_module}' with model '${model.name}' from module '${model.module}'`;
160+
const ModelCls = widgets.createErrorWidgetModel(e, msg);
161+
const errorModel = new ModelCls();
162+
const view = new widgets.ErrorWidgetView({
163+
model: errorModel,
164+
options: this.setViewOptions(options),
165+
});
166+
await view.render();
167+
168+
return view;
157169
}
158170
}
159171
));
@@ -330,35 +342,53 @@ export abstract class ManagerBase implements IWidgetManager {
330342
serialized_state: any = {}
331343
): Promise<WidgetModel> {
332344
const model_id = options.model_id;
333-
const model_promise = this.loadClass(
345+
const model_promise = this.loadModelClass(
334346
options.model_name,
335347
options.model_module,
336348
options.model_module_version
337-
) as Promise<typeof WidgetModel>;
349+
);
338350
let ModelType: typeof WidgetModel;
351+
352+
const makeErrorModel = (error: any, msg: string) => {
353+
const Cls = widgets.createErrorWidgetModel(error, msg);
354+
const widget_model = new Cls();
355+
return widget_model;
356+
};
357+
339358
try {
340359
ModelType = await model_promise;
341360
} catch (error) {
342-
console.error('Could not instantiate widget');
343-
throw error;
361+
const msg = 'Could not instantiate widget';
362+
console.error(msg);
363+
return makeErrorModel(error, msg);
344364
}
345365

346366
if (!ModelType) {
347-
throw new Error(
367+
const msg = 'Could not instantiate widget';
368+
console.error(msg);
369+
const error = new Error(
348370
`Cannot find model module ${options.model_module}@${options.model_module_version}, ${options.model_name}`
349371
);
372+
return makeErrorModel(error, msg);
350373
}
374+
let widget_model: WidgetModel;
375+
try {
376+
const attributes = await ModelType._deserialize_state(
377+
serialized_state,
378+
this
379+
);
380+
const modelOptions: IBackboneModelOptions = {
381+
widget_manager: this,
382+
model_id: model_id,
383+
comm: options.comm,
384+
};
351385

352-
const attributes = await ModelType._deserialize_state(
353-
serialized_state,
354-
this
355-
);
356-
const modelOptions: IBackboneModelOptions = {
357-
widget_manager: this,
358-
model_id: model_id,
359-
comm: options.comm,
360-
};
361-
const widget_model = new ModelType(attributes, modelOptions);
386+
widget_model = new ModelType(attributes, modelOptions);
387+
} catch (error) {
388+
console.error(error);
389+
const msg = `Model class '${options.model_name}' from module '${options.model_module}' is loaded but can not be instantiated`;
390+
widget_model = makeErrorModel(error, msg);
391+
}
362392
widget_model.name = options.model_name;
363393
widget_model.module = options.model_module;
364394
return widget_model;
@@ -520,6 +550,46 @@ export abstract class ManagerBase implements IWidgetManager {
520550
moduleVersion: string
521551
): Promise<typeof WidgetModel | typeof WidgetView>;
522552

553+
protected async loadModelClass(
554+
className: string,
555+
moduleName: string,
556+
moduleVersion: string
557+
): Promise<typeof WidgetModel> {
558+
try {
559+
const promise: Promise<typeof WidgetModel> = this.loadClass(
560+
className,
561+
moduleName,
562+
moduleVersion
563+
) as Promise<typeof WidgetModel>;
564+
await promise;
565+
return promise;
566+
} catch (error) {
567+
console.error(error);
568+
const msg = `Failed to load model class '${className}' from module '${moduleName}'`;
569+
return widgets.createErrorWidgetModel(error, msg);
570+
}
571+
}
572+
573+
protected async loadViewClass(
574+
className: string,
575+
moduleName: string,
576+
moduleVersion: string
577+
): Promise<typeof WidgetView> {
578+
try {
579+
const promise: Promise<typeof WidgetView> = this.loadClass(
580+
className,
581+
moduleName,
582+
moduleVersion
583+
) as Promise<typeof WidgetView>;
584+
await promise;
585+
return promise;
586+
} catch (error) {
587+
console.error(error);
588+
const msg = `Failed to load view class '${className}' from module '${moduleName}'`;
589+
return widgets.createErrorWidgetView(error, msg);
590+
}
591+
}
592+
523593
/**
524594
* Create a comm which can be used for communication for a widget.
525595
*

packages/base-manager/test/src/dummy-manager.ts

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,51 @@ class TestWidget extends widgets.WidgetModel {
9595
};
9696
}
9797
}
98+
class ModelErrorWidget extends widgets.WidgetModel {
99+
defaults(): Backbone.ObjectHash {
100+
return {
101+
...super.defaults(),
102+
_model_module: 'test-widgets',
103+
_model_name: 'ModelErrorWidget',
104+
_model_module_version: '1.0.0',
105+
};
106+
}
107+
initialize(attributes: Backbone.ObjectHash, options: any) {
108+
throw new Error('Model error');
109+
}
110+
}
111+
class ModelWithMissingView extends widgets.WidgetModel {
112+
defaults(): Backbone.ObjectHash {
113+
return {
114+
...super.defaults(),
115+
_model_module: 'test-widgets',
116+
_model_name: 'ModelWithViewError',
117+
_model_module_version: '1.0.0',
118+
_view_module: 'test-widgets',
119+
_view_name: 'MissingView',
120+
_view_module_version: '1.0.0',
121+
};
122+
}
123+
}
124+
class ModelWithViewError extends widgets.WidgetModel {
125+
defaults(): Backbone.ObjectHash {
126+
return {
127+
...super.defaults(),
128+
_model_module: 'test-widgets',
129+
_model_name: 'ModelWithViewError',
130+
_model_module_version: '1.0.0',
131+
_view_module: 'test-widgets',
132+
_view_name: 'ViewErrorWidget',
133+
_view_module_version: '1.0.0',
134+
};
135+
}
136+
}
137+
138+
class ViewErrorWidget extends widgets.WidgetView {
139+
render(): void {
140+
throw new Error('Render error');
141+
}
142+
}
98143

99144
class TestWidgetView extends widgets.WidgetView {
100145
render(): void {
@@ -136,6 +181,10 @@ const testWidgets = {
136181
TestWidgetView,
137182
BinaryWidget,
138183
BinaryWidgetView,
184+
ModelErrorWidget,
185+
ModelWithViewError,
186+
ViewErrorWidget,
187+
ModelWithMissingView,
139188
};
140189

141190
export class DummyManager extends ManagerBase {

packages/base-manager/test/src/manager_test.ts

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,36 @@ describe('ManagerBase', function () {
6464
expect(view._rendered).to.equal(1);
6565
});
6666

67+
it('return ErrorWidget if view class can not be loaded', async function () {
68+
const spec = {
69+
model_name: 'ModelWithMissingView',
70+
model_module: 'test-widgets',
71+
model_module_version: '1.0.0',
72+
model_id: 'id',
73+
};
74+
const manager = this.managerBase;
75+
const model = await manager.new_model(spec);
76+
const view = await manager.create_view(model);
77+
expect(view.generateErrorMessage()['msg']).to.be.equal(
78+
"Failed to load view class 'MissingView' from module 'test-widgets'"
79+
);
80+
});
81+
82+
it('return ErrorWidget if view class can not be created', async function () {
83+
const spec = {
84+
model_name: 'ModelWithViewError',
85+
model_module: 'test-widgets',
86+
model_module_version: '1.0.0',
87+
model_id: 'id',
88+
};
89+
const manager = this.managerBase;
90+
const model = await manager.new_model(spec);
91+
const view = await manager.create_view(model);
92+
expect(view.generateErrorMessage()['msg']).to.be.equal(
93+
"Failed to create view for 'ViewErrorWidget' from module 'test-widgets' with model 'ModelWithViewError' from module 'test-widgets'"
94+
);
95+
});
96+
6797
it('removes the view on model destroy', async function () {
6898
const manager = this.managerBase;
6999
const model = await manager.new_model(this.modelOptions);
@@ -313,7 +343,33 @@ describe('ManagerBase', function () {
313343
);
314344
});
315345

316-
it('throws an error if there is an error loading the class');
346+
it('return ErrorWidget if model class can not be loaded', async function () {
347+
const spec = {
348+
model_name: 'Foo',
349+
model_module: 'bar',
350+
model_module_version: '1.0.0',
351+
model_id: 'id',
352+
};
353+
const manager = this.managerBase;
354+
const model = await manager.new_model(spec);
355+
expect(model.get('msg')).to.be.equal(
356+
"Failed to load model class 'Foo' from module 'bar'"
357+
);
358+
});
359+
360+
it('return ErrorWidget if model can not be created', async function () {
361+
const spec = {
362+
model_name: 'ModelErrorWidget',
363+
model_module: 'test-widgets',
364+
model_module_version: '1.0.0',
365+
model_id: 'id',
366+
};
367+
const manager = this.managerBase;
368+
const model = await manager.new_model(spec);
369+
expect(model.get('msg')).to.be.equal(
370+
"Model class 'ModelErrorWidget' from module 'test-widgets' is loaded but can not be instantiated"
371+
);
372+
});
317373

318374
it('does not sync on creation', async function () {
319375
const comm = new MockComm();

packages/base/css/index.css

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,28 @@
1414
padding: 3px;
1515
align-self: flex-start;
1616
}
17+
18+
.jupyter-widgets-error-widget {
19+
display: flex;
20+
flex-direction: column;
21+
justify-content: center;
22+
height: 100%;
23+
border: solid 1px red;
24+
margin: 0 auto;
25+
}
26+
27+
.jupyter-widgets-error-widget.icon-error {
28+
min-width: var(--jp-widgets-inline-width-short);
29+
}
30+
.jupyter-widgets-error-widget.text-error {
31+
min-width: calc(2 * var(--jp-widgets-inline-width));
32+
min-height: calc(3 * var(--jp-widgets-inline-height));
33+
}
34+
35+
.jupyter-widgets-error-widget p {
36+
text-align: center;
37+
}
38+
39+
.jupyter-widgets-error-widget.text-error pre::first-line {
40+
font-weight: bold;
41+
}

0 commit comments

Comments
 (0)