Skip to content

Move required_interactive_framework to canvas class. #14521

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

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jun 10, 2019

In mpl 3.0, the required_interactive_framework attribute was added to
backend modules. However, it is sometimes required to access it from a
canvas instance (e.g. in _fix_ipython_backend2gui), but the pattern
sys.modules[cls.__module__].required_interactive_framework is brittle
(e.g. it is broken by third-party subclasses).

Instead it makes more sense to put the attribute on the canvas class;
one can easily and robustly go from the backend module to the canvas
class by accessing the FigureCanvas attribute.

xref https://gitter.im/matplotlib/matplotlib?at=5ced522cef853135c83ae7d6, #14426.

Milestoned as 3.2 as the API is still quite new so the earlier we fix it, the better.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

In mpl 3.0, the required_interactive_framework attribute was added to
backend modules.  However, it is sometimes required to access it from a
canvas *instance* (e.g. in _fix_ipython_backend2gui), but the pattern
`sys.modules[cls.__module__].required_interactive_framework` is brittle
(e.g. it is broken by third-party subclasses).

Instead it makes more sense to put the attribute on the canvas class;
one can easily and robustly go from the backend module to the canvas
class by accessing the FigureCanvas attribute.
@anntzer anntzer added this to the v3.2.0 milestone Jun 12, 2019
@tacaswell
Copy link
Member

Should we leave it in both places? That doesn't break any API but it does risk them getting out of sync. Other than than 👍.

Unfortunately, module level __getattr__ is py37+ ...

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 15, 2019
@anntzer
Copy link
Contributor Author

anntzer commented Jun 15, 2019

Assuming we want to make it easier to define 3rd-party backends, requiring to have it only in a single place is clearly better...

@tacaswell
Copy link
Member

Do we have any guess on how many people we are going to break with this? Trying to search for the use of this, it seems a zillion people have put up repos which include their virtual environments (https://github.com/search?q=required_interactive_framework&type=Code from https://github.com/search?l=Python&q=class+_BackendQT4Agg%28_BackendQT5Agg%29%3A&type=Code it looks like there are 3k+ vendored versions of mpl on github that is .... something...).

The problem with this is API change is that there is (currently) no clean way to warn it and it is an immediate AttributeError. I don't think it is worth the effort of making this even more complicated by sub-classing Module, so for now just export it from the Canvas to the module and in the future use module __getattr__ to either make it a mirror or deprecate it.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I have reservations about removing required_interactive_framework at the module level. Enough to not "approve", but not enough to block merging.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 15, 2019

required_interactive_framework is very recent: it was only added in 3.0 (in #11520), so September 2018. Most likely, it is only used by people writing cross-backend code, so likely libraries; but how many people are there writing libraries that already require mpl3.0+? It is also quite difficult to use correctly right now (as seen by the dance with cls.__module__ that this removes, which does not even handle subclasses correctly).

Currently you have milestoned 3.2 for September 2019, so let's assume it will come out at some point before the end of the year. Py3.6 was released late december 2019 so 3.2 will almost certainly still support it, so we wouldn't be able to put in a proper deprecation before mpl3.3 at the earliest (say by mid-to-early 2020), which means the attribute will have another 6-9 months to pick up more users who will be affected by its removal.

In other words the choice is between affecting a smaller number of users right now, and a potentially larger number next year.

@tacaswell
Copy link
Member

I am convinced that this is likely not actually used and safe to pull the band-aid off.

@ivanov ivanov merged commit 8fad359 into matplotlib:master Aug 19, 2019
@anntzer anntzer deleted the rif branch August 19, 2019 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants