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

Enable lazy loading of submodules #3490

Closed
arjxn-py opened this issue Oct 31, 2023 · 15 comments
Closed

Enable lazy loading of submodules #3490

arjxn-py opened this issue Oct 31, 2023 · 15 comments
Assignees
Labels
difficulty: medium Will take a few days priority: low No existing plans to resolve

Comments

@arjxn-py
Copy link
Member

arjxn-py commented Oct 31, 2023

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 #3475 (comment)

@arjxn-py arjxn-py added the priority: high To be resolved as soon as possible label Oct 31, 2023
@prady0t
Copy link
Contributor

prady0t commented Nov 1, 2023

Hey can I look into it?

@agriyakhetarpal
Copy link
Member

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.

@prady0t
Copy link
Contributor

prady0t commented Nov 1, 2023

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

@agriyakhetarpal agriyakhetarpal added the difficulty: medium Will take a few days label Nov 20, 2023
@agriyakhetarpal
Copy link
Member

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

@arjxn-py
Copy link
Member Author

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

@AbhishekChaudharii
Copy link
Contributor

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?

@arjxn-py
Copy link
Member Author

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 🙂

@AbhishekChaudharii
Copy link
Contributor

Sure no problem :)

@arjxn-py arjxn-py added priority: medium To be resolved if time allows and removed priority: high To be resolved as soon as possible labels Feb 20, 2024
@kratman
Copy link
Contributor

kratman commented Mar 22, 2024

@arjxn-py Can we close this now?

@agriyakhetarpal
Copy link
Member

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

@kratman
Copy link
Contributor

kratman commented Mar 22, 2024

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

@agriyakhetarpal
Copy link
Member

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 agriyakhetarpal added priority: low No existing plans to resolve and removed priority: medium To be resolved if time allows labels Mar 25, 2024
@kratman
Copy link
Contributor

kratman commented Mar 25, 2024

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

@agriyakhetarpal
Copy link
Member

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
Copy link
Member

agriyakhetarpal commented Apr 6, 2024

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 agriyakhetarpal closed this as not planned Won't fix, can't repro, duplicate, stale Apr 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium Will take a few days priority: low No existing plans to resolve
Development

Successfully merging a pull request may close this issue.

5 participants