ENH: Permit external packages to deprecate interfaces #2028
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2028 +/- ##
=========================================
+ Coverage 72.2% 72.2% +<.01%
=========================================
Files 1132 1132
Lines 57023 57024 +1
Branches 8165 8165
=========================================
+ Hits 41173 41176 +3
+ Misses 14566 14564 -2
Partials 1284 1284
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #2028 +/- ##
==========================================
+ Coverage 72.17% 72.17% +<.01%
==========================================
Files 1134 1134
Lines 57096 57098 +2
Branches 8179 8179
==========================================
+ Hits 41208 41210 +2
Misses 14602 14602
Partials 1286 1286
Continue to review full report at Codecov.
|
|
I'm moving to This did require me to switch the gitversion scheme to Obviously it isn't appropriate for things like cc @satra @chrisfilo |
| @@ -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__ | |||
There was a problem hiding this comment.
remove LooseVersion from everywhere?
There was a problem hiding this comment.
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.
| @@ -141,7 +141,8 @@ def get_nipype_gitversion(): | |||
| 'configparser', | |||
| 'pytest>=%s' % PYTEST_MIN_VERSION, | |||
| 'mock', | |||
| 'pydotplus' | |||
| 'pydotplus', | |||
| 'packaging', | |||
There was a problem hiding this comment.
packing should be added to requirements.txt and rtd_requirements as well?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Building the docker image (I never figured out how to do virtualenvs) from scratch. Is that an equivalent check?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
and more generally download and install miniconda and then do: conda create -n envname python=2/3 for creating virtual envs
There was a problem hiding this comment.
Builds and installs without issue. (Had to apt-get install build-essential, conda install scipy, lxml and dependencies, but otherwise no problems.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
|
can this wait for the next release? or should it be included now? |
|
It can wait. |
|
@satra This is ready for another look, now the release is out. |
effigies
force-pushed
the
enh/external_deprecation
branch
from
42d1f1e to
2a17697
Compare
|
@effigies - could you please merge with master? |
|
It's rebased off master. |
|
@effigies - that's weird, i haven't seen that error in all the builds i have run in the last few days? also i don't see a commit message above that merges off of current master. (circle doesn't merge with master when a PR is run). |
|
So it seems like restoring docker loses caching... |
|
@effigies - since we are testing this out, could you please now remove those two lines? |
effigies
force-pushed
the
enh/external_deprecation
branch
from
b068c3e to
2a17697
Compare
|
Removed that commit. Circle tests are re-running. |
effigies
force-pushed
the
enh/external_deprecation
branch
from
039362c to
f0d7070
Compare
|
Passing. Doesn't look like we benefited from caching, but the Circle caching mechanism is opaque to me. Perhaps it takes a couple attempts to get something usable to a future build. |
External projects, such as poldracklab/niworkflows, may maintain a set of interfaces, that over time require deprecating portions of their input and output specs. At present, the deprecation machinery is intrinsically tied to nipype's version number.
This PR adds a
package_versionattribute to each interface, which defaults to nipype's, but allows other packages easily to add their own versioning information to specs with a deprecation schedule.