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

Add entry points for viewer registry #411

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

Conversation

jfoster17
Copy link
Member

Populate viewer registry for all viewers

Description

This uses entry points to register all the viewers for glue-jupyter into viewer_registry. This addresses #388 and makes all viewers accessible through the new_data_viewer() command in glue-jupyter. I think this should be compatible with #402, because the viewer registry won't double-populate the same viewers. I have added IpyvolumeScatterView as scatter3d.

Adding an entry point for glue-wwt will be a separate PR in that repository.

Not 100% about the names here. 'image' is a convenient name for calling in code, but is less descriptive than something like '2d image'. This is particularly relevant for 'scatter' (unqualified) versus 'scatter3d'.

Copy link
Collaborator

@maartenbreddels maartenbreddels left a comment

Choose a reason for hiding this comment

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

Nice, i was missing this in glue-solara!

Comment on lines 5 to 6
def setup():
from viewer import BqplotHistogramView # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed, since * is already imported

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I think I got confused because I forgot that you have to manually rerun pip install to rebuild the entry points when I was developing this. And yes, this means we can simplify glue-solara to just build a list of viewers from the registry.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Good idea to define entry points but some comments below on implementation.



def setup():
pass
Copy link
Member

Choose a reason for hiding this comment

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

Ideally the setup function should actually be adding the viewer to the registry (rather than using the decorator) - @CyclingNinja is working on improvements to load_plugins that means that users will be able to opt-in to specific plugins, so we need the viewer to not be added if the entry point is not called.

histogram = glue_jupyter.bqplot.histogram:setup
image = glue_jupyter.bqplot.image:setup
profile = glue_jupyter.bqplot.profile:setup
scatter = glue_jupyter.bqplot.scatter:setup
Copy link
Member

Choose a reason for hiding this comment

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

These plugin names might conflict with the ones from glue-core - the entry point names should be unique across the glue ecosystem.

@@ -7,6 +9,7 @@
__all__ = ['IpyvolumeScatterView']


@viewer_registry("scatter3d")
Copy link
Member

Choose a reason for hiding this comment

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

The registration of the viewer should be done in setup as mentioned above.

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.

4 participants