Skip to content

Widget persistence: only create comm for widgets having valid widget ids #62

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 30, 2015
Merged

Widget persistence: only create comm for widgets having valid widget ids #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e1142b4
get_comm_info function
SylvainCorlay Jul 13, 2015
d6c7712
Don't create comm object for invalid ids and don't request state for …
SylvainCorlay Jul 13, 2015
9330746
Workaround for absence of comm_info_[request/reply]
SylvainCorlay Jul 15, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ipywidgets/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,15 @@ def _handle_ipython():
load_ipython_extension(ip)

_handle_ipython()


# Workaround for the absence of a comm_info_[request/reply] shell message
class CommInfo(Widget):
"""CommInfo widgets are is typically instantiated by the front-end. As soon as it is instantiated, it sends the collection of valid comms, and kills itself. It is a workaround to the absence of comm_info shell message."""

def __init__(self, **kwargs):
super(CommInfo, self).__init__(**kwargs)
self.send(dict(comms={
k: dict(target_name=v.target_name) for (k, v) in self.comm.kernel.comm_manager.comms.items()
}))
self.close()
115 changes: 82 additions & 33 deletions ipywidgets/static/widgets/js/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,22 @@ define([
};

WidgetManager.prototype.create_model = function (options) {
/**
* For backward compatibility. Custom widgets may be relying on the fact
* that create_model was creating a comm if none was provided in options.
* Unlike the old version of create_model, if no comm is passed,
Copy link
Contributor

Choose a reason for hiding this comment

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

Backwards incompatibility

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not backward incompatible because we could not pass model id in the options before.
The new create_model only allows setting the model id.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha, read too quick. Saw "Unlike the old version of create_model"

* options.model_id is used to create the new comm.
*/
console.warn('WidgetManager.create_model is deprecated. Use WidgetManager.new_model');
if (!options.comm) {
options.comm = this.comm_manager.new_comm('ipython.widget',
{'widget_class': options.widget_class},
options.model_id);
}
return this.new_model(options);
}

WidgetManager.prototype.new_model = function (options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create a new method? Why not change create_model?

Copy link
Member Author

Choose a reason for hiding this comment

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

because create_model creates a new comm when it is not passed one. Hence it does not allow comm-less models.

/**
* Create and return a promise for a new widget model
*
Expand All @@ -302,7 +318,7 @@ define([
* Example
* --------
* JS:
* IPython.notebook.kernel.widget_manager.create_model({
* IPython.notebook.kernel.widget_manager.new_model({
* model_name: 'WidgetModel',
* widget_class: 'ipywidgets.IntSlider'
* })
Expand All @@ -320,19 +336,19 @@ define([
* widget_class: (optional) string
* Target name of the widget in the back-end.
* comm: (optional) Comm
* Comm object associated with the widget.
* model_id: (optional) string
* If not provided, the comm id is used.
*
* Create a comm if it wasn't provided.
* Either a comm or a model_id must be provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that it makes sense to require a comm or model id. Thinking symmetrically, you don't need to specify a comm or model id when instantiating a widget from the backend.

If not in this method, it makes sense to have a method that can do this, for the case when the user just wants to make a new widget, and doesn't care about comms or model ids.

I do not object to doing this in another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just renaming and keeping create_model. I do think having both create_model and new_model would be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

create_model does not need a comm or a comm_id.

new_model is meant for

  • creating models for a given comm (in the context of persistence)
  • creating comm-less models (for dead comm widgets with broken links)
    In the latter case, a model id is required.

*/
var comm = options.comm;
if (!comm) {
comm = this.comm_manager.new_comm('ipython.widget', {'widget_class': options.widget_class});
}

var that = this;
var model_id = comm.comm_id;
var model_promise = utils.load_class(options.model_name, options.model_module, WidgetManager._model_types)
var model_id = options.model_id || options.comm.comm_id;
var model_promise = utils.load_class(options.model_name,
options.model_module,
WidgetManager._model_types)
.then(function(ModelType) {
var widget_model = new ModelType(that, model_id, comm);
var widget_model = new ModelType(that, model_id, options.comm);
widget_model.once('comm:close', function () {
delete that._models[model_id];
});
Expand Down Expand Up @@ -423,30 +439,34 @@ define([
return this._get_connected_kernel().then(function(kernel) {

// Recreate all the widget models for the given notebook state.
var all_models = Promise.all(_.map(Object.keys(state), function (model_id) {
// Recreate a comm using the widget's model id (model_id == comm_id).
var new_comm = new comm.Comm(kernel.widget_manager.comm_target_name, model_id);
kernel.comm_manager.register_comm(new_comm);

// Create the model using the recreated comm. When the model is
// created we don't know yet if the comm is valid so set_comm_live
// false. Once we receive the first state push from the back-end
// we know the comm is alive.
return kernel.widget_manager.create_model({
comm: new_comm,
model_name: state[model_id].model_name,
model_module: state[model_id].model_module,
}).then(function(model) {
model.set_comm_live(false);
return model._deserialize_state(state[model.id].state).then(function(state) {
model.set_state(state);
return model.request_state().then(function() {
model.set_comm_live(true);
return model;
var all_models = that._get_comm_info(kernel).then(function(live_comms) {
return Promise.all(_.map(Object.keys(state), function (model_id) {
if (live_comms.hasOwnProperty(model_id)) { // live comm
var new_comm = new comm.Comm(kernel.widget_manager.comm_target_name, model_id);
kernel.comm_manager.register_comm(new_comm);
return kernel.widget_manager.new_model({
comm: new_comm,
model_name: state[model_id].model_name,
model_module: state[model_id].model_module,
}).then(function(model) {
return model.request_state().then(function() {
return model;
});
});
});
});
}, this));
} else { // dead comm
return kernel.widget_manager.new_model({
model_id: model_id,
model_name: state[model_id].model_name,
model_module: state[model_id].model_module,
}).then(function(model) {
return model._deserialize_state(state[model_id].state).then(function(state) {
model.set_state(state);
return model;
});
});
}
}));
});

// Display all the views
return all_models.then(function(models) {
Expand Down Expand Up @@ -480,6 +500,35 @@ define([
});
};

WidgetManager.prototype._get_comm_info = function(kernel) {
/**
* Gets a promise for the open comms in the backend
*/

// Version using the comm_list_[request/reply] shell message.
/*var that = this;
return new Promise(function(resolve, reject) {
kernel.comm_info(function(msg) {
resolve(msg['content']['comms']);
});
});*/

// Workaround for absence of comm_list_[request/reply] shell message.
// Create a new widget that gives the comm list and commits suicide.
var that = this;
return new Promise(function(resolve, reject) {
var comm = that.comm_manager.new_comm('ipython.widget',
{'widget_class': 'ipywidgets.CommInfo'},
'comm_info');
comm.on_msg(function(msg) {
var data = msg.content.data;
if (data.content && data.method === 'custom') {
resolve(data.content.comms);
}
});
});
};

// Backwards compatibility.
IPython.WidgetManager = WidgetManager;

Expand Down