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

Unpin numpy version from 1.17.X to allow the newest version (1.19) #4378

Merged
merged 6 commits into from
Sep 19, 2020

Conversation

CasperWA
Copy link
Contributor

This is an attempt to try and unpin the hard constraint on numpy.

This was introduced in f5d6cba, the reason for it can be seen in the commit message.

I have tried following the wiki page on how to update dependencies, and am currently trying the semi-automatic update of the requirements files by creating this PR.

@CasperWA CasperWA added dependencies Pull requests that update a dependency file pr/work-in-progress PR that is still work in progress but already needs discussion priority/important topic/dependencies/constraint Issues related to dependency constraints that should be resolved. labels Sep 17, 2020
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to our current compatibility timeline we must support numpy version 1.17 until 26-Jul-2021. The compatiblity timeline is derived from AEP 003.

Please note that the 26-Jul-2021 date is not explicitly mentioned in AEP 003, but required to be compatible with NEP 29.

Edit: This means the requirement should be specified as: numpy~=1.17.

@CasperWA CasperWA changed the title Unpin numpy version from 1.17.X to >=1.19 Unpin numpy version from 1.17.X to allow the newest version (1.19) Sep 17, 2020
@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #4378 into develop will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4378      +/-   ##
===========================================
+ Coverage    79.33%   79.33%   +0.01%     
===========================================
  Files          468      468              
  Lines        34755    34757       +2     
===========================================
+ Hits         27568    27571       +3     
+ Misses        7187     7186       -1     
Flag Coverage Δ
#django 72.96% <100.00%> (+0.01%) ⬆️
#sqlalchemy 72.16% <100.00%> (-<0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/tools/data/array/kpoints/legacy.py 31.40% <100.00%> (+0.20%) ⬆️
aiida/engine/daemon/runner.py 82.76% <0.00%> (+3.45%) ⬆️

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 c6bca06...0da7e1b. Read the comment docs.

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the requirements/*.txt files according to the following table:

  • 3.5: numpy==1.18.5
  • 3.6: numpy==1.19.2
  • 3.7: numpy==1.19.2
  • 3.8: numpy==1.19.2

.circleci/config.yml Outdated Show resolved Hide resolved
@csadorf
Copy link
Contributor

csadorf commented Sep 17, 2020

It appears that there is an actual incompatibility:

tests/test_dataclasses.py:3203: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
aiida/tools/data/array/kpoints/__init__.py:87: in get_explicit_kpoints_path
    return method(structure, **kwargs)
aiida/tools/data/array/kpoints/__init__.py:216: in _legacy_get_explicit_kpoints_path
    cell=structure.cell, pbc=structure.pbc, **kwargs
aiida/tools/data/array/kpoints/legacy.py:353: in get_explicit_kpoints_path
    numpy.linspace(ini_coord[0], end_coord[0], num_points[count_piece]),
<__array_function__ internals>:6: in linspace
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

My assumption is that the third argument in the call to numpy.linspace is a float instead of an integer here:

numpy.linspace(ini_coord[0], end_coord[0], num_points[count_piece]),

@CasperWA
Copy link
Contributor Author

It appears that there is an actual incompatibility:

tests/test_dataclasses.py:3203: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
aiida/tools/data/array/kpoints/__init__.py:87: in get_explicit_kpoints_path
    return method(structure, **kwargs)
aiida/tools/data/array/kpoints/__init__.py:216: in _legacy_get_explicit_kpoints_path
    cell=structure.cell, pbc=structure.pbc, **kwargs
aiida/tools/data/array/kpoints/legacy.py:353: in get_explicit_kpoints_path
    numpy.linspace(ini_coord[0], end_coord[0], num_points[count_piece]),
<__array_function__ internals>:6: in linspace
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

My assumption is that the third argument in the call to numpy.linspace is a float instead of an integer here:

numpy.linspace(ini_coord[0], end_coord[0], num_points[count_piece]),

Yes, already looked into this and just pushed a possible fix.

@CasperWA
Copy link
Contributor Author

Please update the requirements/*.txt files according to the following table:

  • 3.5: numpy==1.18.5
  • 3.6: numpy==1.19.2
  • 3.7: numpy==1.19.2
  • 3.8: numpy==1.19.2

Just to understand. Did you write this comment manually or is this part of the semi-automatic workflow?

@csadorf
Copy link
Contributor

csadorf commented Sep 17, 2020

Please update the requirements/*.txt files according to the following table:

  • 3.5: numpy==1.18.5
  • 3.6: numpy==1.19.2
  • 3.7: numpy==1.19.2
  • 3.8: numpy==1.19.2

Just to understand. Did you write this comment manually or is this part of the semi-automatic workflow?

I checked those manually. The automated workflow creates a PR if and only if the tests succeed and also only if the requirements files are outdated, which with the change to numpy~=1.17 they actually are not. So we actually don't have to change them and now that I think about it, maybe we shouldn't.

@CasperWA CasperWA marked this pull request as ready for review September 17, 2020 14:04
@CasperWA CasperWA added pr/ready-for-review PR is ready to be reviewed and removed pr/work-in-progress PR that is still work in progress but already needs discussion labels Sep 17, 2020
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Codecov is complaining about the coverage, I'm ok to just ignore that, although we might need to check with someone else whether we can override that check.

But before we do that, maybe you can give it a 5min try of providing the faulty input to cover the section?

aiida/tools/data/array/kpoints/legacy.py Outdated Show resolved Hide resolved
For newer versions of numpy, the entries in num_points are of type
numpy.float64, and need to be integers so they can be used for indexing
as num in numpy.linspace().
aiida/tools/data/array/kpoints/legacy.py Outdated Show resolved Hide resolved
@CasperWA
Copy link
Contributor Author

@csadorf and @sphuber This is ready for a re-review + merge. Your old review comments have all been addressed.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, thanks @CasperWA

@sphuber sphuber merged commit 12be9ad into develop Sep 19, 2020
@sphuber sphuber deleted the unpin_numpy branch September 19, 2020 09:16
chrisjsewell added a commit to marvel-nccr/ansible-role-aiida that referenced this pull request Oct 13, 2020
Also drop sperate numpy install , since this should no longer be needed, in accordance with: aiidateam/aiida-core#4378
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file pr/ready-for-review PR is ready to be reviewed priority/important topic/dependencies/constraint Issues related to dependency constraints that should be resolved. topic/dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants