PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Enable lazy loading of submodules

Open arjxn-py opened this issue 2 years ago • 8 comments

Enabling lazy loading of submodules should potentially make PyBaMM import faster. See also: https://scientific-python.org/specs/spec-0001/

Originally posted by @agriyakhetarpal in https://github.com/pybamm-team/PyBaMM/issues/3475#issuecomment-1786894302

arjxn-py avatar Oct 31 '23 15:10 arjxn-py

Hey can I look into it?

prady0t avatar Nov 01 '23 06:11 prady0t

Hi @prady0t, thanks for helping us. Doing this would be much appreciated, but it would be better to start after #3475 gets completed. While we need import pybamm to be faster, the expected outcome is that there must not be any changes to the user-facing API.

agriyakhetarpal avatar Nov 01 '23 10:11 agriyakhetarpal

Thanks for replying. I will be looking at other issues in the meantime.

prady0t avatar Nov 01 '23 10:11 prady0t

See also: https://docs.python.org/3/library/importlib.html#implementing-lazy-imports; a rather crude, bare-bones implementation for delaying imports

agriyakhetarpal avatar Nov 26 '23 17:11 agriyakhetarpal

I can look into this now. Keeping in mind that it is not a beginner level issue and @prady0t is occupied with other one.

arjxn-py avatar Nov 27 '23 13:11 arjxn-py

Hi @arjxn-py @agriyakhetarpal If you're not working on this issue, May I go ahead and try to solve this? Also can you guide me?

AbhishekChaudharii avatar Jan 12 '24 08:01 AbhishekChaudharii

Hey @AbhishekChaudharii, thanks for showing your interest on this but I am currently on it and going to open up a PR in a couple of days. Feel free to look into other issues 🙂

arjxn-py avatar Jan 12 '24 09:01 arjxn-py

Sure no problem :)

AbhishekChaudharii avatar Jan 12 '24 09:01 AbhishekChaudharii

@arjxn-py Can we close this now?

kratman avatar Mar 22 '24 19:03 kratman

I would say we should keep it open (other, large packages are able to make it work – maybe we can too), but yes, it is @arjxn-py's call after all

agriyakhetarpal avatar Mar 22 '24 19:03 agriyakhetarpal

I thought the consensus was that it was not a major bottleneck so the work/code required to do it was bigger than the problem

kratman avatar Mar 22 '24 19:03 kratman

Right, maybe let's not close it because it's still within scope but lower down the priority for now – I'll change the labels.

agriyakhetarpal avatar Mar 25 '24 12:03 agriyakhetarpal

We can always open new tickets when problems recur, no need to keep this open

kratman avatar Mar 25 '24 12:03 kratman

But this is a recurring problem, isn't it? I just tested this in a fresh notebook/kernel without sys.modules being cached and it took ~20 seconds to import PyBaMM – so it's probably still valid. At least for organisational purposes it would be nice to keep open

agriyakhetarpal avatar Mar 25 '24 12:03 agriyakhetarpal

I just noticed that https://github.com/scientific-python/lazy_loader is now being recommended for Python versions 3.11 and later, and that too, with very specific minimum patch versions to avoid races, say 3.11.9, for example. Python 3.11 being the lowest version for us will be quite many years away at the time of writing (at least four). It sucks that I would have liked to have seen this done for PyBaMM but I don't think we have a viable choice at this time, I think we can close this and re-open later (or open a new issue, whichever way works). A better and more reliable way would be to profile PyBaMM's import and see which of the bigger modules are causing the bottleneck, and if needed then implement minor breaking changes or lazy loading at the functionality level manually at the time of use of a function, so that we aren't importing a lot of those functions when one runs import pybamm. So I'm agreeing with @kratman here and closing this issue, please feel more than free to re-open later, @arjxn-py, or to suggest an alternative enhancement to speed up the import (whose slowness remains very much a valid problem – it's just not suited to be done for the entire public API because of problems that have been previously discussed).

agriyakhetarpal avatar Apr 06 '24 01:04 agriyakhetarpal