Skip to content

ENH: Permit external packages to deprecate interfaces #2028

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 24, 2017
Merged

ENH: Permit external packages to deprecate interfaces #2028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfc12de
ENH: Permit external packages to deprecate interfaces
effigies May 17, 2017
75b6ae3
ENH: Use PEP440-compliant versioning, including for gitversion
effigies May 18, 2017
f0d7070
Update requirements files
effigies May 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions nipype/info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def get_nipype_gitversion():
if __version__.endswith('-dev'):
gitversion = get_nipype_gitversion()
if gitversion:
__version__ = __version__.replace('-dev', '-' + gitversion + '.dev')
__version__ = '{}+{}'.format(__version__, gitversion)

CLASSIFIERS = ['Development Status :: 5 - Production/Stable',
'Environment :: Console',
Expand Down Expand Up @@ -141,7 +141,8 @@ def get_nipype_gitversion():
'funcsigs',
'pytest>=%s' % PYTEST_MIN_VERSION,
'mock',
'pydotplus'
'pydotplus',
'packaging',
Copy link
Member

Choose a reason for hiding this comment

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

packing should be added to requirements.txt and rtd_requirements as well?

Copy link
Member

Choose a reason for hiding this comment

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

i think, this would also need to be added to SETUP_REQUIRES. you can check by trying to install nipype in a clean environment using python setup.py install.

Copy link
Member Author

Choose a reason for hiding this comment

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

Building the docker image (I never figured out how to do virtualenvs) from scratch. Is that an equivalent check?

Copy link
Member

Choose a reason for hiding this comment

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

you can do this inside a basic continuumio/miniconda docker instance - think of that as a virtualenv.

docker run -it --rm -v /path/to/nipype:/nipype-src continuumio/miniconda bash
> cd /nipype-src
> python setup.py bdist_wheel

Copy link
Member

Choose a reason for hiding this comment

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

and more generally download and install miniconda and then do: conda create -n envname python=2/3 for creating virtual envs

Copy link
Member Author

Choose a reason for hiding this comment

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

Builds and installs without issue. (Had to apt-get install build-essential, conda install scipy, lxml and dependencies, but otherwise no problems.)

Copy link
Member

Choose a reason for hiding this comment

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

so that latter are runtime dependencies not setup dependencies. there is difference between setup requires and run/test requires. setup requires are things that the setup.py file imports.

Copy link
Member

Choose a reason for hiding this comment

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

e.g. see: https://github.com/conda-forge/nipype-feedstock/blob/master/recipe/meta.yaml#L17

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I don't think packaging is required by that. Running python setup.py bdist_wheel worked fine. I then also ran python setup.py install which required the other dependencies.

Sorry if I'm missing something.

]

if sys.version_info <= (3, 4):
Expand Down
6 changes: 4 additions & 2 deletions nipype/interfaces/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from warnings import warn
import simplejson as json
from dateutil.parser import parse as parseutc
from packaging.version import Version

from .. import config, logging, LooseVersion, __version__
Copy link
Member

Choose a reason for hiding this comment

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

remove LooseVersion from everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used for versioning on external programs, e.g. FSVersion < 6.0 or FSLVersion > 507. Since these don't follow PEP440, I intended to leave them as they are.

from ..utils.provenance import write_provenance
Expand All @@ -43,7 +44,7 @@
from ..external.due import due

runtime_profile = str2bool(config.get('execution', 'profile_runtime'))
nipype_version = LooseVersion(__version__)
nipype_version = Version(__version__)
iflogger = logging.getLogger('interface')

FLOAT_FORMAT = '{:.10f}'.format
Expand Down Expand Up @@ -352,6 +353,7 @@ class BaseTraitedSpec(traits.HasTraits):
XXX Reconsider this in the long run, but it seems like the best
solution to move forward on the refactoring.
"""
package_version = nipype_version

def __init__(self, **kwargs):
""" Initialize handlers and inputs"""
Expand Down Expand Up @@ -444,7 +446,7 @@ def _deprecated_warn(self, obj, name, old, new):
else:
msg3 = ''
msg = ' '.join((msg1, msg2, msg3))
if LooseVersion(str(trait_spec.deprecated)) < nipype_version:
if Version(str(trait_spec.deprecated)) < self.package_version:
raise TraitError(msg)
else:
if trait_spec.new_name:
Expand Down
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ configparser
pytest>=3.0
mock
pydotplus
packaging
1 change: 1 addition & 0 deletions rtd_requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ mock
pydotplus
psutil
matplotlib
packaging