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

Backend refactor #8777

Closed
wants to merge 58 commits into from
Closed

Conversation

tacaswell
Copy link
Member

This is a rebase of #4143

@jklymak
Copy link
Member

jklymak commented May 30, 2018

Re-milestoning. This looks like something someone will have to sit down with for a few days....

@jklymak jklymak added Difficulty: Hard https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues MEP: MEP27 decouple pyplot from backends status: needs rebase labels May 30, 2018
@jklymak jklymak modified the milestones: v3.0, v3.1 Jul 9, 2018
@leycec
Copy link

leycec commented Dec 13, 2018

This was amazing – and desperately needed. Given the growing laundry list of conflicts, this also seems decreasingly likely to ever hit the mainline. This would only be the latest in a chain of similar pull requests to ultimately fail: first #2617, then #2624, then #4143, and now this.

Throwing essential volunteer contributions away year-after-year isn't the best recipe for success. Is the core issue here the practical infeasibility of refactoring the pyplot API in this extravagant manner without breaking backward compatibility? If so, would creating a parallel object-oriented API resolving these issues while preserving the existing pyplot API as is possibly be a more pragmatic solution?

The merit of the latter approach is that it would permit the new API to be constructed iteratively over the course of multiple pull requests rather than all-at-once over the course of a single pull request, as has been the case until now. In theory, an evolutionary rather than revolutionary approach should substantially reduce the likelihood of volunteer burnout and inevitable abandonment.

Of course, any solution is going to be heavyweight, highly non-trivial, and prone to misery. But our current approach doesn't appear to be working at all. Perhaps it's time to contemplate alternate thought patterns.

@anntzer
Copy link
Contributor

anntzer commented Dec 13, 2018

FWIW I have read through the PR a few times and I still don't understand its fundamental motivation (what does it solve?). Can you describe why you consider this "desperately needed"?

@jklymak jklymak modified the milestones: v3.1.0, v3.2.0 Feb 7, 2019
@tacaswell tacaswell modified the milestones: v3.2.0, needs sorting Aug 19, 2019
@jklymak jklymak added the stale label Jul 15, 2020
@jklymak jklymak marked this pull request as draft September 8, 2020 01:58
@OceanWolf
Copy link
Contributor

Hi @anntzer, sorry a bit late in answering your question, as the person who authored this version of the backend refactor, I will do my best to (at least partially) answer it.

Many motivations exist for this, the main one being that this exists as backend code, which means that from a design principle it should not know anything about matplotlib. Solving this means:

  1. We significantly reduce the amount of code that has been duplicated across the backends. Duplicated code: increases the maintenance requirements; makes it more likely that bugs get created; creates a larger file download.

  2. The possibility to use the "ready to use" GUI inside other applications, without having to reconstruct the window/toolbar. For example @fariza wanted to create a tabbed figure manager. Separating out the backend Window code from the matplotlib FigureManager allows us to reuse the Window code.

  3. Requiring pyplot in the backends creates a huge unneeded resource import, it also creates potential import loops (where A imports B, B imports C, and C imports A) in the future.

  4. During my refactor I found many inconsistencies in the API used by the various backends, we need this cleaned up.

  5. Reducing the amount of spaghetti code in matplotlib which exists as a barrier to new potential developers from getting involved. We want to get more people involved in contributing to matplotlib, by getting such fundamentals sorted this helps to make that happen.

  6. Matplotlib has been accumulating PRs and open issues, all the above points will help with that by:

    • reducing the possibility of bugs getting introduced
    • reduces the time needed for developers to fix bugs and add features
    • makes it easier to attract new developers.

If you haven't already done this you should check out the MEP on this, MEP 27.

@jklymak
Copy link
Member

jklymak commented Apr 17, 2022

Of note here is https://github.com/matplotlib/mpl-gui which aims to replace pyplot as the gui manager. It doesn't refactor anything at a low level, but it obviates the need for pyplot to used to keep a figure stack.

@OceanWolf
Copy link
Contributor

@jklymak Thank you for that information, I look forward to taking a proper look at it. :)

@OceanWolf
Copy link
Contributor

@leycec Wow, thank you for your amazing comment, I feel deeply touched to receive such glowing praise of my work! Please feel free to add your reasons for why you see this as desperately needed; I would love to hear them! :)

To answer your question: AFAIK there were no issues with the PR itself, it was just a matter of reviewers having the time to review a large PR; as a (now mainly) volunteer community, time exists as a very precious commodity for us. We had just merged the main MEP22 PR into the codebase which was over twice the size of this PR so I had presumed that I had created a PR small enough to get accepted even if it would take a while for the reviewers to have a block of time big enough to review another large PR; however I had not anticipated how frequently I would have to rebase the PR, as this PR, while smaller, touched many more parts of the codebase than MEP22 and I also failed to understand how this frequent need to rebase would drain me of energy; I have learnt my lessons. :)

Now though I have a plan to break this main PR into several much smaller PRs, assuming this still wants to get done. ;) Right now I can't afford the time to commit to this but I have applied for the Research Software Engineer position, so if I get it then I most certainly will have the time, and quite soon! :)

@anntzer
Copy link
Contributor

anntzer commented May 10, 2022

Many motivations exist for this, the main one being that this exists as backend code, which means that from a design principle it should not know anything about matplotlib. Solving this means:

Backends code literally exist to extend matplotlib's functionality, so it's not clear to me what "not knowing about matplotlib" means... (or perhaps you meant pyplot?)

We significantly reduce the amount of code that has been duplicated across the backends. Duplicated code: increases the maintenance requirements; makes it more likely that bugs get created; creates a larger file download.

Note that a large amount of code deduplication also already went in with #8773 and more recently #22925, using a different (and backcompatible) strategy.

The possibility to use the "ready to use" GUI inside other applications, without having to reconstruct the window/toolbar. For example @fariza wanted to create a tabbed figure manager. Separating out the backend Window code from the matplotlib FigureManager allows us to reuse the Window code.

My PoV is that embedding should use FigureCanvas and treat FigureManagers as a pyplot-only concept (see #18854 (comment)), although I am not dead set on that.

Requiring pyplot in the backends creates a huge unneeded resource import, it also creates potential import loops (where A imports B, B imports C, and C imports A) in the future.

I don't believe the actual import is costly at all (e.g. compared to importing the GUI toolkit). Likewise, let's not tread into hypothetical import loops if they don't actually exist.

During my refactor I found many inconsistencies in the API used by the various backends, we need this cleaned up.

I agree inconsistencies should be fixed but this can be done in a piecemeal fashion, not in the context of a mega-PR that changes many other things at the same time.

Reducing the amount of spaghetti code in matplotlib which exists as a barrier to new potential developers from getting involved. We want to get more people involved in contributing to matplotlib, by getting such fundamentals sorted this helps to make that happen.

Matplotlib has been accumulating PRs and open issues, all the above points will help with that by:

reducing the possibility of bugs getting introduced
reduces the time needed for developers to fix bugs and add features
makes it easier to attract new developers.

I do hope that the improvements in #8773/#22925 help in that direction, by clarifying and simplifying the definition of "what is a backend" (concretely, my ultimate goal would be to say "a backend is a module with a FigureCanvas global" (with extra functionality that can be overridden or provided by inheritance), nothing more, which seems(?) simpler than the definition proposed by MEP27).

If you haven't already done this you should check out the MEP on this, MEP 27.

I have indeed read MEP27 quite a few times.

@OceanWolf
Copy link
Contributor

Backends code literally exist to extend matplotlib's functionality, so it's not clear to me what "not knowing about matplotlib" means... (or perhaps you meant pyplot?)

I think it depends on how we define pyplot and matplotlib. But I do also think that I see this very differently from you. Maybe I have gotten this wrong, but I thought the purpose of the backends was to allow MPL to be backend-agnostic. In my mind that means we should treat our backend code as just something that wraps up the various external backend APIs in a nice neat bow, creating one unified backend API. Because of this it should not provide any more functionality than what the external APIs provide by themselves, as such additional functionality could get done at a higher level using our unified backend API.

So rather than seeing the backends as extending MPL, I see the backends as providing a common user interface toolkit that matplotlib uses and gets built upon. I don't see the backends as extending MPL because MPL requires the use of a backend to output the figure, whether using a non-graphical backend to output to a file, or a graphical one to output directly to the screen.

Likewise, let's not tread into hypothetical import loops if they don't actually exist.

I find it difficult not to tread here, because I see it as a self-fulfilling prophecy:

  • We don't fix the possibility for for import loops because they exist as hypothetical.
  • Import loops remain hypothetical as people will find alternative solutions that do not create them.

I see this as part of future-proofing MPL, better to do the hard work now before people find that they need it, in which case they will just write bad hacky code.

Note that a large amount of code deduplication also already went in with #8773 and more recently #22925, using a different (and backcompatible) strategy.

I think I like this, I don't think it exists as a different strategy though, tacaswell said it works orthogonal with this one..

I agree inconsistencies should be fixed but this can be done in a piecemeal fashion, not in the context of a mega-PR that changes many other things at the same time.
I completely agree that this should get broken down into smaller PRs which I noted above. I think this large block of code needed to get created initially to provide a proof of concept that the end goal was reachable, but then I think it should have gotten broken down. As I recall the reason it didn't was that there was talk of wanting to preserve the commit history both for the commit messages, and to give an idea to outsiders how much work was done.

@OceanWolf
Copy link
Contributor

But yes, I look forward to breaking this down into smaller chunks which will then enable more targeted conversation about specific design elements.

@tacaswell tacaswell closed this Sep 1, 2022
@tacaswell tacaswell deleted the backend-refactor branch September 1, 2022 22:48
@tacaswell tacaswell restored the backend-refactor branch September 1, 2022 22:48
@story645 story645 removed this from the future releases milestone Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty: Hard https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues MEP: MEP27 decouple pyplot from backends status: needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants