cauldron / activity-browser

GUI for Brightway
GNU Lesser General Public License v3.0
0 stars 3 forks source link

Module discovery is poorly optimized #13

Closed ughstudios closed 4 months ago

ughstudios commented 5 months ago

Issue

discover_plugins in plugins.py needs to be updated. This code is very bad because it's iterating over every module that's imported in the python interpreter. When I ran this locally it was iterating over 400 modules. This should be limited. It's looping over all of the modules and then doing a string comparison (.startswith()).

def discover_plugins(self):
    plugins = []

    for module in iter_modules():

    if module.name.startswith('ab_plugin'):

    plugins.append(module.name)

    return plugins

Suggestions

marc-vdm commented 5 months ago

Some thoughts from my side;

First off: agree, this code is not ideal.

What I personally don't like is that the plugins have to start with ab_plugin, it's not very nice.

When I ran this locally it was iterating over 400 modules

We generally recommend to install AB in it's own environment, so that should hopefully not have 400 modules, remember that the target audience of AB is people that panic when seeing black screens with white text, those people generally don't install things they shouldn't have in their environments 😅

We should update this to only scan a single directory instead

Could you be more specific? What dir should be scanned? How would you go about this when plugins could have been downloaded from conda or pip? How would you find them?

We could also add a setting in the UI for users to set their own paths for plugins to be loaded from.

I like this idea a lot!

ughstudios commented 5 months ago

Could you be more specific? What dir should be scanned? How would you go about this when plugins could have been downloaded from conda or pip? How would you find them?

I like the idea of either allowing users to click a URI to install a plugin, or allow them to specify the search directory via some QT UI.

For example, if we had a URI, it could be like this: ab_plugin_install://plugin_name and then they could simply share this URI with people and when it's clicked it could open a confirmation prompt to ensure they actually want to install the plugin.

marc-vdm commented 5 months ago

Fine by me to add that URIs to find stuff, but I do think people should still be able to add plugins through conda/pip if they so desire.

Have you timed how long it takes to iterate over 400 modules anyway? I can't imagine iterating over all modules in an env is that expensive.

Perhaps efforts could be better spent on more impactful changes to this project, but Chris's input is most useful to you here probably.

ughstudios commented 5 months ago

Fine by me to add that URIs to find stuff, but I do think people should still be able to add plugins through conda/pip if they so desire.

I agree. Conda/pip would still be supported. But on top of this, we could have a system with the URIs. I know how to do this on Windows, but I am developing on Mac so I need to do some research on how to do it on Mac. Where do the majority of users use the program from? Windows or Mac? We should have both. I can get started on adding the support for it in Windows very quickly. I'm also waiting on Chris to setup a meeting with him to discuss some testing.

Have you timed how long it takes to iterate over 400 modules anyway? I can't imagine iterating over all modules in an env is that expensive.

I'm sure it's not that expensive, it's likely only a few extra milliseconds. But, it's best to just write code that isn't doing any unnecessary work.

Perhaps efforts could be better spent on more impactful changes to this project, but Chris's input is most useful to you here probably.

I would like to try and get the app running as a standalone executable. But I don't want to just start doing work before getting approval from everyone on the team.

will7200 commented 5 months ago

Just chiming in here, while on the surface it may seem inefficient, it is one of three valid ways that python supports. The benefit to the naming convention is that its clear to the developer how the plugin system imports and there is no downstream requirements from vendors such as using a namespace module or using entrypoints, except for the naming convention.

We can still present the user a URI system to install, but honestly, plugin developers should be using python infrastructure for distributions as this a open-source solution as opposed to a closed one.

https://packaging.python.org/en/latest/guides/creating-and-discovering-plugins/

ughstudios commented 5 months ago

Just chiming in here, while on the surface it may seem inefficient, it is one of three valid ways that python supports. The benefit to the naming convention is that its clear to the developer how the plugin system imports and there is no downstream requirements from vendors such as using a namespace module or using entrypoints, except for the naming convention.

We can still present the user a URI system to install, but honestly, plugin developers should be using python infrastructure for distributions as this a open-source solution as opposed to a closed one.

https://packaging.python.org/en/latest/guides/creating-and-discovering-plugins/

That's true that it is a method of doing it. But code should not be doing unnecessary work. There's no legitimate reason for it to load through modules that aren't actually going to be used. Also, having people manually install plugins using terminal commands (conda install) is not a very user-friendly workflow for people who are not technical.

will7200 commented 5 months ago

iter_modules does not load all modules. It is listing all possible top level modules available as dictated by sys.path. You can even pass a prefix to iter_modules to filter them out there. Importing still needs to be done after obtaining the available module. It is pretty much doing what you are proposing of siloing to a single directory, except it more versatile.

Sure i agree it's not user friendly, but we can still take advantage of the fact that plugins are distributed via python channels and install from there. We would avoid the hurdle of having to reinvent the plugin wheel when python is already capable of such things. Probably just wrap pip in the UI for installation

will7200 commented 4 months ago

Not a valid issue IMO. Closing out