ENH: Reduce within-package absolute imports #2059

effigies · 2017-06-01T17:03:36Z

With our (FMRIPREP/MRIQC) mismatched release cycles, we're running into the situation where correctly managing dependencies puts an undue burden on users (at least those installing natively, rather than running the Docker/Singularity images). We're attempting to move to include nipype as a submodule in niworkflows. Our projects will then import from niworkflows.nipype, which will avoid clashing with other installs of nipype.

Absolute imports assume that the root module is named nipype, where relative imports will permit (relatively) easy use as a submodule.

This should not affect the actual behavior of nipype at all, but will require that future PRs refrain from using absolute imports.

Changes proposed in this pull request:

Move to relative imports in non-testing code

chrisgorgo · 2017-06-01T17:20:38Z

pycharm is showing me many more occurences of "from nipype" in non-testing code:

effigies · 2017-06-01T17:23:14Z

The slicer modules are auto-generated. It looks like the generating script has been updated, I just didn't take the time to figure out how to run it.

Edit: And I believe the other instances are in doctests or functions that import locally. I think those have to be absolute.

codecov-io · 2017-06-01T18:58:19Z

Codecov Report

Merging #2059 into master will increase coverage by <.01%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master    #2059      +/-   ##
==========================================
+ Coverage   72.17%   72.17%   +<.01%     
==========================================
  Files        1136     1136              
  Lines       57147    57150       +3     
  Branches     8185     8185              
==========================================
+ Hits        41243    41249       +6     
+ Misses      14617    14614       -3     
  Partials     1287     1287

Flag	Coverage Δ
#smoketests	`72.17% <97.87%> (ø)`	⬆️
#unittests	`69.78% <97.87%> (ø)`	⬆️

Impacted Files	Coverage Δ
nipype/workflows/smri/freesurfer/ba_maps.py	`90.27% <100%> (ø)`	⬆️
nipype/workflows/smri/freesurfer/autorecon2.py	`71.26% <100%> (+0.11%)`	⬆️
nipype/utils/__init__.py	`100% <100%> (ø)`	⬆️
nipype/algorithms/metrics.py	`53.47% <100%> (ø)`	⬆️
nipype/interfaces/utility/wrappers.py	`86.11% <100%> (-0.26%)`	⬇️
nipype/algorithms/icc.py	`58.44% <100%> (ø)`	⬆️
nipype/workflows/smri/freesurfer/autorecon1.py	`38.42% <100%> (+0.28%)`	⬆️
nipype/workflows/smri/freesurfer/autorecon3.py	`69.09% <100%> (+0.1%)`	⬆️
nipype/algorithms/misc.py	`44.22% <100%> (ø)`	⬆️
nipype/interfaces/petpvc.py	`55.55% <100%> (+1.85%)`	⬆️
... and 8 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 a61e455...aaef1f3. Read the comment docs.

chrisgorgo · 2017-06-01T19:16:34Z

I think there are still some imports apart from the autogenerated classes (in dipy and petpvc for example).

effigies · 2017-06-01T20:26:49Z

I think I got all of them now.

oesteban

LGTM, only make sure those from __future__ import absolute_import are there (for python 2 to work equivalently)

oesteban · 2017-06-01T22:27:22Z

nipype/interfaces/fsl/ICA_AROMA.py

-    traits,
-    OutputMultiPath
-)
+from ..base import (TraitedSpec, CommandLineInputSpec, CommandLine,


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:27:57Z

nipype/interfaces/fsl/fix.py

@@ -54,7 +54,7 @@

 """

-from nipype.interfaces.base import (
+from ..base import (


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:28:12Z

nipype/interfaces/niftyseg/base.py

@@ -16,8 +16,8 @@
 See the docstrings of the individual classes for examples.
 """

-from nipype.interfaces.niftyreg.base import no_nifty_package
-from nipype.interfaces.niftyfit.base import NiftyFitCommand
+from ..niftyreg.base import no_nifty_package


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:28:44Z

nipype/utils/__init__.py

@@ -1,6 +1,4 @@
 # -*- coding: utf-8 -*-
-from __future__ import absolute_import


Why removing this?

oesteban · 2017-06-01T22:28:59Z

nipype/workflows/dmri/fsl/artifacts.py

@@ -4,13 +4,13 @@
 # vi: set ft=python sts=4 ts=4 sw=4 et:
 from __future__ import division

-from nipype.utils import NUMPY_MMAP
-
 from ....interfaces.io import JSONFileGrabber


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:29:18Z

nipype/workflows/smri/freesurfer/autorecon1.py

-from nipype.interfaces.utility import Function,IdentityInterface
-import nipype.pipeline.engine as pe  # pypeline engine
-from nipype.interfaces.freesurfer import *
+from ....utils import NUMPY_MMAP


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:29:24Z

nipype/workflows/smri/freesurfer/autorecon2.py

-from nipype.interfaces.utility import Function, IdentityInterface, Merge
-import nipype.pipeline.engine as pe  # pypeline engine
-from nipype.interfaces.freesurfer import *
+from ....interfaces.utility import Function, IdentityInterface, Merge


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:29:33Z

nipype/workflows/smri/freesurfer/autorecon3.py

-from nipype.interfaces.utility import IdentityInterface, Merge, Function
-import nipype.pipeline.engine as pe  # pypeline engine
-from nipype.interfaces.freesurfer import *
+from ....interfaces.utility import IdentityInterface, Merge, Function


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:29:42Z

nipype/workflows/smri/freesurfer/ba_maps.py

-from nipype.interfaces.freesurfer import *
-from nipype.interfaces.io import DataGrabber
-from nipype.interfaces.utility import Merge
+from ....interfaces.utility import Function, IdentityInterface


for backwards compatibility, make sure to add from __future__ import absolute_import before this

oesteban · 2017-06-01T22:29:54Z

nipype/workflows/smri/niftyreg/groupwise.py

-import nipype.interfaces.utility as niu
-import nipype.interfaces.niftyreg as niftyreg
-import nipype.pipeline.engine as pe
+from ....interfaces import utility as niu


for backwards compatibility, make sure to add from __future__ import absolute_import before this

effigies · 2017-06-01T23:06:38Z

I thought from __future__ import absolute_import was for if you wanted to import within the package using absolute import paths. If we're eliminating them, it seemed unnecessary.

Instead it seems that it does the opposite, always preferring an installed package to the local package. Which makes me wonder why we were using absolute imports for within-package imports at all.

effigies · 2017-06-02T13:17:30Z

@oesteban Updated.

ENH: Reduce within-package absolute imports

b96180c

effigies force-pushed the relative_imports branch from f5056c4 to 891f69f Compare June 1, 2017 19:38

RF: Local absolute imports to file-level relative

10c641d

effigies force-pushed the relative_imports branch from 891f69f to 10c641d Compare June 1, 2017 20:17

oesteban approved these changes Jun 1, 2017

View reviewed changes

COMPAT: Add __future__ imports to touched files

aaef1f3

satra merged commit ed9af31 into nipy:master Jun 3, 2017

effigies mentioned this pull request Jun 3, 2017

ENH: ICA_AROMA: make current working directory default output directory #2056

Merged

effigies deleted the relative_imports branch October 5, 2017 20:29

satra added this to the 0.14.0 milestone Oct 20, 2017

		@@ -1,6 +1,4 @@
		# -- coding: utf-8 --
		from __future__ import absolute_import

ENH: Reduce within-package absolute imports #2059

ENH: Reduce within-package absolute imports #2059

Uh oh!

Conversation

effigies commented Jun 1, 2017

Uh oh!

chrisgorgo commented Jun 1, 2017

Uh oh!

effigies commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chrisgorgo commented Jun 1, 2017

Uh oh!

effigies commented Jun 1, 2017

Uh oh!

oesteban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Jun 1, 2017

Uh oh!

effigies commented Jun 2, 2017

Uh oh!

Uh oh!

effigies commented Jun 1, 2017 •

edited

Loading

codecov-io commented Jun 1, 2017 •

edited

Loading