search

MDAnalysis / membrane-curvature

MDAnalysis tool to calculate membrane curvature.
https://membrane-curvature.readthedocs.io/
GNU General Public License v3.0
29 stars 6 forks source link

MDA>=2.0.0 version added to `install_requires` #49

Closed ojeda-e closed 3 years ago

ojeda-e commented 3 years ago

This PR fixes #46

@IAlibay, would you please confirm no more changes are needed here at the moment?

pep8speaks commented 3 years ago

Hello @ojeda-e! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-07-23 02:41:25 UTC
codecov[bot] commented 3 years ago

Codecov Report

Merging #49 (a241d99) into main (19d8a24) will not change coverage. The diff coverage is n/a.

ojeda-e commented 3 years ago

Thanks for your comments @tylerjereddy and @orbeckst. Although tests passed with Python>=3.7, I added Python=3.6 and numpy>=1.16.0 so the requirements will be consistent with those in MDAnalysis.

ojeda-e commented 3 years ago

Here is the summary of what I found (thanks @IAlibay for the guidance): According to NEP29, a project should work with

According to this table from NEP29, the minimum NumPy version supported for MembraneCurvature should be Python>=3.6 and Numpy>=1.15.

It also says: "In setup.py, the python_requires variable should be set to the minimum supported version of Python. All supported minor versions of Python should be in the test matrix and have binary artifacts built for the release."

Now, following NEP29 would mean I have to add all the 12 minor versions of python which seems a bit excessive at this point. Also according to NEP29, I would have to use Numpy>=1.15, which makes me hesitate about what to do since MDAnalysis currently set a higher version minimum requirement.

I tried all the options that occurred to me to cover different versions of NumPy in the tests to check they work. So I failed and I'm giving up at this point.

In Python=3.6, the tests are running and passing with NumPy=1.19.5; In Python=3.7, tests are passing with Numpy=1.21.1 .So I am setting Numpy>=1.19.5 and Python>=3.6, and I will ignore NEP29 for now. I still don't understand why if install_requires is enough, then the min version of Numpy is not the same across versions of Python.

Sorry for the annoying notifications @orbeckst @tylerjereddy @IAlibay @lilyminium , I cleaned the mess.

lilyminium commented 3 years ago

I believe that that support table is the minimum versions that numpy will continue to support, instead of the release schedule. As we're now in July 2021, that would soon be Python 3.7+ and 1.18+. MDAnalysis tends to take a longer view of these things -- our last Python 2.7-supporting release was sometime this year, an entire year after the Python 2 end of life. There was some discussion about this on Discord in June (probably easier to read on the mailing list too -- if you're not subscribed already, it's a great way to track code-related discussions).

I think you are right that you can probably drop numpy 1.16 and 1.17 in following NEP29 (I believe 1.16 was required for MDAnalysis 1.0 for the 2.7 support). 1.19.5 is a bit far, though -- do your tests pass with lower versions? Usually having the most lax requirements possible is best, because that gives users more flexibility in building their Python environments.

Now, following NEP29 would mean I have to add all the 12 minor versions of python which seems a bit excessive at this point. I tried all the options that occurred to me to cover different versions of NumPy in the tests to check they work.

That's very thorough of you! Would you mind elaborating on what you tried and didn't work? These things are always good to know for the future.

However, I'm not sure this much testing is necessary. I guess I think of the tests more as a way to make sure that I'm not using features that are too new, or old features that have been removed. So for the former I would just check that I can run my tests with my minimum numpy version, e.g. 1.16, and minimum Python version, e.g. 3.6. For the latter, I keep an eye out for deprecation warnings and eventually the tests will fail. For example, if you are testing Python 3.9, only numpy 1.20+ is supported; so if tests suddenly start failing here, then I know something might have changed in numpy 1.20. @MDAnalysis/gsoc-mentors please do chip in if you disagree or have more to add.

Also, in semantic versioning, the versions go MAJOR.MINOR.PATCH. So for Python 3.7.1, the major version is 3, the minor version is 7, and the patch version is 1. Therefore when it says "All supported minor versions of Python should be in the test matrix", there are only four between [3.6, 3.7, 3.8, 3.9]. I think you're already testing all of these!

ojeda-e commented 3 years ago

Thanks for the comments @lilyminium Yes, my bad I didn't elaborate on the issues. My limitation is to test the lowest version of NumPy and different Python versions simulatenously. To clarify, individual tests pass, but what I couldn't manage to do is installing more than one specific version of NumPy for the tests in combination with the matrix of Python. I used different approaches, and in some of them I think MDAnalysis rewrites the version of NumPy when it is installed, and when I go through non very orthodox approaches like the one here below, there is a side effect in the installation of MDAanalysis. For instance, something as you suggested (tried retrieving the comment but can't find it), slightly modified:

      shell: bash -l {0}
      run: |
        python -m pip install MDAnalysis==2.0.0b0
        python -m pip unistall -y numpy
        python -m pip install numpy=={{ NUMPY_VERSION }}
        python -m pip install . --no-deps
        conda list

it doesn't work. The error suggests MDAnalysis is not properly installed (I didn't reproduce this because it's ugly).

Now, if I try to use a matrix for the version of NumPy, to have Python>=3.6 and Numpy=1.16.0, as in the same style as MDAnalysis, I got this mamba error

Looking for: ['numpy=1.16.0', 'pip', 'pytest', 'mmtf-python', 'biopython', 'networkx', 'cython', 'matplotlib-base', 'scipy', 'griddataformats', 'hypothesis', 'gsd', 'codecov', 'threadpoolctl']

Pinned packages:
  - python 3.8.5

Encountered problems while solving:
  - package numpy-1.16.0-py37_blas_openblash1522bff_1000 requires python >=3.7,<3.8.0a0, but none of the providers can be installed

With that error (And excluding the fact that windows is windows and it fails), I said, oh ok, so maybe I do have a min version of Python. So first, changed to 3.7 min, even if that would contradict tests from MDAnalysis (because they pass with 3.6). So,

Pinned packages:

Encountered problems while solving:

So this is the summary of how I gave up and ended up with Numpy=1.19.5. Also, Thanks for this

For example, if you are testing Python 3.9, only numpy 1.20+ is supported; so if tests suddenly start failing here, then I know something might have changed in numpy 1.20.

tylerjereddy commented 3 years ago

There's also something to be said for using pyproject.toml these days instead of leaning too heavily on setup.py for some things, but I can't remember if even core MDA has started down that path just yet.

i.e., https://setuptools.readthedocs.io/en/latest/build_meta.html

That said, you could spend all your time trying to get packaging right, bit of a pain in Python still..

IAlibay commented 3 years ago

There's also something to be said for using pyproject.toml these days instead of leaning too heavily on setup.py for some things, but I can't remember if even core MDA has started down that path just yet.

Not yet (as far as I know). @tylerjereddy at some point I'd like to pick your brains about all the potential changes to core MDA's setup that you would recommend. I've been thinking it might be a good idea to see if we can get an NF small development grant to do an overhaul of setup + automated weekly/monthly dev builds.

That said, you could spend all your time trying to get packaging right, bit of a pain in Python still..

In my opinion here, we can probably defer this to a new issue / future work, given that:

a) for now we are following the CMS cookiecutter (which hasn't adopted things like pyproject.toml) b) we aren't overly concerned about releases yet (but rather just making sure that CI works).