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 2 support #2654

Merged
merged 42 commits into from
Nov 13, 2019
Merged

MNT: Drop Python 2 support #2654

merged 42 commits into from
Nov 13, 2019

Conversation

effigies
Copy link
Member

@effigies effigies commented Jul 26, 2018

Summary

This PR is a notification that I've started a dev/2.0 branch, and perhaps a place to centralize discussion.

It's already basically unreadable, due to the changes to make specs, so if anybody wants to review, I'd recommend doing it commit-by-commit. Once we decide that this is okay, we then direct any further work to PRs against this branch (e.g. #2629 and #2648).

I'd also suggest periodically opening a PR to merge master into dev/2.0, perhaps at release time, so that 1.x improvements make it in.

List of changes proposed in this PR (pull-request)

  • Remove Python 2 compatibility shims
  • Remove Python 2 tests in CI
  • Remove interfaces.ants.legacy
  • Re-run make specs

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@effigies effigies added this to the Nipype 2.0 milestone Jul 26, 2018
@codecov-io
Copy link

codecov-io commented Jul 26, 2018

Codecov Report

Merging #2654 into master will decrease coverage by 0.31%.
The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2654      +/-   ##
==========================================
- Coverage   68.14%   67.83%   -0.32%     
==========================================
  Files         297      295       -2     
  Lines       39810    39255     -555     
  Branches     5218     5171      -47     
==========================================
- Hits        27130    26627     -503     
+ Misses      11968    11919      -49     
+ Partials      712      709       -3
Flag Coverage Δ
#smoketests 51.22% <82.14%> (-0.58%) ⬇️
#unittests 65.05% <86.2%> (-0.33%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/ants/base.py 60% <ø> (-1.71%) ⬇️
nipype/interfaces/afni/preprocess.py 81.93% <ø> (-0.05%) ⬇️
nipype/__init__.py 58.06% <ø> (-3.85%) ⬇️
nipype/interfaces/diffusion_toolkit/postproc.py 80.64% <ø> (-0.61%) ⬇️
nipype/interfaces/afni/model.py 86.93% <ø> (-0.08%) ⬇️
nipype/interfaces/camino/convert.py 71.92% <ø> (-0.11%) ⬇️
nipype/interfaces/camino2trackvis/convert.py 68% <ø> (-0.63%) ⬇️
nipype/interfaces/bru2nii.py 68.96% <ø> (-1.04%) ⬇️
nipype/interfaces/afni/utils.py 81.84% <ø> (-0.05%) ⬇️
nipype/interfaces/ants/segmentation.py 74.39% <ø> (-0.1%) ⬇️
... and 183 more

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 5e9d9b5...39fbd54. Read the comment docs.

@satra
Copy link
Member

satra commented Aug 13, 2018

@effigies - since 2.0 is python 3 only, a lot of current code (e.g., use of future) can be removed/simplified. but this will also make bringing fixes in from 1.0 require some changes. we should perhaps not merge master, but cherry pick commits from master into a new PR against dev/2.0 (where a clean merge is possible that would be fine with me).

another thought is whether to break off the engine, interfaces into their own separate projects. we can start with the engine, since i think that is the most self-contained component.

fi
- persist_to_workspace:
root: /tmp
paths:
- docker

test_py3_fmri_fsl_spm:
test_fmri_fsl_spm:
Copy link
Member

Choose a reason for hiding this comment

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

should we consider breaking off the workflows and workflow tests to niflows, before we break off 2.0?

Copy link
Member

Choose a reason for hiding this comment

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

i'm also fine with making all the breaks with 2.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to start moving things to niflows. Have we figured out a working plan for that? Maybe could do a chat sometime this week to iron out a rough plan?

Copy link
Member

Choose a reason for hiding this comment

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

i'll coordinate a time.

@@ -339,21 +316,15 @@ jobs:
- run:
name: Check pypi preconditions
command: |
pyenv local 3.5.2
pip install --upgrade twine future wheel readme_renderer setuptools
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we remove future?

@satra
Copy link
Member

satra commented Aug 13, 2018

@effigies - current changes look good to me.

@effigies
Copy link
Member Author

we should perhaps not merge master, but cherry pick commits from master into a new PR against dev/2.0

The reason I like merging master is that it marks changes as already included, which makes the next merge off master simpler. I am doing a git merge --no-commit and curating, at this point. In the future, we could split some branch off master that we then PR into dev/2.0, and can do any cleanups there. Maybe we can try that after 1.1.3?

@effigies
Copy link
Member Author

And thanks for the review. I'll make the future change you suggested, and then we can start going through the PR process for this branch instead.

@satra
Copy link
Member

satra commented Aug 13, 2018

@effigies - merging master is fine, as long as we are curating.

@satra
Copy link
Member

satra commented Aug 21, 2019

@effigies, @oesteban - should the following feature set be nipype 1.(next + 1) or 2?

  • py3 only
  • separate out interface packages as different repos

i.e do we make this clean break with pydra-based nipype or earlier?

@effigies
Copy link
Member Author

I would be happy to drop official Py2 support before 2.0. This branch is almost entirely that, and...

Screen Shot 2019-08-20 at 22 04 32

My feeling is that if it's mostly done and would make 1.x maintenance easier overall (to whatever extent that will need to keep happening), it's worth doing.

Which purging Python 2 is. Separating out interface packages isn't really, so I'd lean toward making that part of 2.0.

@effigies
Copy link
Member Author

@satra I interpreted "nipype 1.(next + 1)" as 1.4.0. Is this correct, and do you have a timeline? Perhaps early January, to match Python 2.7 EOL?

@satra
Copy link
Member

satra commented Sep 18, 2019

@effigies - let's say 1.4 will be released mid december, with master branch moving from supporting both to pure py3 starting after the next release 1.3 (which is scheduled soon, correct?) we should make a release branch for any specific hot fixes that should go in to a 1.3.x release

@effigies
Copy link
Member Author

effigies commented Sep 18, 2019

@satra Sounds good to me. Including that, this would be the current schedule

Release Date Note
1.2.3 September 23 Final 3.4 release
1.3.0 October 28 Final 2.7 series
1.3.1 November 25 1.3.0 + backported bug fixes only
1.4.0 December 16 Python 3.5+, all new features post-1.3.0

The 1.3.1 would be contingent on there being bugs to fix, although we rarely go a month without...

@effigies effigies changed the title [WIP] Nipype 2.0 development branch [WIP] MNT: Drop Python 2 support Sep 20, 2019
@effigies effigies modified the milestones: Nipype 2.0, 1.4.0 Sep 20, 2019
@oesteban
Copy link
Contributor

oesteban commented Oct 8, 2019

Do you need a review on this?

@effigies
Copy link
Member Author

effigies commented Oct 8, 2019

Not really, though you're welcome to. We'll do another sync after the final 1.3.0 release, and then we can merge in, so it might make more sense to wait on that.

@effigies effigies changed the title [WIP] MNT: Drop Python 2 support MNT: Drop Python 2 support Nov 12, 2019
@effigies
Copy link
Member Author

@satra Can you verify the latest merge commit? If that's good, this should be good to merge.

@satra
Copy link
Member

satra commented Nov 12, 2019

looks good.

@effigies effigies merged commit 10169f3 into master Nov 13, 2019
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.

4 participants