-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
API: Add entrypoint for plotting #27488
API: Add entrypoint for plotting #27488
Conversation
Libraries, including pandas, register backends via entrypoints. xref pandas-dev#26747
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.
Couple of comments.
this seems like a very complex solution why is not the simpler plotting.backend = ‘altair.pandas_plot’ not much more straightforward here? eg this is a standard import as passed to importlib |
We want to decouple the backend name ( |
and how does my suggestion not do this is? it is fully decoupled |
I guess I don't understand your suggestion then. You say
which seems to require a top-level |
not at all |
The reason I proposed this is because I believe it's cleaner for the implementation's code path to be separate from how the user specifies it. My goal would be for a user to write
and for this to cause the plotting backend to use Altair. Before this solution, that would require me to pollute the Altair package's top-level namespace with APIs that are irrelevant to users of the package. I think entrypoints is a clean solution to that problem (decoupling of specification from code paths), and one that is in common use among packages in the jupyter ecosystem. |
I just want to avoid a situation where users have to remember, "for Matplotlib use Seems like it would lead to more confusion than necessary. |
Thanks @jakevdp. Agreed entirely. |
ok I see your point @jakevdp and I agree that is nice to de-couple the user supplied key to plotting.backend, but then again it does couple the api of the package to pandas directly, meaning we then need to make an update in pandas to actually use a new package / api. So this is nice, I would then also allow a fully qualified module.function call to succeed |
I may be misunderstanding, but I don't see how the change to use entrypoints affects that. If you're saying that we'll need to update pandas so that Basically, pandas adds an EntryPoint "group" in our setup.py. And third party libraries add items to that group in their setup.pys. Pandas and the library don't need to talk to each other directly to register a backend. We talk through
We do allow that (see |
ok so if this works then that's great. +1. |
Yep, this should work out just fine. |
@datapythonista ok on this? |
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.
lgtm, just couple of small things in the docstring.
Thanks, fixed. |
thanks @TomAugspurger |
|
||
|
||
@td.skip_if_no_mpl | ||
def test_register_entrypoint(): |
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.
this is failing for me locally
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.
What are you seeing?
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.
Oh, I'm guessing it's a KeyError: 'pandas_plotting_backends'
?
You'll need to re-run python -m pip install -e .
in your pandas directory. This adds the entrypoint.
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.
Yep that does fix it, thanks
Libraries, including pandas, register backends via entrypoints.
xref #26747
cc @datapythonista @jakevdp @philippjfr