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

MNT: Drop Python 3.6, test setuptools builds, pip installations #593

Merged
merged 5 commits into from
Dec 16, 2020

Conversation

effigies
Copy link
Member

As noted in other discussions, setuptools is aiming to be an installation backend, not an installer, and has no plans to implement various installation PEPs, such as respecting the PythonRequires metadata exposed by PyPI.

Therefore I think the recommended installation method needs to be pip. Since we distribute sdists and wheels, we should continue to test those, and those are built with setuptools and wheel, but we do not need to test against old versions.

We may want to test against old versions of pip again, but I think after a year or two of using pyproject.toml, we're pretty comfortable that pip can fetch installation dependencies as needed for this project and its dependencies.

@effigies effigies force-pushed the ci/drop_setuptools_tests branch from 8d8d940 to e6006f5 Compare December 15, 2020 20:59
@oesteban
Copy link
Member

Since Numpy is dropping Python 3.6, should we follow suit and make our lives easier?

(BTW, the failed test is unrelated to these changes, obviously - I'm getting a similar error on sdcflows proper)

@effigies
Copy link
Member Author

Yup, I'm okay with us following numpy's lead in NEP29.

For reference NiBabel and Nipype are on NEP29 + 1 year, and PyBIDS is just going with Python EOL at this point. But as mostly user-facing tools with containerized installations, there's no strong need for nipreps to outlast numpy.

@effigies effigies changed the title CI: Test setuptools builds, pip installations MNT: Drop Python 3.6, test setuptools builds, pip installations Dec 16, 2020
@oesteban
Copy link
Member

The failing test is because scikit-image. Docker build log:

ERROR: scikit-image 0.18.0 has requirement numpy>=1.16.5, but you'll have numpy 1.15.4 which is incompatible.

Testing an update on nipreps/sdcflows#158

@effigies
Copy link
Member Author

Yeah, let's update the numpy/scipy/scikit-learn dependencies.

Should I use the versions you're using over there?

@oesteban
Copy link
Member

Should I use the versions you're using over there?

I think so. I'm trying to find the least annoying way of installing scikit-image. It seems that pip will be the fastest option (conda-forge also attempts to change many package's versions/builds).

I have also bumped setuptools and pip inside the Dockerimage - we probably want to unpin setuptools and use the latest at the time is cleaned up.

@oesteban
Copy link
Member

I'm also thinking that we could be more flexible with pinning and do not force patch releases. That should make the installation less breakable (assuming packages do not break dependency tables on patch release changes, which seems fairly safe to assume).

Then, when releasing, we could clean up the conda layer to force a fresh installation (or have a cron job do this).

@oesteban
Copy link
Member

Okay, done on SDCFlows. Have the dependency be resolved by pip was the best option. Pinned:
numpy-1.19
scikit-learn-0.19
scipy-1.5

(https://github.com/nipreps/sdcflows/blob/7c81f25e90c79453570c27e9d00cac96f59d4f72/Dockerfile#L83-L102)

@oesteban
Copy link
Member

Do you want to include that in this PR?

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #593 (4d862a0) into master (d50f7ae) will decrease coverage by 1.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
- Coverage   47.59%   46.44%   -1.15%     
==========================================
  Files          44       44              
  Lines        5356     5352       -4     
  Branches      785      781       -4     
==========================================
- Hits         2549     2486      -63     
- Misses       2714     2782      +68     
+ Partials       93       84       -9     
Flag Coverage Δ
documentation 0.00% <ø> (ø)
masks ?
reportlettests 100.00% <ø> (ø)
unittests 46.64% <ø> (ø)

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

Impacted Files Coverage Δ
niworkflows/__init__.py 0.00% <0.00%> (-81.82%) ⬇️
niworkflows/func/util.py 21.17% <0.00%> (-47.06%) ⬇️
niworkflows/interfaces/fixes.py 50.00% <0.00%> (-5.56%) ⬇️
niworkflows/interfaces/utils.py 40.14% <0.00%> (-2.38%) ⬇️
niworkflows/interfaces/report_base.py 47.54% <0.00%> (-1.64%) ⬇️
niworkflows/viz/utils.py 9.52% <0.00%> (+0.02%) ⬆️
niworkflows/interfaces/mni.py 25.86% <0.00%> (+0.14%) ⬆️
niworkflows/interfaces/images.py 66.15% <0.00%> (+0.16%) ⬆️
niworkflows/utils/bids.py 75.00% <0.00%> (+0.74%) ⬆️

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 d50f7ae...4d862a0. Read the comment docs.

@oesteban oesteban merged commit b631029 into nipreps:master Dec 16, 2020
mgxd added a commit to mgxd/niworkflows that referenced this pull request Jan 7, 2021
@effigies effigies deleted the ci/drop_setuptools_tests branch June 20, 2023 11:52
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