Repository navigation
Move required_interactive_framework to canvas class. - #14521
ivanov merged 1 commit into
Conversation
Sorry, something went wrong.
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.
|
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+ ... |
Sorry, something went wrong.
|
Assuming we want to make it easier to define 3rd-party backends, requiring to have it only in a single place is clearly better... |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
tacaswell
left a comment
There was a problem hiding this comment.
I have reservations about removing required_interactive_framework at the module level. Enough to not "approve", but not enough to block merging.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I am convinced that this is likely not actually used and safe to pull the band-aid off. |
Sorry, something went wrong.
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