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

over elaborate __init__ files are a performance hinderance #410

Open
jf--- opened this issue Apr 6, 2024 · 3 comments
Open

over elaborate __init__ files are a performance hinderance #410

jf--- opened this issue Apr 6, 2024 · 3 comments

Comments

@jf---
Copy link

jf--- commented Apr 6, 2024

I have a small gripe with compas_fab over elaborate __init__.py files.

A specific example is compas_fab/backends/__init__.py

All of the backends are loaded, where you're most likely using a single one at the time.
Now, ROS, the analytic solver and pybullet are loaded even while its unlikely to use more one solver at the time.
( FWIW, I'm working on the tesseract backend )
Loading pybullet will take seconds to load... and in the case of macOs pop-up a UI...

So a huge amount of unnecessary code is loaded...

Finally, the __init__ arguable could be just be an empty file, rather then the 166 lines it is now.
The "hardcoded" ( dare I say Javaesque 👺 ) style of the current init files are also stylistically a little substandard of the usual finesse and elegance of the compas_fab codebase.

( And then the irony of providing a LazyLoader in compas_fab.utils.lazy_loader 🧐 😉 )

@gonzalocasas
Copy link
Member

You are so very right. The lazy loader should actually avoid pybullet from loading, it seems it's not working correctly :/
I've considered multiple times whether it would be a better idea to split off all backends, and have compas_fab_ros, compas_fab_pybullet, etc. They could still live together in this repo for the time being, but it would create a better separation of concerns for sure, and would potentially open up the option of completely moving each backend to a new project/repo in the future without breaking changes in the namespacing.

@jf--- what do you think?

@jf---
Copy link
Author

jf--- commented Apr 8, 2024

You are so very right

Confirming my pedantry is an invitation for moar 😉

actually avoid pybullet from loading

I haven't checked that actually

whether it would be a better idea to split off all backends, and have compas_fab_ros, compas_fab_pybullet, etc

a git submodule might make sense here?
pybullet definitely qualifies as a plugin

now ( compas 2.0 ) feels like a nice time to do so

@yck011522
Copy link
Contributor

yck011522 commented Apr 19, 2024

Regarding splitting the code to a separate repo and link with submodule, I think it would only make sense to spend that energy if we take the opportunity to let users choose which backends to install.

However, I think this is not yet the right time. I think most compas_fab users are beginners into this field, otherwise they would interface with a backend directly, so focusing them to install all and thus going them easy access to try them all is a nice thing.

Moreover each of our backends are not really fully featured yet. And it is much easier to develop / PR / review while keeping everything in one place.

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

No branches or pull requests

3 participants