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

PyBaMM import error - Cannot run anything (import broken due to pybtex) #3940

Closed
agriyakhetarpal opened this issue Mar 28, 2024 Discussed in #3928 · 11 comments · Fixed by #3968
Closed

PyBaMM import error - Cannot run anything (import broken due to pybtex) #3940

agriyakhetarpal opened this issue Mar 28, 2024 Discussed in #3928 · 11 comments · Fixed by #3968
Assignees
Labels
bug Something isn't working priority: high To be resolved as soon as possible

Comments

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Mar 28, 2024

Discussed in #3928

Originally posted by abhishek-appana March 25, 2024
Hello,

I am running Python 3.11.8 and installed PyBaMM 24.1 using pip on a Windows system. Earlier I was using Python 3.10 and everything was fine. But I am getting the below error after upgrading Python.

When I run import pybamm, it gives me the error:

ModuleNotFoundError: Optional dependency pybtex.database is not available. See https://docs.pybamm.org/en/latest/source/user_guide/installation/index.html#optional-dependencies for more details.

I tried installing pybamm using pip install pybamm[all]. I was still getting this error. How do I resolve this?


I think that this is a valid bug and it is still failing, and import pybamm should work without installing pybamm[all]. It can be noticed in the logs here: https://github.com/pybamm-team/PyBaMM/actions/runs/8461852686/job/23182296194, so I've converted this discussion to an issue instead (cc: @Saransh-cpp @arjxn-py).

Perhaps this isn't being properly checked through #3892, @lorenzofavaro?

@agriyakhetarpal
Copy link
Member Author

Perhaps #3886 broke this where the try-catch blocks everywhere were removed? I haven't looked at this in enough detail, but the breakage is coming from this instantiation of the Citations class at the end of the file.

citations = Citations()

where __init__ tries to call the function _reset(), and that in-turn is trying to register the PyBaMM and NumPy citation, which will fail?

@Saransh-cpp
Copy link
Member

Hmm, I somehow can't reproduce this locally. I'm not sure where is this originating from.

@agriyakhetarpal
Copy link
Member Author

Me neither

@agriyakhetarpal agriyakhetarpal added the priority: high To be resolved as soon as possible label Mar 28, 2024
@agriyakhetarpal
Copy link
Member Author

I haven't looked at this again, but I think having it fail in a fresh CI environment means there is indeed a bug, even if we can't reproduce it on our own machines. I added the "high priority" label for now.

Maybe a fix can be to register the PyBaMM and NumPy papers only inside the print_citations() method and only if they don't exist in the _papers_to_cite variable, so that they get cited when someone actually runs pybamm.print_citations(). I'm not sure if this is a complete fix, though.

@prady0t
Copy link
Contributor

prady0t commented Mar 29, 2024

Let me look into this.

@arjxn-py
Copy link
Member

arjxn-py commented Apr 1, 2024

I have looked into this and looks like this is failing as we merged #3932 & haven't removed the jobs to test pybamm_install_odes
Oh wait, I see that they've been removed already

@lorenzofavaro
Copy link
Contributor

lorenzofavaro commented Apr 5, 2024

I had a chance to look at the issue and understand the problem.

Actually at the moment pybamm has a hidden mandatory dependency, namely pybtex. It's imported during pybamm/citations.py:41, that in turn trigger pybamm/citations.py:70:

parse_file = import_optional_dependency("pybtex.database", "parse_file")

During #3892 the error was still masked in pybamm/citations in the except:

try:
  self.read_citations()
  self._reset()
except Exception as e:  # pragma: no cover
  self._citation_err_msg = e

Beyond that, the error was not reported after for a subtle issue.
The issue is that test_pybamm_import is done inside tests/unit/test_util.py and if we look at the module tests/unit/_init__.py we find that it imports StandardModelTest which in turn imports pybamm.

When we don't have the optional libraries installed locally (e.g., from local with pybamm.[dev]), this import avoid the tests execution during the nox session (it stops before), even though it reports them as PASSED. In fact, if you try to run the specific tests individually (i.e. python -m unittest tests.unit.test_util.TestUtil.test_pybamm_import), you would get an exception before the test run.

Instead, when we have the optional libraries installed (e.g. from CI with pybamm.[all]), we don't notice the error because digging deep into the importlib documentation, importlib.reload(...) does not reload all sub-modules.
So when it initially in StandardModelTest imports pybamm, it also imports pybtex.database which will no longer be imported even with the reload (not triggering the error we expect). To solve this, we should edit the test_pybamm_import function.

Possible solution steps:

  1. Remove the hidden mandatory dependency from pybtex - I know that @prady0t is investigating to solve this.
  2. Fix error detection - Edit tests/unit/test_util.py, in particular edit test_pybamm_import:
    • Do not use importlib.reload(...) to reload pybamm, instead remove pybamm from the sys.modules and all its sub-modules (so also pybamm.citations etc..).
    • In the sys.modules set to None all the optional modules (e.g. pybtex) and all their sub-modules (so also pybtex.database, etc..).

I could work on this in a shared PR with @prady0t.

@agriyakhetarpal
Copy link
Member Author

Thanks for posting this here, @lorenzofavaro, I think this is a reasonable solution based on our conversation earlier. You mentioned that you're working on some aspects of this too, so I'll assign you to this alongside @prady0t.

@prady0t
Copy link
Contributor

prady0t commented Apr 8, 2024

Hey @agriyakhetarpal, I won't be able to work on anything for at least a week due to a minor road accident. If @lorenzofavaro you are able to completely take care of this please go ahead.

@arjxn-py
Copy link
Member

arjxn-py commented Apr 8, 2024

Sorry to hear that @prady0t, Get well soon.
Don't worry as we'll be able to take care of this one.

@prady0t
Copy link
Contributor

prady0t commented Apr 8, 2024

Thanks a lot 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: high To be resolved as soon as possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants