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 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

Merged
merged 10 commits into from
Nov 5, 2023

Conversation

paxtonfitzpatrick
Copy link
Contributor

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:

install_requires=['python_version>="2.7",!="3.0*",!="3.1*",!="3.2*",!="3.3*"', 'tensorflow']

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!

paxtonfitzpatrick and others added 10 commits November 3, 2023 23:19
Read the docs requires this starting Sep 25 2023 (https://blog.readthedocs.com/migrate-configuration-v2/)
Migrate python requirements from readthedocs settings
specify conf.py location
remove python 2.7 as it has been deprecated due to EOL
sphinx_rtd_theme no longer installed by default.
pin sphinx rtd theme for stability.
try to get python setup.py install to run for readthedocs
try to get setup.py install working again.
@syncrostone
Copy link
Collaborator

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

Copy link

codecov bot commented Nov 5, 2023

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           

@syncrostone syncrostone changed the title fix invalid specifier that was breaking example notebooks & pip-installing from GitHub/sdist 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. Nov 5, 2023
@syncrostone syncrostone merged commit aff98e1 into murraylab:master Nov 5, 2023
24 checks passed
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.

2 participants