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

Fix FigureWidget attribute error on wildcard import with ipywidgets not installed #2445

Merged
merged 2 commits into from
May 7, 2020

Conversation

jonmmease
Copy link
Contributor

Fixes the regression in 4.7.0 reported in #2443. Also takes care of #1111 by producing a more informative error message.

The problem is that with Python 3.7+, we add "FigureWidget" to the __all__ list in the plotly.graph_objs/plotly.graph_objects modules regardless of whether ipywidgets is installed. But when FigureWidget is lazily imported, an exception is raised if a supported version of ipywidgets is not installed. This results in the reported error when a user, or library, calls from plotly.graph_objs import *.

One option would be to check for the proper version of ipywidgets when plotly.graph_objs is imported and only add FigureWidget to __all__ if it is found. But this results in a performance hit on every import of plotly.graph_objs, whether or not FigureWidget is used.

Another option would be to always allow FigureWidget to be imported, but raise an exception when a user tries to construct a FigureWidget if ipywidgets is not found. Unfortunately, this isn't possible because ipywidgets is required in order to define the FigureWidget class itself so we can't import it without ipywidgets.

Instead, this PR introduces a new plotly.missing_ipywidgets.FigureWidget class. This class is a subclass of BaseFigure (just like the standard Figure), but all it does is raise an informative exception in the constructor informing the user that they need to install ipywidgets in order to use FigureWidget. With this approach, "FigureWidget" is still always added to __all__, but on lazy import we check whether ipywidgets is installed. If it is, the current plotly.graph_objs._figurewidget.FigureWidget class is returned, otherwise the new plotly.missing_ipywidgets.FigureWidget class is returned.

Tests added in plotly/tests/test_core/test_figure_widget_backend/test_missing_ipywigets.py.

Code PR

  • I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • I have added tests (if submitting a new feature or correcting a bug) or
    modified existing tests.
  • I have added a CHANGELOG entry if fixing/changing/adding anything substantial.

jonmmease added 2 commits May 7, 2020 07:20
Introduces a dummy `plotly.missing_ipywidgets.FigureWidget` class that is imported as
plotly.graph_obj(ect)?s when ipywidgets is not installed with a supported version. This
class is a BaseFigure subclass that raises an ImportError in the constructor.
@jonmmease jonmmease added the bug something broken label May 7, 2020
@jonmmease jonmmease added this to the 4.7.1 milestone May 7, 2020
from .basedatatypes import BaseFigure


class FigureWidget(BaseFigure):
Copy link
Contributor

Choose a reason for hiding this comment

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

why even bother subclassing BaseFigure here? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed safer to me for it to still pass isinstance(fig, BaseFigure) checks, but it probably doesn't make a difference.

@nicolaskruchten
Copy link
Contributor

💃

@jonmmease jonmmease merged commit ecb635a into master May 7, 2020
@cliffckerr
Copy link

Awesome, thank you for addressing this so quickly @jonmmease !

@leo-smi
Copy link

leo-smi commented Jan 2, 2022

the error still occur #2443 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants