search

ManimCommunity / manim

A community-maintained Python framework for creating mathematical animations.
https://www.manim.community
MIT License
20.94k stars 1.52k forks source link

Using pluggy for managing Plugins #1133

Open naveen521kk opened 3 years ago

naveen521kk commented 3 years ago

Enhancement proposal

Pluggy: https://pluggy.readthedocs.io/

Currently how out plugin system works is

With pluggy in place, we can decide the parts of the library which can be extendable without subclassing, see an example. https://pluggy.readthedocs.io/en/latest/#a-toy-example And it feels like we shouldn't reinvent the wheel rather than using something which is already available.

Additional comments

This would kinda require all the plugins to update to use pluggy. Let the previous implementation of importing to global namespace stay, for backward compatibility.

Proof of concept

eulertour commented 3 years ago

I think the drawback from learning a new tool will outweigh the potential benefit from using a solution outside the standard library, especially this early on. I think following the instructions from python's documentation will probably work for what we need.

GameDungeon commented 3 years ago

The tiny amount of plugins would make this the best time to make this change if we ever do,

naveen521kk commented 3 years ago

I think following the instructions from python's documentation will probably work for what we need.

That is already implemented and is working, using https://packaging.python.org/guides/creating-and-discovering-plugins/#using-package-metadata

I think the drawback from learning a new tool will outweigh the potential benefit from using a solution outside the standard library, especially this early on

I think for the number of plugins we have currently, it is better and easier to move to something which is well tested and has more features. With the current implementation, people can either subclass or monkey patching, which can sometimes become problematic, see https://pluggy.readthedocs.io/en/latest/#why-is-it-useful.

eulertour commented 3 years ago

One thing that I'd be cautious of is the idea of requiring changes to the main code in order to accommodate new plugins. From that page,

The pluggy approach puts the burden on the designer of the host program to think carefully about which objects are really needed in a hook implementation.

If we do want to go with this approach we should be careful not to have a situation where people who develop plugins are continually making PRs to add hooks for them.

GameDungeon commented 3 years ago

If #1131 goes through, and we do trim the fat, it should not be a big issue to add hooks. Esessialy with our class inheritance structure.

naveen521kk commented 3 years ago

If we do want to go with this approach we should be careful not to have a situation where people who develop plugins are continually making PRs to add hooks for them.

Could be painful yeah, but if we already have good implementation at first, this won't happen. Also, this would help us implemented new Renderer's, Mobjects, Animations very easily. As in that's what you are telling in #1131. Actually, speaking it would hassle to implements the things you told in #1131 (about removing extra code), without this implementation.

GameDungeon commented 3 years ago

@eulertour Going with what was said above, I think in the code move that we will make most of the needed hooks. I say someone should make a pull for this so we can see the changes in reality.

GameDungeon commented 3 years ago

This Issue has been put on hold until we migrate to OpenGL

eulertour commented 3 years ago

If we want to put this in hold then we should specify what it means to move to opengl. Does this mean cairo rendering needs to be removed? Or just not used by default or deprecated or something like that.

GameDungeon commented 3 years ago

I'm sorry I meant to put that in #1131. I still want this to go through now, as I think it will help plugin devs (Me :P) with their plugins.