Skip to content

Drop distutils #3439

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

Closed
Tracked by #3457
effigies opened this issue Mar 21, 2022 · 7 comments · Fixed by #3458
Closed
Tracked by #3457

Drop distutils #3439

effigies opened this issue Mar 21, 2022 · 7 comments · Fixed by #3458
Milestone

Comments

@effigies
Copy link
Member

Summary

distutils is deprecated. Here is the output of grep -rI distutils . in the nipype root:

./doc/changelog/0.X.X-changelog.rst:* ENH: Abandon distutils, only use setuptools (https://github.com/nipy/nipype/pull/1627)
./nipype/interfaces/afni/base.py:from distutils import spawn
./nipype/interfaces/dipy/preprocess.py:from distutils.version import LooseVersion
./nipype/interfaces/dipy/reconstruction.py:from distutils.version import LooseVersion
./nipype/interfaces/dipy/registration.py:from distutils.version import LooseVersion
./nipype/interfaces/dipy/setup.py:    from numpy.distutils.misc_util import Configuration
./nipype/interfaces/dipy/setup.py:    from numpy.distutils.core import setup
./nipype/interfaces/dipy/stats.py:from distutils.version import LooseVersion
./nipype/interfaces/dipy/tracks.py:from distutils.version import LooseVersion
./nipype/interfaces/mrtrix3/connectivity.py:            from distutils.spawn import find_executable
./nipype/interfaces/niftyreg/base.py:from distutils.version import StrictVersion
./nipype/interfaces/slicer/generate_classes.py:    from numpy.distutils.misc_util import Configuration
./nipype/interfaces/slicer/generate_classes.py:    from numpy.distutils.core import setup
./nipype/utils/misc.py:from distutils.version import LooseVersion
./nipype/utils/misc.py:        distutils.version.LooseVersion.
./nipype/utils/config.py:from distutils.version import LooseVersion
./nipype/__init__.py:from distutils.version import LooseVersion
./tools/toollib.py:from distutils.dir_util import remove_tree

We can break this down into groups:

  • LooseVersion - We import this directly from distutils in many places and also import it into nipype.__init__.py from which it is imported in many other places. The semantics of LooseVersion are not the same as packaging.version.Version, although in most places it should generally be close enough. Are we okay with a blanket replace?
  • StrictVersion - Probably safe to replace with Version?
  • distutils.spawn.find_executable - We can use shutil.which
  • numpy.distutils - Used in dipy/setup.py and files generated by slicer/generate_classes.py; I don't understand this use case so I'm hesitant to do anything.
  • distutils.dir_util.remove_tree - I don't think tools/toollib.py is part of anybody's workflow. Easier just to remove? Alternately, can use shutil.rmtree.

Assigning to 1.8 so that there will be a semver hurdle.

@effigies effigies added this to the 1.8.0 milestone Mar 21, 2022
@effigies effigies mentioned this issue Apr 27, 2022
11 tasks
@effigies
Copy link
Member Author

@satra I think I'm going to need your input on the above questions.

@satra
Copy link
Member

satra commented Apr 27, 2022

your plan seems good on all accounts. if necessary, we could probably concoct our own Version function. i wrote one that covers simple versioning.

regarding the slicer generate classes, it should probable move into tools. it is use to generate the python classes from slicer's executables. but since we have not run it in a while, i think fine to make a note and update later.

@ghisvail
Copy link
Contributor

ghisvail commented May 6, 2022

Since nipype 1.8.0 is targeting Python 3.7, why not just use importlib.metadata (there is a backport module for 3.7) and forget about maintaining your own version class?

@effigies
Copy link
Member Author

effigies commented May 6, 2022

Because we use LooseVersion to compare the versions of the tools being wrapped by nipype.

@ghisvail
Copy link
Contributor

ghisvail commented May 6, 2022

I see, and the packaging module does not cut it?

distutils is the legacy of the sad packaging story Python has had for a while. I am really not sure it's worth vendoring in any code base.

@effigies
Copy link
Member Author

effigies commented May 6, 2022

Strict versions may be brittle for projects/languages that don't follow PEP-440, and packaging is deprecating their LegacyVersion class that is much more forgiving. I would not intend to vendor distutils as a whole, just LooseVersion.

Alternately, I suppose we could vendor LegacyVersion, but LooseVersion does nicely document its semantics and it's a more well-known quantity in the community.

@ghisvail
Copy link
Contributor

ghisvail commented May 6, 2022

I would not intend to vendor distutils as a whole, just LooseVersion.

I understand, but I worry vendoring it in nipype will not incentivise downstream projects to move away from this deprecated piece of code.

I guess It depends whether the version class exposed by nipype is considered part of its core API or just a utility.

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 a pull request may close this issue.

3 participants