-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Plugin system for backends #19482
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
Plugin system for backends #19482
Conversation
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers
or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Can you clarify this statement (i.e. the goal of this PR)? Currently you can already make any module a custom backend (which is then selectable using the |
@anntzer sorry, I realize my PR comment is indeed missing some context. This is hopefully the first PR of a bunch of PRs that would clean up the IPython-Matplotlib interface. Currently, IPython provides a This PR tries to make this more modular, by enforcing the backends to provide a manifest that advertises the backend name, module, and if it's compatible with IPython (interactive). This way IPython and Matplotlib can dynamically discover available backends without importing them. Ideally, this This comes after a discussion that happened with @tacaswell and @SylvainCorlay something like a year ago, where we discussed improving this logic. |
Maybe @Carreau would have some ideas on this as well. |
No worries :)
That's a worthwhile goal too :)
But that could fully be solved on IPython's side, no? IPython could just support
Actually this is possible, see https://github.com/matplotlib/mplcairo/blob/master/lib/mplcairo/_util.py#L15. (I'm not saying patching backend2gui is particularly nice, but it's possible, and a nicer but backcompatible API could be developed around that.)
Per (*) I believe reusing module names as backend names is simple and sufficient enough. For example, you don't have to worry about two different modules claiming to provide the same backend name. A priori the metadata for interactivity is the
The importance of avoiding import is unclear to me (but I may be missing something).
Agreed. |
Thank your for your detailed answer :)
But my point is that it should be up to the backend to provide its name and the path to the Python module containing its implementation. It should not be IPython's job. It's a separation of concerns issue. It seems to me that it should be up to Matplotlib to have a way to provide a list of available backends. And it should not be hardcoded anywhere, not in IPython, not in Matplotlib.
This works indeed. But it's only a short-term solution. Also, it doesn't seem to me that
Thanks! I missed that :)
This is just a good thing to have IMO, to avoid importing unused libraries. If we can find a way to avoid importing all available backends it's better. |
My point is that the backend name should exactly be the module name (with the exception of the builtin backends (and possibly the ipympl backends), which are grandfathered). See
Why do you need to have this list available?
OK, but AFAICT IPython only needs this dict because it doesn't know to read the
But why do you need to list them at all? The user says |
I understand. I honestly don't mind if they are not named. Doesn't it seem more user-friendly that backend can provide a name? Just for convenience. Why do you think the backend name should be the module name?
I see. But don't you think inserting to
Fair point 👍🏼 |
I maintain the mplcairo backends (https://github.com/matplotlib/mplcairo) and
It doesn't really have to be a module, we could just support |
My take on this is that we cannot change the fact that there are hundreds of thousands of notebooks out there that make use of the matplotlib magic
and should we make any change to how the matplotlib magics maps to backends, we have to make sure that we don't break these notebooks. In my mind, What I think this PR brings is for the backends to provide their own abbreviation if any, so that the mapping from abbreviation to module name is maintained in the same place as the module name. |
Again, I believe that the already existing list of abbreviations should be grandfathered for backcompat purposes (if you don't want to hardcode the mapping in multiple places, we can reasonably add it to matplotlib as an (immutable) mapping), but that we should not support adding more abbreviations, and instead make people use module names instead. We don't have a metadata/plugin system to allow people to write To be more precise, if people want to be able to write |
We talked about this on the mpl dev call and came to the consensus that:
We already have a mechanism for the backends to report what GUI framework they need and if we discover that IPython is already imported reach back and set up the event loop integration so I think I think the above is non-controversial, can be done quickly, and is a big step forward. Below is my thoughts, not consensus from the call. I can see two main upsides of auto-discovery: being able to list available backends and backends being able to provide an alias/metadata. That said, I could be talked into either or both of those being valuable enough to be worth implementing auto-discovery, but I would prefer if we did it through I think the |
Clarification w.r.t. "%matplotlib unknown.module gets up-converted to
module://unknown.module".
Does this mean that matplotlib could end up trying to import something as a
last resort? Let's say they mistyped nbagg as nbag? Will matplotlib then
end up trying to import nbag.py somewhere?
…On Thu, Feb 25, 2021 at 6:15 PM Thomas A Caswell ***@***.***> wrote:
We talked about this on the mpl dev call and came to the consensus that:
- under no condition should we change something that breaks existing %matplotlrib
XYZ behavior because there are too many notebooks and too much muscle
memory for that to be plausible
- the hard coded list of abbreviations -> modules should move from
IPython to Matplotlib but the magic should still probably live in
ipykernal. IPython can then either conditionally import the mapping back,
or if mpl now owns it, we can handle the re-mapping in switch_backend.
- we should adopt @anntzer <https://github.com/anntzer> 's suggestion
that %matplotlib unknown.module gets up-converted to
module://unknown.module and processed as such (we can do that on the
mpl side so that mean). I think this means that IPython will be able to
blindly pass the backend through to pyplot.switch_backend and let mpl
handle all of the normalization / aliasing / exception and I *think*
gui fallback.
We already have a mechanism for the backends to report what GUI framework
they need and if we discover that IPython is already imported reach back
and set up the event loop integration so I think backend2gui can be fully
removed!
I think the above is non-controversial, can be done quickly, and is a big
step forward.
------------------------------
Below is my thoughts, not consensus from the call.
I can see two main upsides of auto-discovery: being able to list available
backends and backends being able to provide an alias/metadata. %matplotlib
--list currently exists, but we could move that to the current static
list and add "If you have any third-party backends installed initialize %matplotlib
module.name" or similar. I agree with @anntzer
<https://github.com/anntzer> that adding aliases here is a bit
superfluous if we stop requiring module:// .
That said, I could be talked into either or both of those being valuable
enough to be worth implementing auto-discovery, but I would prefer if we
did it through entrypoints and I think can be handled separately from the
points above.
------------------------------
I think the %matplotlib magic has to continue to live in IPython as it
needs to be available to users without any imports and our import time is
bad enough you really do not want a unconditional import of even our
top-level to make make it available.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#19482 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHF6EG3MMH5OHSHEKMCMDTA3K25ANCNFSM4XJMERIA>
.
|
Would that be a problem? People can also mistype regular imports. If you have nbag.py on your system it should be safe to import. We'd just raise afterwards because it does not provide a backend. If you have files that are dangerous to import, you likely have bigger issues. There are various tools that imports lots of stuff (AFAIR e.g. Sphinx, pytest, ...) |
There is a clear difference for an end-user between mistyping an `import
nbag` and mistyping `%matplotlib nbag`. The latter is not obvious to the
end-user that an import of that name could end up happening, especially
given that nominal behavior doesn't result in that.
Note, I don't think this is a red line for me, I was only asking for
clarification that I was interpreting the proposal correctly. And yes,
other tools do import stuff, of course, but I think your examples are
missing the point that an ipython magic (or even a matplotlib.use())
doesn't advertise itself as an arbitrary import, and hasn't done so in the
past, while those tools make it explicit and nominal behavior that it is
importing things based on user inputs. Meanwhile, specifying
`module://foo.bar` is fairly clear that we want to import that module (at
least for the class of users that would utilize that feature).
I think we need to think carefully about whether or not this is what we
want. Indeed, it is probably "safe enough" given that it isn't going to
trigger an import of a remote resource. But, even so, people have pointed
out the dangers of typos in python before:
https://snyk.io/blog/malicious-packages-found-to-be-typo-squatting-in-pypi/
…On Fri, Feb 26, 2021 at 11:47 AM Tim Hoffmann ***@***.***> wrote:
Would that be a problem? People can also mistype regular imports. If you
have nbag.py on your system it should be safe to import. We'd just raise
afterwards because it does not provide a backend. If you have files that
are dangerous to import, you likely have bigger issues. There are various
tools that imports lots of stuff (AFAIR e.g. Sphinx, pytest, ...)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#19482 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACHF6G5XYIRJRIWWIE6VYDTA7GD3ANCNFSM4XJMERIA>
.
|
I agree with @timhoffm that if you have On another point, assuming that the end goal is to have the implementation of (Of course we can't break backcompat so still need to support a backend being passed to |
The main argument I see against keeping an hard coded list somewhere is the ipympl example: we are currently stuck by IPython on the naming of the ipympl backend module (which was named Moving this hard coded list from IPython to Matplotlib would, unfortunately, not make this kind of issues easier. I would find it nicer/more sensible if it's the backend itself that advertises its alias, as mentioned by @tacaswell.
I totally agree on this. I can try to come up with a PR adding such logic of discovering the entrypoints if you want. |
I don't really understand what (if anything) changed here, compared to the previous discussion: numpy, for example, is stuck forever on having |
NumPy could definitely decide to deprecate some modules and show a deprecation warning saying that those modules would be removed in a coming major release. The problem here with the hard-coding of the aliases in IPython is that if ipympl wants to rename its module (which it should), then IPython will need to figure out the ipympl version that is installed in order to know which module the alias points to. This seems like an unreasonable situation. In my opinion, IPython should not even know what ipympl is, it does not depend on it in any way, note that it doesn't even depend on Matplotlib. Having entry points would fix this, because ipympl would advertise to others which submodule contains the backend. ipympl would be the source of truth.
Why not? Being able to introduce new aliases without having to change IPython's code would actually be nice :) Being able to do |
But then that's not different from doing a deprecation dance on a backend name to reintroduce it under another backend name, and have users to try... catch around that (well, I don't know if you can wrap
As stated previously, I am totally OK with moving the current alias list from ipython to matplotlib, so ipython would not need to know about ipympl.
Because this breaks the simple modulename<->backendname equivalence, and I remain unconvinced that the benefits justify adding that machinery and the code that goes with it.
As stated previously, I (and @tacaswell too) am OK with supporting the spelling `%matplotlib foobar.path.to.backend, and then it's just up to you how deep you want to nest your submodule... |
The hardcoded list "grandfathered" into Matplotlib suggested by @anntzer is not a practical solution because several of the backends listed in there are not core to Matplotlib (the inline and widget backends). A hardcoded list would lead to the same "locking" problem as we have now with respect to changing the package name in e.g. ipympl. The approach proposed here by @martinRenou is the most practical in my opinion. |
ok, I think a summary of the points here are:
Hopefully everyone agrees these statements are true? My proposal now is very similar to my one above:
Hopefully this gets everyone what they want?
I think there is some legitimate concern about "why do both?!", but we already support both and can not back out of either. If we are going to support aliases, we should make the pluggable. I am having some hesitation about the implicit import via un-prefixed strings. There is a bit of @WeatherGod 's concern of surprise imports (but with aliases that also will import lots of things), but I am more concerned about the ambiguity of if you will get the module you named or some very differently named module which has claimed that name. I also think there is a non-zero risk of having an alias (because that is how you want to roll) and then some un-related package installs with that name and now we try (and fail) to import that |
Can you clarify the following point?
I read that as The rest looks good. |
If I recall correctly in the implementation of a magic it gets access to the input as strings as well as access to the In []: import my_backend as foo
in []: %matplotlib foo This would be magic-side logic, I very much agree that |
But do you actually want
to work? I guess I don't have an overly strong opinion of the API on IPython's side, but that doesn't look too great to me? I think we should have
and as a convenience
which maps to
|
I was thinking how to bring |
My immediate reaction is that I don't really like it, but I don't feel that strongly about it either right now. |
IPython magic is not python code. I don't think we should try and make it look like it. You also cannot do:
|
OK, I'm sold on this position. |
Actually, one point I did miss about the alias system: I guess this means that now |
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
Closing this as there will be a different implementation, see issue #27663. |
PR Summary
This PR adds a plugin system for backends. Now backends need to install a manifest under
$PREFIX/share/matplotlib/backends
containing the name of the backend and the Python module containing this backend:This will make it easier to create custom backends, by simply installing the right file in the right place for Matplotlib to discover.
I am opening this PR for heads-up and for starting discussions, I might still change the implementation.
Note 1: This breaks the inline and the ipympl backend in IPython, because they currently do not install the files.
Note 2: This could be the opportunity to take some core backends out of the Matplotlib code base, in their own packages?
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).