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

Fix AbinitTimerParseError import #267

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

mbercx
Copy link
Contributor

@mbercx mbercx commented Aug 21, 2023

Fixes #262

In the following breaking change in pymatgen:

materialsproject/pymatgen@5b88fc1

The AbinitTimerParserError class was renamed to AbinitTimerParseError. Although the code has already been updated for this change, in some environments installing pymatgen>=2023.7.11 will lead to dependency conflicts. Here we adapt the import to be dependent on the installed version of pymatgen at runtime.

@mbercx
Copy link
Contributor Author

mbercx commented Aug 21, 2023

@gmatteo I noticed that the pymatgen version specifier in the setup.py is still set to >=2022.0.14:

abipy/setup.py

Line 158 in f1bb3ec

"pymatgen>=2022.0.14",

which allowed me to install it in my AiiDA common workflows environment, but according to the CHANGELOG the version should be pinned to 2023.7.17? I'm not sure that there are other reasons than the import above, but if not it might perhaps be better to be a bit more flexible?

PS: Just noticed the linting issues, will take care of that asap.

@mbercx
Copy link
Contributor Author

mbercx commented Aug 21, 2023

PS: Just noticed the linting issues, will take care of that asap.

Ah, it seems that this isn't only due to my changes. ^^

@gmatteo
Copy link
Member

gmatteo commented Aug 28, 2023

@gmatteo I noticed that the pymatgen version specifier in the setup.py is still set to >=2022.0.14:

abipy/setup.py

Line 158 in f1bb3ec

"pymatgen>=2022.0.14",

which allowed me to install it in my AiiDA common workflows environment, but according to the CHANGELOG the version should be pinned to 2023.7.17? I'm not sure that there are other reasons than the import above, but if not it might perhaps be better to be a bit more flexible?

Hi @mbercx
There's indeed an inconsistency between the pymatgen version specified in requirements.txt and the one in setup.py.
The problem is that pymatgen is breaking backward compatibility very frequently and it's difficult to maintain abipy in synch with the new versions. Not all the python modules of Abipyt are covered by integration tests. There's are indeed ab-initio workflows requiring a rather complex interplay among abipy/abinit and pymatgen and these workflows require pymatgen>=2023.7.17. What it the minimum pymatgen version required by aiida-common-workflows?

@mbercx
Copy link
Contributor Author

mbercx commented Aug 29, 2023

What it the minimum pymatgen version required by aiida-common-workflows?

aiida-common-workflows currently specifies pymatgen>=2022.1.20 (see here), but some of the dependencies (including aiida-core[atomic_tools]) also have their own pymatgen dependencies. Looking at the output of pip-compile:

pymatgen==2022.2.1
    # via
    #   abipy
    #   aiida-abinit
    #   aiida-common-workflows (pyproject.toml)
    #   aiida-core
    #   aiida-gaussian
    #   aiida-vasp

We can see the list of packages that restrict the pymatgen version and what the result is. Then looked at the (trimmed down) output of:

$ for package in abipy aiida-abinit aiida-core aiida-gaussian aiida-vasp; do echo $package; pipdeptree -p $package| grep pymatgen; done
abipy
├── pymatgen [required: >=2022.0.14, installed: 2022.2.1]
aiida-abinit
│   ├── pymatgen [required: >=2022.0.14, installed: 2022.2.1]
└── pymatgen [required: >=2022.1.20, installed: 2022.2.1]
aiida-core
aiida-gaussian
└── pymatgen [required: >=2020.4, installed: 2022.2.1]
aiida-vasp
└── pymatgen [required: >=2019.7.2,<=2022.02.03,!=2019.9.7, installed: 2022.2.1]

We can see that it's aiida-vasp's upper limit that's restricting the version to 2022.2.1. So looking at the pymatgen release history we're pretty much limited to only three pymatgen versions at this point (2022.1.20, 2022.1.24 and 2022.2.1).

Is it possible to leave the pymatgen version setup.py as it is for now, merge this PR and do a release (maybe after I run our tests again versus your develop branch)?

@gmatteo
Copy link
Member

gmatteo commented Aug 29, 2023

Is it possible to leave the pymatgen version setup.py as it is for now, merge this PR and do a release (maybe after I run our tests again versus your develop branch)?

Ok, I'm gonna merge this PR. Let me know if my develop branch passes your CI tests as I'm already seeing new incompatibilities with pymatgen develop due to pydantic

@gmatteo gmatteo merged commit b93ece7 into abinit:develop Aug 29, 2023
1 check failed
@mbercx mbercx deleted the fix/261/parser-error branch August 29, 2023 12:20
@mbercx
Copy link
Contributor Author

mbercx commented Aug 29, 2023

@gmatteo
Copy link
Member

gmatteo commented Aug 29, 2023

Ok, I release version 0.9.6 then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImportError when using abicheck.py with abipy and pymatgen
2 participants