murraylab / PsychRNN

https://psychrnn.readthedocs.io
MIT License
133 stars 42 forks source link

fix invalid specifier that was breaking example notebooks & pip-installing from GitHub/sdist. Also update readthedocs configuration files and remove testing on python 2.7 due to breaking changes in readthedocs and GitHub actions. #49

Closed paxtonfitzpatrick closed 11 months ago

paxtonfitzpatrick commented 11 months ago

Hi all, thanks for creating such a useful package!

Noticed during @johndmurray's guest lecture in torwager/ComputationalFoundations that the Minimal Example and Simple Example Colab notebooks are broken. Installing PsychRNN from the GitHub repo in the first cell fails because the version specifier after "python_version" in this line is invalid: https://github.com/murraylab/PsychRNN/blob/2f5415534de786b2901fd3b5914d94c94a0f68fa/setup.py#L29

#

The reason the example notebooks were working fine previously (and regular pip install PsychRNN is still working for now) is actually a little convoluted --

When building a package from a source distribution (e.g., the GitHub repo), old versions of setuptools (prior to v61.0) would simply ignore invalid specifiers rather than throwing an error. The Colab VM had an older setuptools version installed back when the example notebooks were written, so at that point the invalid specifier in PsychRNN/setup.py was just getting ignored. But when Colab's setuptools installation was finally updated back in March 2023 (it's now at v67.7.2), the example notebooks suddenly broke because that line in setup.py started preventing the package from being successfully built from the repo.

The reason that ignoring the invalid specifier like setuptools used to do actually caused things to work right is kind of a happy accident. Ignoring the specifier makes that line in setup.py read:

install_requires=['python_version', 'tensorflow'] 

Items passed to install_requires are names of packages that the current package depends on. So rather than specifying what version(s) of Python itself the PsychRNN package requires (which I think was the intent based on the README), passing 'python_version' to install_requires actually means "This package requires a package named python_version".

Just by chance, there actually happens to be a package named "python_version" available on PyPI, so PsychRNN has been installing the "python_version" package as a dependency rather than checking for the Python versions it actually requires.

Finally, the reason you can still successfully install PsychRNN from PyPI with pip install PsychRNN for the moment is that the package is available on PyPI as a binary distribution (a.k.a. "wheel") that was pre-built with an old setuptools version back when it was uploaded in Jan. 2021. Installing this built distribution doesn't means setup.py doesn't have to be parsed again to build the package after downloading it, which is the step that's failing. (Though it will fail if you try to install the source distribution with pip install --no-binary :all: PsychRNN). Importantly however, this will also stop working beginning in Jan. 2024. The next major release of pip (v24.0) will start enforcing valid specifiers just like setuptools v61.0 did, a new release will need to be pushed to PyPI for users to continue to be able to install the package (with up-to-date pip versions).

#

This PR is a minimal change but should fix all of these issues.

Note the python_requires argument added to setup.py does technically impose minimum version requirements on both setuptools (>=24.2.0) and pip (>=9.0.0), however both of these versions were released >7 years ago, and any Python versions that could possibly still be using anything older are long since EOL.

Thanks again, and hope this is helpful!

syncrostone commented 11 months ago

Thank you @paxtonfitzpatrick for the fix and the explanation!

I made some changes due to changes in readthedocs and the GitHub python testing infrastructure, and as soon as all the tests pass, I will merge it in!

For the collab linked from readthedocs to update after that, I will need to push the new PsychRNN version to PyPi, which may take me some time to get to.

In the meantime, manually changing the line first cell in the Colab notebooks that read !pip install git+https://github.com/murraylab/PsychRNN@$blob to !pip install git+https://github.com/paxtonfitzpatrick/PsychRNN@master should do the trick

codecov[bot] commented 11 months ago

Codecov Report

Merging #49 (1822059) into master (2f54155) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   78.69%   78.69%           
=======================================
  Files          14       14           
  Lines         887      887           
=======================================
  Hits          698      698           
  Misses        189      189