Resolve default imports for optional dependencies

[arjxn-py]

Description

With this PR i'm trying to fix the error getting while importing pybamm without any extra dependencies for pybamm=23.9rc0

Fixes #3473

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[Saransh-cpp]

Thanks, @arjxn-py! Some comments below -

[agriyakhetarpal]

https://github.com/pandas-dev/pandas/blob/984d75543fce5c68f0dcc4f0b3500256e4a9c0c9/pandas/io/pytables.py#L566

https://github.com/pandas-dev/pandas/blob/984d75543fce5c68f0dcc4f0b3500256e4a9c0c9/pandas/compat/_optional.py#L83-L165

This is how pandas does it, we could add a function with similar functionality in pybamm.util. These are design choices imo after all, but a write-once-use-anywhere paradigm would be useful in this scenario. For example, adding a decorator would be great such that it can be used on a class (or at its instantiation, not recommended however) which would automatically check if an optional dependency can be imported or not, and would apply to all subsequent functions defined inside it. I am not sure how one would go about implementing it, but something like

@pybamm._check_and_import_optional_dependency(name="pybtex")
class Citations:
   
   def print_citation(...):
    ...

This way, it would raise a warning plus a helpful message if pybtex cannot be found, but load it into the namespace if it can be found. We currently import the print_citations() method directly, which means the decorator would apply to the method only and not throw duplicate warnings. Python imports are cached very effectively which means there is negligible overhead if we import a dependency multiple times inside the same file or within a module.

Although these function-scoped imports are also a great idea btw. We already do it for matplotlib in the QuickPlot classes and methods; the good thing is that we don't add optional dependencies often, which means this is going to be a one-time thing and maintaining such imports would not be difficult (skfem for the SpatialMethod module, bpx for BPX, etc.)

TIL that importlib.metadata.distribution("pybamm").requires prints out a list of strings that contain all required and optional dependencies. The messy part is however about parsing these strings.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bfddc83) 99.58% compared to head (d685c38) 99.58%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3475   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        20058    20116   +58     
========================================
+ Hits         19975    20033   +58     
  Misses          83       83           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arjxn-py]

I've finally managed to make a decorator function but there are complications with it :

• If applied pybamm.util.have_optional_dependency will be executed whenever pybamm is imported. This behavior is expected since decorators are typically execute when the class is imported.
• I am having trouble to make module available within the local scope of the method & i'm not sure if this is possible with a decorator function or not.

I'll also put in some efforts to do it with simple reusable function first.

[agriyakhetarpal]

I've finally managed to make a decorator function but there are complications with it :

• If applied pybamm.util.have_optional_dependency will be executed whenever pybamm is imported. This behavior is expected since decorators are typically execute when the class is imported.

I think this should be fine, the decorator should just execute "silently" and return nothing if a dependency is found. Is there a way to "lazy-load" an entire module at the time of import, e.g., to receive no warnings when doing import pybamm but to receive a warning about pybtex only if someone executes any of the functionality under pybamm.citations. or in other words tries to instantiate the Citations class? Doing that it would make import pybamm faster, even, I would say, since certain modules would be skipped.

[agriyakhetarpal]

I am having trouble to make module available within the local scope of the method & i'm not sure if this is possible with a decorator function or not.

In that case, it would be fine to do this for (inside) methods only and not for classes: this practice won't conform to DRY, but atleast it won't apply to private methods available under a class (we want to mark just those pieces of functionality which are available as a part of the public API)

[agriyakhetarpal]

See also: https://scientific-python.org/specs/spec-0001/ (this demands a separate issue, I guess)

[Saransh-cpp]

See also: https://scientific-python.org/specs/spec-0001/ (this demands a separate issue, I guess)

Yes, separate and important issue. Importing pybamm usually takes relatively more time than other libraries, and I think this should fix that.

[arjxn-py]

See also: https://scientific-python.org/specs/spec-0001/ (this demands a separate issue, I guess)

Yes, happy to open one issue for it & also look into the same, @agriyakhetarpal do let me know if you want to open with some suggestion or simplifications.

[agriyakhetarpal]

I see that there are some issues with building the documentation (there is a possibility it might break some of our internal Sphinx extensions that rely on autodoc), though I think we can discuss this further on a new issue thread. Some other simplications could be to reorganise top-level imports without an expense to the current API (i.e., no breaking changes).

[Saransh-cpp]

Took over and made some changes. Probably the last change request (phew)

We should also be testing the "lite" pybamm in CI (as pointed out by @agriyakhetarpal), but I think we can do that after this release. The testing part will also tie up nicely with unittest to pytest migration (pytest.importorskip is quite intelligent and can be added module-wide).

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[Saransh-cpp]

Thanks a ton for the amazing work, @arjxn-py! 🙂

I'll wait for others to review this before merging.

[agriyakhetarpal]

Looks good now, thank you @arjxn-py and @Saransh-cpp! Just needs a CHANGELOG entry and a documentation update to mark anytree as a required dependency in the installation guide.

I tested all example scripts with pybamm[plot] and everything worked as expected. I would also recommend re-running the failed integration tests and notebook tests just to be on the safer side of things.

[arjxn-py]

Not sure why pre-commit.ci is failing now 😕

[Saransh-cpp]

Not related to this PR, se #3519

[brosaplanella]

Looks good to me, thank you very much to all who contributed fixing this issue!