-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Refactor backend loading #9551
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
Refactor backend loading #9551
Conversation
81d7c52
to
c1dfd56
Compare
c1dfd56
to
54e3169
Compare
54e3169
to
439338d
Compare
439338d
to
e45d8b0
Compare
@@ -3362,3 +3250,129 @@ def set_message(self, s): | |||
Message text | |||
""" | |||
pass | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this block just a big copy/paste? (so we don't have to bother reviewing the code if it is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, except for the fact that the FigureManager attribute now defaults to FigureManagerBase.
33fb09d
to
56858ae
Compare
56858ae
to
c6e21f4
Compare
c6e21f4
to
a16e3ec
Compare
a16e3ec
to
2c73aba
Compare
2c73aba
to
181cb70
Compare
lib/matplotlib/backends/__init__.py
Outdated
backend_name = (name[9:] if name.startswith("module://") | ||
else "matplotlib.backends.backend_{}".format(name.lower())) | ||
backend_mod = importlib.import_module(backend_name) | ||
Backend = type("Backend", (_Backend,), vars(backend_mod)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining this bit of magic? I get that it returns a base _Backend class, but am not quite sure what vars(backend_mod)
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vars(obj) returns the attributes of obj as a dict. In the case of a module, that's basically the module's contents.
So effectively it's as if I'm running all of the module's code (of course, not a second time) from within a class Backend(_Backend): ...
context, and refetching attributes from that class. The point being that the base _Backend class can provide default implementations of backend_version, show, draw_if_interactive through inheritance rather than pylab_setup having to do it manually. Will add a comment to that effect, but does that sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, not objecting to the magic at all, but I think a short comment to help those of us with lesser python skills know whats going on would help make sure the code stays maintained...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comment.
# Create a local Backend class whose body corresponds to the contents of | ||
# the backend module. This allows the Backend class to fill in the missing | ||
# methods through inheritance. | ||
Backend = type("Backend", (_Backend,), vars(backend_mod)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we create the class, use the inheritance to fill in the missing functions and then return class level functions (which 'forget' they were part of the class)?
That is quite clever.
To check, if a backend is already using @_Backend.export
then there should be nothing extra we need to provide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, backends that were using @_Backend.export
don't really need that anymore (the contents of the local _Backend class could just be dumped at the module level (or alternatively they could just do without the temporary creation of a new Backend class))... except for one thing: the _Backend class still allows inheritance between backends, so that e.g. _BackendQt5Agg can inherit from _BackendQt5.
@@ -1699,8 +1699,7 @@ def pstoeps(tmpfile, bbox=None, rotated=False): | |||
shutil.move(epsfile, tmpfile) | |||
|
|||
|
|||
class FigureManagerPS(FigureManagerBase): | |||
pass | |||
FigureManagerPS = FigureManagerBase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a very subtle API change in here if people are looking at __name__
, __module__
, or __file__
of these classes. I am not sure if that is worth an API change note or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an API note. The real intent here is the following:
I think the "correct" way to refer to a backend's FigureCanvas, FigureManager, etc. class is under the unsuffixed names (FigureCanvas instead of FigureCanvasPS, etc.) The reason is that 1) these unsuffixed names need to be defined anyways for the backend machinery to pick them up, and 2) some of these classes (FigureManager, Toolbar, though not FigureCanvas of course) can be shared between multiple backends (for example, the xxxcairo backends and mplcairo just reuses the FigureManager classes for each of the GUI toolkits).
So sure, I can define (e.g.) FigureManagerQt5 and FigureManagerQt5Agg and FigureManagerQt5Cairo and FigureManagerMplCairoQt to all be aliases (or trivial subclasses) of one another, but that's just muddying the picture IMO; promoting the use of the same FigureManager name everywhere seems clearer.
7b8b41b
to
36fab4c
Compare
This needs a rebase. @anntzer Please self-merge once CI passes. |
Self-merging per #9551 (comment). |
This is preliminary work towards fixing the runtime detection of default
backend problem (the first step being to make it easier to check whether
a backend can be loaded). No publically visible API is changed.
Backend loading now defines default versions of
backend_version
,draw_if_interactive
, andshow
using the same inheritance strategy asbuiltin backends do.
The
_Backend
class had to be moved to the end of the module asFigureManagerBase
needs to be defined first. TheShowBase
class hadto be moved even after that as it depends on the
_Backend
class.PR Summary
PR Checklist