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

[Bug]: v24.9.0 broken with [cite] optional dependency #4432

Closed
BradyPlanden opened this issue Sep 11, 2024 · 14 comments · Fixed by #4478
Closed

[Bug]: v24.9.0 broken with [cite] optional dependency #4432

BradyPlanden opened this issue Sep 11, 2024 · 14 comments · Fixed by #4478
Assignees
Labels
bug Something isn't working priority: high To be resolved as soon as possible

Comments

@BradyPlanden
Copy link
Member

BradyPlanden commented Sep 11, 2024

PyBaMM Version

v24.9.0

Python Version

3.12

Describe the bug

Installing with optional [cite] dependency results with the below import error.

Steps to Reproduce

python -m pip install 'pybamm[cite]' produces the below error when running import pybamm.

The above exception was the direct cause of the following exception:

ModuleNotFoundError                       Traceback (most recent call last)
Cell In[1], line 1
----> 1 import pybamm

File ~/.pyenv/versions/3.12.4/envs/pybamm-cite-test/lib/python3.12/site-packages/pybamm/__init__.py:25
     23 from .logger import logger, set_logging_level, get_new_logger
     24 from .settings import settings
---> 25 from .citations import Citations, citations, print_citations
     27 # Classes for the Expression Tree
     28 from .expression_tree.symbol import *

File ~/.pyenv/versions/3.12.4/envs/pybamm-cite-test/lib/python3.12/site-packages/pybamm/citations.py:271
    265         raise Exception(
    266             "Verbose output is available only for the terminal and not for printing to files",
    267         )
    268     pybamm.citations.print(filename, output_format, verbose)
--> 271 citations = Citations()

File ~/.pyenv/versions/3.12.4/envs/pybamm-cite-test/lib/python3.12/site-packages/pybamm/citations.py:36, in Citations.__init__(self)
     33 # Dict mapping citations keys to BibTex entries
     34 self._all_citations: dict[str, str] = dict()
---> 36 self.read_citations()
     37 self._reset()

File ~/.pyenv/versions/3.12.4/envs/pybamm-cite-test/lib/python3.12/site-packages/pybamm/citations.py:68, in Citations.read_citations(self)
     64 """Reads the citations in `pybamm.CITATIONS.bib`. Other works can be cited
     65 by passing a BibTeX citation to :meth:`register`.
     66 """
     67 if not self._module_import_error:
---> 68     parse_file = import_optional_dependency("pybtex.database", "parse_file")
     69     citations_file = os.path.join(pybamm.__path__[0], "CITATIONS.bib")
     70     bib_data = parse_file(citations_file, bib_format="bibtex")

File ~/.pyenv/versions/3.12.4/envs/pybamm-cite-test/lib/python3.12/site-packages/pybamm/util.py:332, in import_optional_dependency(module_name, attribute)
    328         return module
    330 except ModuleNotFoundError as error:
    331     # Raise an ModuleNotFoundError if the module or attribute is not available
--> 332     raise ModuleNotFoundError(err_msg) from 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.

Relevant log output

No response

@agriyakhetarpal
Copy link
Member

cc: @kratman who was working on this back in #4383. I don't know why this fails, though, because we test against this behaviour as well...

@agriyakhetarpal agriyakhetarpal added the priority: high To be resolved as soon as possible label Sep 11, 2024
@kratman
Copy link
Contributor

kratman commented Sep 11, 2024

Yeah I am looking into it

@agriyakhetarpal
Copy link
Member

Thanks! I guess we should release 24.9.1 with a fix by today? Also, we install all the optional dependencies when testing the wheels, too, which includes [cite], so this shouldn't have happened.

@kratman
Copy link
Contributor

kratman commented Sep 11, 2024

@BradyPlanden There must be a dependency issue, I can reproduce this with

pip install 'pybamm[cite]'

but not with

pip install 'pybamm[all,cite]'

@agriyakhetarpal
Copy link
Member

I'm unable to reproduce with pybamm[cite]

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Sep 11, 2024

This is what I tried:

mamba create -n pybamm-test-env python
mamba activate pybamm-test-env
python -m pip install 'pybamm[cite]'
python -c "import pybamm"  # works

Edit: and couldn't reproduce it for a second time

@agriyakhetarpal
Copy link
Member

/opt/homebrew/bin/python3.12 -m venv venv
source venv/bin/activate
python -m pip install pybamm"[cite]"   # this fails

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Sep 11, 2024

@kratman and @BradyPlanden, this is coming from an undeclared dependency on setuptools, we require it on Python 3.12 and later because pybtex requires it. Virtual environments for Python 3.12 and later don't include seed packages (such as setuptools and wheel – mamba envs do, which is why I couldn't reproduce in the earlier example). Installing setuptools in the venv above fixes the error.

@agriyakhetarpal
Copy link
Member

Unfortunately, pybtex is still unmaintained and they haven't released the fix for this yet (see #3648).

@kratman
Copy link
Contributor

kratman commented Sep 11, 2024

The reason you do not see it with pip install "pybamm[all]" is because of pip install "pybamm[examples]".

@BradyPlanden
Copy link
Member Author

It looks like pybtex has a unreleased fix. It's a bit annoying that this is due to not deploying a release. Here's the relevant issue: https://bitbucket.org/pybtex-devs/pybtex/issues/169/replace-pkg_resources-with

@agriyakhetarpal
Copy link
Member

Yes, this is why we were installing setuptools in our nox sessions

@kratman
Copy link
Contributor

kratman commented Sep 11, 2024

Yeah we just need to swap out pybtex and make citations non-optional. The citations object is global, all the "check if it is installed" stuff is because we are hiding a dependency too

@agriyakhetarpal
Copy link
Member

I would be okay with making citations non-optional, too. I think the functionality is nice to have by default, since we encourage researchers to use pybamm.print_citations() at the end of each script, especially through the example notebooks.

@kratman kratman self-assigned this Sep 11, 2024
@BradyPlanden BradyPlanden changed the title [Bug]: v24.9.0 broken with [cite] optional dependancy [Bug]: v24.9.0 broken with [cite] optional dependency Sep 11, 2024
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.

3 participants