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

Creating a widget registry on the Python side. #6493

Merged
merged 4 commits into from Oct 30, 2014

Conversation

SylvainCorlay
Copy link
Member

Widgets classes are registered on definition of the class using a decorator called register.

@register('custom.Baz')
class Baz(Widget)
    #   ....

When not specifying any argument, the view name is used as a key.

@register()
class Baz(Widget)
    #   ....

The motivation is twofold:
1- Creation of Widgets from the JavaScript side
2- Graphical gui building interface, that needs to know about a registry of widgets.

For the latter case rather than using a registry in the form of a key: widget_class dictionary as now, we could have some other metadata store with it:

key: {
    class: widget_class, 
    metadata: {}
}

The metadata could be used for categorization of widget (control, visualization, container) etc...

@SylvainCorlay
Copy link
Member Author

@jdfreder @jasongrout

@SylvainCorlay
Copy link
Member Author

Rebased - Now the default key is the fully qualified python class name. Core Jupyter widgets all have custom keys, which are not Python-specific and can be used from the frontend with other backends than Python ones.

@SylvainCorlay
Copy link
Member Author

Another possible implementation would be to have something like

+def register(widget):
    """Registers a widget class in the widget registry. 
    The key in the registry is the widget attribute `_backend_name` provided for each core 
    Jupyter widget so that the frontend can use this key regardless of the language of the kernel
    If no backend name is provided, the full class name is used as the key.
    """
    name = widget._backend_name.default_value
    key = name  if name is not None else widget.__module__ + widget.__name__
    Widget.widget_types[key] = widget
    return widget

Then widgets are potentially declared with one more attribute _backend_name.

@@ -43,14 +43,17 @@ def _validate(self, name, old, new):
self.value = min(max(new, self.min), self.max)


@register('jupyter.FloatText')
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why namespace the Widget names under Jupyter instead of no namespace at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Namespaces are one honking great idea -- let's do more of those :)
Just that a name like "Box" is really generic, and should not be reserved in the general namespace.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha , I agree Jupyter is the right name, since the backend could be arbitrary. @ellisonbg do you think it's okay to introduce the name Jupyter into the IPython repository in this context?

Copy link
Member

Choose a reason for hiding this comment

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

Not yet

On Thu, Sep 25, 2014 at 1:10 PM, Jonathan Frederic <notifications@github.com

wrote:

In IPython/html/widgets/widget_float.py:

@@ -43,14 +43,17 @@ def _validate(self, name, old, new):
self.value = min(max(new, self.min), self.max)

+@register('jupyter.FloatText')

Gotcha , I agree Jupyter is the right name, since the backend could be
arbitrary. @ellisonbg https://github.com/ellisonbg do you think it's
okay to introduce the name Jupyter into the IPython repository in this
context?


Reply to this email directly or view it on GitHub
https://github.com/ipython/ipython/pull/6493/files#r18056451.

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Copy link
Member

Choose a reason for hiding this comment

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

So maybe it's best to either use IPython as the namespace, or no namespace at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. What do you prefer between the two proposals? Adding a _backend_name synced attribute or providing it in the decorator?

Copy link
Member

Choose a reason for hiding this comment

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

By 'providing it in the decorator', do you mean hardcoding it into the decorator function? If so, I think just replacing jupyter with ipython would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant that besides the discussion on the namespace, I proposed two ways of defining the key for the registry. The first is the one implemented in the PR and the second one is the one proposed in the first comment.

@SylvainCorlay
Copy link
Member Author

Renamed the namespace from jupyter to IPython as discusses with @ellisonbg.

@jdfreder
Copy link
Member

This looks good @SylvainCorlay , I'm 👍 to merge this. Did @ellisonbg look at it in person?

@ellisonbg
Copy link
Member

No I haven't looked at the code

On Tue, Oct 14, 2014 at 11:26 AM, Jonathan Frederic <
notifications@github.com> wrote:

This looks good @SylvainCorlay https://github.com/SylvainCorlay , I'm [image:
👍] to merge this. Did @ellisonbg https://github.com/ellisonbg look
at it in person?


Reply to this email directly or view it on GitHub
#6493 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@jdfreder
Copy link
Member

@ellisonbg did you want to review this before we merge it? It's related to #6664 - instantiating widgets from Javascript.

@ellisonbg
Copy link
Member

No don't wait for me on this one, as long as others have looked at it.

On Tue, Oct 14, 2014 at 2:13 PM, Jonathan Frederic <notifications@github.com

wrote:

@ellisonbg https://github.com/ellisonbg did you want to review this
before we merge it? It's related to #6664
#6664 - instantiating widgets
from Javascript.


Reply to this email directly or view it on GitHub
#6493 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@jdfreder
Copy link
Member

@minrk @Carreau or @takluyver care to take a look?

@minrk
Copy link
Member

minrk commented Oct 14, 2014

I thought part of the goal of using require to load widgets was to remove the need for a widget registry on the javascript side. Do we really want to add one on the Python side, then? Or am I confused about something?

@jdfreder
Copy link
Member

I don't think you're confused at all- #6664 allows widgets to be instantiated from the front-end without the need for this registry. Instead it uses the Python namespace to load the appropriate widget. Like the Javascript counterpart, the registry would be checked first and then, if not found, the namespace would be checked.

@SylvainCorlay
Copy link
Member Author

it is a way to refer to the backend implementation without specifying a language-specific path. (like the current front-end registry does for the js implementation of core widgets)

provided for each core IPython widget so that the frontend can use
this key regardless of the language of the kernel"""
def wrap(widget):
l = key if key is not None else widget.__module__ + widget.__name__
Copy link
Member

Choose a reason for hiding this comment

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

The implementation in #6664 will import a module and use the class from that, without needing it in the registry. Assuming we accept that (and I think we should - it nicely parallels loading widgets from require modules in the other direction), is there any use in the @register() decorator without an argument? It registers the class with a name that can be used to find it anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

When you pass an argument to the decorator, it gives a name that is independent of the backend language. The R and Julia kernels may have an alternative implementation of the Slider, Text, and all the other core widgets.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that makes sense - I'm asking what's the use for allowing @register() without specifying a name. You register it as modulename.Classname in that case, but #6664 will let modulename.Classname work with no need to register it, just by using it as an import name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, the other usecase for the registry is to have in one place all the widget classes that were loaded, the goal being to have a (very simple) gui builder.

@takluyver
Copy link
Member

This needs a rebase, by the way.

@SylvainCorlay
Copy link
Member Author

Rebased.

@jdfreder
Copy link
Member

2- Graphical gui building interface, that needs to know about a registry of widgets.

I thought I had comment somewhere, but I don't see it anymore, that alternatively you could walk the IPython widget namespace instead of registering all of the IPython widgets with the global registry as done here. That way we don't have to add the decorator above every class. @SylvainCorlay @takluyver ?

@takluyver
Copy link
Member

I'd go with 'explicit is better than implicit' there.

@takluyver
Copy link
Member

... i.e. I prefer having the widget classes individually registered than having something separate from them that quietly registers them all.

@SylvainCorlay
Copy link
Member Author

An advantage of the decorator over walking through the module is that I can use the decorator with other widgets than the core ones, defined in various modules.
Besides, some widgets classes are pure implementation details and one may not want to register them.

@jdfreder
Copy link
Member

I'd go with 'explicit is better than implicit' there.

No, the thinking was that the core widgets don't need to be registered at all.

One of the arguments against not registering the core widgets was the ability enumerate the widgets. I was just saying we could enumerate through them via the namespace too.

An advantage of the decorator over walking through the module is that I can use the decorator with other widgets than the core ones, defined in various modules.

I wasn't suggesting removing the decorator, just removing the decorate applied to the IPython internal widgets.

Besides, some widgets classes are pure implementation details and one may not want to register them.

True, but one could argue that's a white or black listing (either would work) that should be implemented by whoever is interested in it.

@SylvainCorlay
Copy link
Member Author

Wouldn't it be weird to propose a decorator on the one side and to introspect the module in the case of the core widgets? Besides, you would still need to add some class-level information to set the key in the registry.

@jdfreder
Copy link
Member

Wouldn't it be weird to propose a decorator on the one side and to introspect the module in the case of the core widgets?

I wouldn't worry about that since we wouldn't be implementing the introspection anywhere in IPython itself (a widget builder isn't on the roadmap).

Although, an argument for the registry is the ability for the user to overwrite internal names. I think this helps IPython remain on-the-fly hack friendly, which IMO is Pythonic.

Overall I'm still okay with this being merged as-is. @takluyver do you want to review this anymore (I notice you didn't respond to @SylvainCorlay 's response to your code comment)?

@takluyver
Copy link
Member

I'm still not convinced that it's useful to put things in the registry if they can be found by import path anyway, but I don't care that much. Merging.

takluyver added a commit that referenced this pull request Oct 30, 2014
Creating a widget registry on the Python side.
@takluyver takluyver merged commit 3801ab3 into ipython:master Oct 30, 2014
@jdfreder
Copy link
Member

Thanks @SylvainCorlay !

@SylvainCorlay SylvainCorlay deleted the python_widget_registry branch October 30, 2014 23:49
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
…stry

Creating a widget registry on the Python side.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants