Skip to content

Commit

Permalink
Introduce synchronous has_model function in the manager base class.
Browse files Browse the repository at this point in the history 
This simplifies our code that has had to work around get_model either
returning undefined or a rejected promise if a model was not registered.
This paves the way for a future get_model in 8.0 that is truly asynchronous, never returning undefined.
  • Loading branch information
@jasongrout
jasongrout committed Feb 11, 2022
1 parent c419bfe commit 2d32fe7
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 91 deletions.
1 change: 1 addition & 0 deletions docs/source/changelog.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Highlights include:
- Fix installation on Python 3.10. [#3368](https://github.com/jupyter-widgets/ipywidgets/pull/3368)
- Throw an error if we cannot render a widget, enabling the rendering system to fall back to rendering a different data type if available. [#3290](https://github.com/jupyter-widgets/ipywidgets/pull/3290)
- Create a new widget control comm channel, enabling more efficient fetching of kernel widget state. [#3201](https://github.com/jupyter-widgets/ipywidgets/pull/3021)
- Refactor logic for fetching kernel widget state to the manager base class. This logic first tries to use the new widget control comm channel, falling back to the existing method of requesting each widget's state individually. [#3337](https://github.com/jupyter-widgets/ipywidgets/pull/3337)
- Enable HTMLManager output widgets to render state updates. [#3372](https://github.com/jupyter-widgets/ipywidgets/pull/3372)
- Do not reset JupyterLab CSS variables if they are already defined. [#3344](https://github.com/jupyter-widgets/ipywidgets/pull/3344)
- Fix variable inspector example. [#3302](https://github.com/jupyter-widgets/ipywidgets/pull/3302)
Expand Down
154 changes: 63 additions & 91 deletions packages/base/src/manager-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,17 @@ abstract class ManagerBase<T> {
return this._models[model_id];
}

/**
* Returns true if the given model is registered, otherwise false.
*
* #### Notes
* This is a synchronous way to check if a model is registered, so that
* get_model above *could* be a purely async function.
*/
has_model(model_id: string): boolean {
return this._models[model_id] !== undefined;
}

/**
* Handle when a comm is opened.
*/
Expand Down Expand Up @@ -464,27 +475,10 @@ abstract class ManagerBase<T> {
// Create comms for all new widgets.
let widget_comms = await Promise.all(
Object.keys(states).map(async (widget_id) => {
let comm = undefined;
let modelPromise = undefined;
try {
modelPromise = this.get_model(widget_id);
if (modelPromise === undefined) {
comm = await this._create_comm('jupyter.widget', widget_id);
} else {
// For JLab, the promise is rejected, so we have to await to
// find out if it is actually a model.
await modelPromise;
}
} catch (e) {
// The JLab widget manager will throw an error with this specific error message.
if (e.message !== 'widget model not found') {
throw e;
}
comm = await this._create_comm('jupyter.widget', widget_id);
}
const comm = this.has_model(widget_id) ? undefined : await this._create_comm('jupyter.widget', widget_id);
return {widget_id, comm}
})
)
);

await Promise.all(widget_comms.map(async ({widget_id, comm}) => {
const state = states[widget_id];
Expand All @@ -497,28 +491,26 @@ abstract class ManagerBase<T> {
);
}
try {

if (comm === undefined) {
// model already exists here
const model = await this.get_model(widget_id);
model!.set_state(state.state);
} else {
// This must be the first await in the code path that
// reaches here so that registering the model promise in
// new_model can register the widget promise before it may
// be required by other widgets.
await this.new_model(
{
model_name: state.model_name,
model_module: state.model_module,
model_module_version: state.model_module_version,
model_id: widget_id,
comm: comm,
},
state.state
);
}

if (comm) {
// This must be the first await in the code path that
// reaches here so that registering the model promise in
// new_model can register the widget promise before it may
// be required by other widgets.
await this.new_model(
{
model_name: state.model_name,
model_module: state.model_module,
model_module_version: state.model_module_version,
model_id: widget_id,
comm: comm,
},
state.state
);
} else {
// model already exists here
const model = await this.get_model(widget_id);
model!.set_state(state.state);
}
} catch (error) {
// Failed to create a widget model, we continue creating other models so that
// other widgets can render
Expand All @@ -539,54 +531,36 @@ abstract class ManagerBase<T> {
// For each comm id that we do not know about, create the comm, and request the state.
const widgets_info = await Promise.all(
Object.keys(comm_ids).map(async (comm_id) => {
try {
const model = this.get_model(comm_id);
// TODO Have the same this.get_model implementation for
// the widgetsnbextension and labextension, the one that
// throws an error if the model is not found instead of
// returning undefined
if (model === undefined) {
throw new Error('widget model not found');
}
await model;
// If we successfully get the model, do no more.
if (this.has_model(comm_id)) {
return;
} catch (e) {
// If we have the widget model not found error, then we can create the
// widget. Otherwise, rethrow the error. We have to check the error
// message text explicitly because the get_model function in this
// class throws a generic error with this specific text.
if (e.message !== 'widget model not found') {
throw e;
}

const comm = await this._create_comm(this.comm_target_name, comm_id);

let msg_id = '';
const info = new PromiseDelegate<Private.ICommUpdateData>();
comm.on_msg((msg) => {
if (
(msg.parent_header as any).msg_id === msg_id &&
msg.header.msg_type === 'comm_msg' &&
msg.content.data.method === 'update'
) {
const data = msg.content.data as any;
const buffer_paths = data.buffer_paths || [];
const buffers = msg.buffers || [];
utils.put_buffers(data.state, buffer_paths, buffers);
info.resolve({ comm, msg });
}
const comm = await this._create_comm(this.comm_target_name, comm_id);

let msg_id = '';
const info = new PromiseDelegate<Private.ICommUpdateData>();
comm.on_msg((msg) => {
if (
(msg.parent_header as any).msg_id === msg_id &&
msg.header.msg_type === 'comm_msg' &&
msg.content.data.method === 'update'
) {
const data = msg.content.data as any;
const buffer_paths = data.buffer_paths || [];
const buffers = msg.buffers || [];
utils.put_buffers(data.state, buffer_paths, buffers);
info.resolve({ comm, msg });
}
});
msg_id = comm.send(
{
method: 'request_state',
},
this.callbacks(undefined)
);
});
msg_id = comm.send(
{
method: 'request_state',
},
this.callbacks(undefined)
);

return info.promise;
}
})
);
return info.promise;
}));

// We put in a synchronization barrier here so that we don't have to
// topologically sort the restored widgets. `new_model` synchronously
Expand Down Expand Up @@ -681,8 +655,8 @@ abstract class ManagerBase<T> {

// If the model has already been created, set its state and then
// return it.
if (this._models[model_id]) {
return this._models[model_id].then(model => {
if (this.has_model(model_id)) {
return this.get_model(model_id)!.then(model => {
// deserialize state
return (model.constructor as typeof WidgetModel)._deserialize_state(modelState || {}, this).then(attributes => {
model.set_state(attributes); // case 2
Expand Down Expand Up @@ -774,9 +748,7 @@ abstract class ManagerBase<T> {
protected filterExistingModelState(serialized_state: any) {
let models = serialized_state.state as {[key: string]: any};
models = Object.keys(models)
.filter((model_id) => {
return !this._models[model_id];
})
.filter(model_id => !this.has_model(model_id))
.reduce((res, model_id) => {
res[model_id] = models[model_id];
return res;
Expand Down

0 comments on commit 2d32fe7

Please sign in to comment.