Require spglib~=1.15

[hongyi-zhao]

See here: https://github.com/atztogo/spglib/issues/93#issuecomment-615519305 for more information.

[codecov]

Codecov Report

Merging #3944 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3944   +/-   ##
========================================
  Coverage    78.23%   78.23%           
========================================
  Files          461      461           
  Lines        34075    34075           
========================================
  Hits         26660    26660           
  Misses        7415     7415           

Flag	Coverage Δ
#django	70.28% <ø> (ø)
#sqlalchemy	71.13% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c746331...04e01fa. Read the comment docs.

[csadorf]

See here: atztogo/spglib#93 (comment) for more information.

@hongyi-zhao Thank you for the contribution! Could you briefly elaborate as to why increasing the minimal required minor version of this dependency addresses the linked issue? I couldn't find anything regarding this in spglib's changelog.

Edit: Looks like this was just not mentioned: atztogo/spglib@54983ce

[csadorf]

@hongyi-zhao Could you please run this command: ./util/dependency_management.py generate-all and then push to this branch again?

See also the description here: https://github.com/aiidateam/aiida-core/wiki/AiiDA-Dependency-Management#updating-dependencies

[csadorf]

Please run this command ./util/dependency_management.py generate-all within the repository root and then push again. Thank you!

[sphuber]

@csadorf note that this is proposing to merge directly into master which we don't want. The branch should also be rebased on develop and merged into there

[sphuber]

The wrong base branch and failing tests put aside, is it really necessary to pin spglib==1.15.0. We currently have spglib~=1.14 which will allow 1.15 to be installed just fine. Unless there is a good explanation that we should pin the version, I am voting to close this.

[csadorf]

The wrong base branch and failing tests put aside, is it really necessary to pin spglib==1.15.0.

This PR does not propose to pin the dependency.

We currently have spglib~=1.14 which will allow 1.15 to be installed just fine. Unless there is a good explanation that we should pin the version, I am voting to close this.

If this solves a real-world issue with the installation then we should at least consider to increase the required minor version. I don't see much reason why ~1.15 should put much more stress on our environment solution compared to ~1.14 as it does not introduce new dependencies: https://github.com/atztogo/spglib/compare/v1.14.1..v1.15.0#diff-8cf6167d58ce775a08acafcfe6f40966R136

[csadorf]

@hongyi-zhao Please rebase this branch on develop. Thx!

[atztogo]

Probably I succeeded toI create wheels for v1.14.1.post0 of python 3.8 https://pypi.org/project/spglib/1.14.1.post0/#files. If this can be a tentative solution, we may be able to postpone this PR. The update of functionality in v1.15 over v1.14.1 is minor, and v1.14.1 may be safer because this has survived for long time.

[hongyi-zhao]

@atztogo Have you succeeded in this manner?

Regards

[hongyi-zhao]

@csadorf I give the power of making decision to @atztogo, due to he is the author of this package and that commit.

Regards

[greschd]

@hongyi-zhao does the install of 1.14.1.post0 now succeed on your machine, even when numpy is not installed first?

[hongyi-zhao]

@greschd It seems does the trick now:

(test) werner@ubuntu-01:~$ pip install spglib==1.14.1.post0
Looking in indexes: https://pypi.tuna.tsinghua.edu.cn/simple
Collecting spglib==1.14.1.post0
  Downloading https://pypi.tuna.tsinghua.edu.cn/packages/fb/67/7bedb4c3bbbfe863e64174d8d1e709205e16c7c8d502cc1355a51786258a/spglib-1.14.1.post0-cp38-cp38-manylinux2010_x86_64.whl (287kB)
     |████████████████████████████████| 296kB 1.9MB/s 
Collecting numpy (from spglib==1.14.1.post0)
  Downloading https://pypi.tuna.tsinghua.edu.cn/packages/ca/c6/cca531518aab1c161233c61e090728024aa647f2ff9c3b91d3f4e68e7e0e/numpy-1.18.3-cp38-cp38-manylinux1_x86_64.whl (20.6MB)
     |████████████████████████████████| 20.6MB 25.6MB/s 
Installing collected packages: numpy, spglib
Successfully installed numpy-1.18.3 spglib-1.14.1.post0
WARNING: You are using pip version 19.2.3, however version 20.0.2 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
(test) werner@ubuntu-01:~$ python -V
Python 3.8.2

[sphuber]

Just for some additional information. We have had similar issues before with pymatgen. It requires numpy in its setup procedure and when we specify requirements on numpy in aiida-core those are ignored when building pymatgen because those build requirements do not consider the rest of the requirements. Building pymatgen then would simply install the latest numpy which was no longer python 2 compatible, which would then fail our build which was running python 2. Our restriction on numpy was simply ignored.

[greschd]

As @sphuber mentioned upgrading to ~=1.15 is not strictly necessary, because new installs will go for that version anyway. I don't think it's necessary to force an install of the newer version -- obviously if spglib is already installed, there won't be an installation problem 😄

The published wheel fixes the issue also for 1.14.1.post0, but only until Python 3.9 is released; and not in case someone wants to build AiiDA with the --no-binary :all: option.

We could also decide to "vendor" the numpy setup requirement - i.e. add a pyproject.toml roughly as follows:

[build-system]
requires = ["setuptools >= 40.6.0", "wheel", "numpy"]
build-backend = "setuptools.build_meta"

That would solve the "missing numpy" problem for all our dependencies. Related issue: #3922

Note: There is some overlap between setup_requires specified in setuptools.setup and the requires field in pyproject.toml, I haven't figured out the exact difference yet. The latter seems to work better, at least if the package in question is imported in setup.py itself.

[hongyi-zhao]

Sebastiaan Huber <notifications@github.com> 于2020年4月20日周一 下午11:59写道：

Just for some additional information. We have had similar issues before with pymatgen. It requires numpy in its setup procedure and when we specify requirements on numpy in aiida-core those are ignored when building pymatgen because those build requirements do not consider the rest of the requirements. Building pymatgen then would simply install the latest numpy which was no longer python 2 compatible, which would then fail our build which was running python 2. Our restriction on numpy was simply ignored.

It seems I still have this issue when installing aiida-core 1.2 with $ pip install -e .[all] Which will invoke the installation of pymatgen. The solution I use now it firstly install the numpy with: $ pip install numpy Regards

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3944 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACVDTDT5PVXVYMM6A5DZ7T3RNRWOLANCNFSM4MLZV5VQ> .

-- Hongyi Zhao <hongyi.zhao@gmail.com>

[sphuber]

That would solve the "missing numpy" problem for all our dependencies.

Note that at least for the pymatgen story that I described, the problem was not that numpy was missing, it was exactly that numpy was not yet installed, pymatgen (one of our dependencies) needs it for its setup, and so package utils installs numpy first, but without considering our version requirements on numpy. So they ended up installing a version of numpy that was then incompatible with the rest of our requirements.

Edit: the work-around there was to then, before installing aiida-core, to first manually a version of numpy that would work for us. Then the pymatgen build would not try to install a newer one

[greschd]

@hongyi-zhao would you mind quickly checking if creating a pyproject.toml file at the root of the AiiDA repository with the content

[build-system]
requires = ["setuptools >= 40.6.0", "wheel", "numpy"]
build-backend = "setuptools.build_meta"

solves the issue with the pymatgen install?

[csadorf]

@sphuber @greschd You are correct. Increasing the required minor version is not strictly necessary.

@greschd I've played around with adding numpy to the build-requirements as part of trying to fix the installation process for Python 3.5 and that did in fact not solve the problem with pymatgen. So I am not sure whether this would even be effective.

Concerning the overlap between setup_requires and the pyproject.toml: The pyproject.toml is only respected by newer versions of setuptools and actually resolves the issue that a build-system dependency cannot be required as part of the build process (executing setup()).

[hongyi-zhao]

Dominik Gresch <notifications@github.com> 于2020年4月21日周二 上午12:11写道：

@hongyi-zhao would you mind quickly checking if creating a pyproject.toml file at the root of the AiiDA repository with the content [build-system] requires = ["setuptools >= 40.6.0", "wheel", "numpy"] build-backend = "setuptools.build_meta" solves the issue with the pymatgen install?

As Carl Simon Adorf told that this method cannot solve the problem for all python versions. Should you think I should test it now? Regards

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

-- Hongyi Zhao <hongyi.zhao@gmail.com>

[greschd]

@csadorf That's interesting... I just tried creating a minimal package with setup.py

#!/usr/bin/env python

import setuptools
from setuptools import find_packages

import numpy
assert numpy.__version__ >= "1.18"

if __name__ == "__main__":
    setuptools.setup(
        setup_requires=["numpy>=1.18"],
        install_requires=["numpy<1.18"],
        find_packages=find_packages()
    )

and pyproject.toml

[build-system]
requires = ["setuptools >= 40.6.0", "wheel", "numpy>=1.18"]
build-backend = "setuptools.build_meta"

This works, in that it installs >=1.18 for the build, and then 1.17 into the actual environment.

[csadorf]

@greschd Maybe I don't fully remember what the actual issue was. We eventually resolved by explicitly updating the setuptools to the required version for Python 3.5:

aiida-core/.github/workflows/test-install.yml

Lines 136 to 138 in e33504c

	- name: upgrade setuptools [py35]
	if: matrix.python-version == 3.5
	run: pip install -I setuptools==38.2.0

So fewer problems with newer version of setuptools. This is the related PR: #3771

[greschd]

Yeah I don't know since when setuptools can do these isolated build environments, but it may well be more recent than Py3.5

[csadorf]

It seems to me that this problem is in fact resolved upstream, unless there are any objections, I suggest to close this PR and also the related issue.