Skip to content
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

Move Matplotlib backend mapping to Matplotlib #14371

Merged
merged 8 commits into from Apr 12, 2024

Conversation

ianthomas23
Copy link
Contributor

@ianthomas23 ianthomas23 commented Mar 19, 2024

This is WIP to move IPython's backend mapping into Matplotlib and extends it to allow backends to register themselves via entry points. It is a coordinated effort across Matplotlib, IPython, matplotlib-inline and ipympl. Closes #14311. Most of the work is in Matplotlib (matplotlib/matplotlib#27948) but I will repeat the relevant points here.

When using a Matplotlib magic command of the form %matplotlib something the identification of the Matplotlib backend and GUI loop are now performed in Matplotlib not IPython. This supports:

  1. Matplotlib backends that IPython already supports such as qtagg and tkagg.
  2. Other built-in Matplotlib backends such as qtcairo.
  3. Backends that use module://something syntax such as module://mplcairo.qt.
  4. Backends that self-register using entry points, currently inline and widget/ipympl.

Implementation details:

  1. The magic command is now explicitly %matplotlib gui_or_backend rather than %matplotlib gui. This is already effectively the case as e.g. %matplotlib ipympl is really a backend not GUI name. Within Matplotlib the gui_or_backend is checked first to see if it is a GUI name and then falls back to checking if it is a backend name.
  2. If you select the inline backend the corresponding GUI is now None not inline. All backends which do not have a GUI loop return None, otherwise we have to keep explicit checks within IPython for particular backends.
  3. backends and backend2gui are now deprecated but are still exposed, with a deprecation warning, if required. If using Matplotlib, ipympl, etc releases that include the corresponding changes to this PR then they are not needed as Matplotlib deals with it all. But for backward compatibility they must still be available for a while along with the remainder of the existing backend-handling code.
  4. I haven't yet updated the documentation but we need to keep information about valid GUI frameworks and I propose that we should remove all lists of valid Matplotlib backends, replacing them with instructions on how to obtain the current list (pointing to the Matplotlib docs and using %matplotlib --list). If we keep any lists then they will inevitably become out of date. This extends to the backend_keys in IPython/core/shellapp.py.

Because the four related projects are loosely coupled without direct dependencies on each other (except for ipython and matplotlib-inline), backward compatibility requires all possible combinations of projects before and after the new functionality (I will call these "old" and "new" from now on) to continue to work. I have tested these all locally, and the CI of this PR will test new IPython against old Matplotlib for example, but I need to add one or more new temporary CI runs to test new IPython against new Matplotlib etc. The identification of new versus old depends on version checks on the other libraries, so here is a table that I will update showing the current status of progress in the 4 projects:

Project Relevant PRs Possible release version
matplotlib-inline ipython/matplotlib-inline#34, ipython/matplotlib-inline#35 0.1.7
ipympl matplotlib/ipympl#549 0.9.4
Matplotlib matplotlib/matplotlib#27948 3.9.1
IPython #14371 (this) 8.24.0

The two widget projects can be released soon, once we are happy with the entry point approach. The other two projects' PRs will have to be synchronised as each includes version checks on each other.

To do

  • Add CI runs against the new PR branches of the other projects.
  • Add comments for conditions required for backward-compatibility code blocks to be removed.
  • Update documentation, including removal of lists of valid backends.
  • Update version checks before merging.

@Carreau
Copy link
Member

Carreau commented Mar 31, 2024

Sorry I was a bit overwhelmed this month, I'll try to push this forward for next release.

@ianthomas23
Copy link
Contributor Author

Sorry I was a bit overwhelmed this month, I'll try to push this forward for next release.

That's OK, there are still things I need to do to this before it is reviewable. Maybe you should just ignore it until I confirm when it is ready for review.

@ianthomas23 ianthomas23 force-pushed the entry_points branch 2 times, most recently from f50f97b to b01464d Compare April 2, 2024 13:56
IPython/core/magics/pylab.py Show resolved Hide resolved
IPython/core/pylabtools.py Outdated Show resolved Hide resolved
# Deprecated attributes backends and backend2gui mostly following PEP 562.
def __getattr__(name):
if name in ("backends", "backend2gui"):
warnings.warn(f"{name} is deprecated", DeprecationWarning)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warnings.warn(f"{name} is deprecated", DeprecationWarning)
warnings.warn(f"{name} is deprecated since IPython 8.24, backends are managed in matplotlib and can be externally registered.", DeprecationWarning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -319,7 +333,23 @@ def find_gui_and_backend(gui=None, gui_select=None):

import matplotlib

has_unified_qt_backend = getattr(matplotlib, "__version_info__", (0, 0)) >= (3, 5)
if _matplotlib_manages_backends():
backend_registry = matplotlib.backends.registry.backend_registry
Copy link
Member

Choose a reason for hiding this comment

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

Does that need explicit import of matplotlib.backends ? and/or other hasattr checks ?

In [1]: import matplotlib

In [2]: matplotlib.backends
...
AttributeError: module 'matplotlib' has no attribute 'backends'

In [3]: import matplotlib.backends

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _matplotlib_manages_backends() call checks that the version of Matplotlib is late enough to include the required backed_registry functions, so the import should always work. But now you have drawn my attention to this I realise it would be better to be explicit and use hasattr checks for backend_registry and its function resolve_gui_or_backend. This would avoid us having to have a hardcoded Matplotlib version and would allow us to merge this work before we are entirely sure what version of Matplotlib will include the corresponding changes.

IPython/core/pylabtools.py Outdated Show resolved Hide resolved
# Since IPython 8.23.0 use None for no gui event loop rather than "inline".
if gui == "inline":
gui = None

Copy link
Member

Choose a reason for hiding this comment

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

Should we emit a warning here, to make sure that things like backend_to_gui never return "inline" or similar ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only get to this code if we are using "old" Matplotlib (i.e. before the backend_registry changes), in which case we will have gui == "inline" for the inline backend. There will already have been a DeprecationWarning issued for accessing backend2gui (a few lines above) and a second warning here might be more confusing than helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add an extra comment in the code here though.

IPython/core/pylabtools.py Outdated Show resolved Hide resolved
IPython/core/tests/test_pylabtools.py Show resolved Hide resolved
@Carreau
Copy link
Member

Carreau commented Apr 3, 2024

Thanks for your patience.

Misc questions, if you tell me this is ready we can merge as is and we can resolve the questions in subsequent iterations.

@ccordoba12
Copy link
Member

Quick question: does some work also need to be done in IPykernel to support this? I say it because it has its own logic to handle backends, which is independent of IPython.

@Carreau
Copy link
Member

Carreau commented Apr 4, 2024

I think this is fine, I can't find any direct reference to this in IPykernel, and I believe this is why we have matplotlib-inline; to not have this cross-dependency. It might create issues with much older ipykernel though, but I think it's fine to require more recent ones.

@ianthomas23
Copy link
Contributor Author

I can confirm that no changes are required in current IPyKernel for this. The backend handling hasn't changed, just where the mapping from matplotlib magic to backend and gui loop occurs. I have been testing with Jupyter as I go along; this work is sponsored by QuantStack so Jupyter support is very much at the forefront of my mind.

There are a couple of places in IPyKernel that use backend to gui loop mapping and hardcoded backend names (matplotlib-inline) that could be improved in future as this information could come from Matplotlib. I will deal with these.

@ccordoba12
Copy link
Member

Ok, thanks for the confirmation @ianthomas23 and @Carreau!

@Carreau
Copy link
Member

Carreau commented Apr 8, 2024

I think you may have forgotten to push ? You replied "Done" in a few place but I don't see the changes...

@ianthomas23
Copy link
Contributor Author

I think you may have forgotten to push ? You replied "Done" in a few place but I don't see the changes...

Yes, I got distracted 😄.

@ianthomas23
Copy link
Contributor Author

This breaks the --matplotlib=something command-line option:

$ ipython --matplotlib=osx -c "print(23)"
[TerminalIPythonApp] CRITICAL | Bad config encountered during initialization: The 'matplotlib' trait of a TerminalIPythonApp instance expected any of [] (case-insensitive) or None, not the str 'osx'.

@ianthomas23
Copy link
Contributor Author

I've added a fix for ipython --matplotlib=something and --pylab=something. These previously used a hardcoded list derived from the now-deprecated backends module-level attribute, and now they get the list from Matplotlib. I am lazily-evaluating this list as people using IPython without Matplotlib don't need the information. To do this I have subclassed the CaselessStrEnum traitlet:

https://github.com/ianthomas23/ipython/blob/b1d9d89eda82337fdc7f47689b024a93a38342c0/IPython/core/shellapp.py#L114

so that initially the list of possible values is None and it is only correctly populated when first accessed (via __getattribute__). It is not much code but I am not sure if this is the best solution. I am not very familiar with traitlets and I couldn't find a better approach.

@Carreau
Copy link
Member

Carreau commented Apr 12, 2024

Ok, let's get that in.

@Carreau Carreau merged commit e0d3e4c into ipython:main Apr 12, 2024
16 of 19 checks passed
@ianthomas23 ianthomas23 deleted the entry_points branch April 12, 2024 12:50
@ianthomas23
Copy link
Contributor Author

Ok, let's get that in.

Thanks. I'll update the docs separately.

When you want to do the next release I should probably look over the code again to check it is not about to break anything.

@Carreau
Copy link
Member

Carreau commented Apr 16, 2024

When you want to do the next release I should probably look over the code again to check it is not about to break anything.

I usually do the releases on last Friday of each month, so next release should be in about 10 days. If there is a critical issue I can yank and we "only" have to wait another month. So it's ok.

@Carreau Carreau added this to the 8.24 milestone Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move backend mapping to Matplotlib
3 participants