Skip to content

MAINT: Drop pytest-xdist requirement, minimum pytest version #2856

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

Merged
merged 10 commits into from
Jan 20, 2019

Conversation

effigies
Copy link
Member

Summary

This is an addendum to #2854, following #2854 (comment). I'm basing off of master instead of @yarikoptic's branch to make sure tests still work. @yarikoptic Feel free to cherry-pick these pieces, if you like, but probably a good idea to wait to make sure that I'm not breaking things horribly. Or if you're happy with this and have nothing further to add, we can just merge this.

@satra @djarecka I believe you had strong feelings on pytest's place in everything, so might be worth a couple minutes of your time.

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

  • Drop pytest-xdist from requirements
  • Drop minimum pytest version (was there to support new pytest-xdist)
  • Adds pytest-xdist to Travis and CircleCI builds (via docker, in the latter case) for the sake of speed
  • Moves configparser requirement into PEP508 format, as right now I think it's being ignored utterly, since we build with Python 3.

Acknowledgment

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

@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #2856 into master will decrease coverage by <.01%.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2856      +/-   ##
==========================================
- Coverage   67.46%   67.46%   -0.01%     
==========================================
  Files         341      341              
  Lines       43392    43393       +1     
  Branches     5383     5382       -1     
==========================================
- Hits        29276    29273       -3     
- Misses      13419    13423       +4     
  Partials      697      697
Flag Coverage Δ
#smoketests 50.52% <12.5%> (-0.02%) ⬇️
#unittests 64.88% <12.5%> (-0.01%) ⬇️
Impacted Files Coverage Δ
nipype/info.py 93.93% <ø> (-0.27%) ⬇️
nipype/__init__.py 59.45% <12.5%> (-7.21%) ⬇️

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 ebbd062...50e2f47. Read the comment docs.

@effigies effigies added this to the 1.1.8 milestone Jan 17, 2019
Copy link
Member

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

This looks pretty good - left some small comments.

@@ -108,6 +108,8 @@ function generate_main_dockerfile() {
--miniconda use_env=neuro \
pip_opts="-e" \
pip_install="/src/nipype[all]" \
Copy link
Member

Choose a reason for hiding this comment

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

we could just add pytest-xdist here

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure we want to install it in editable mode.

Copy link
Member

Choose a reason for hiding this comment

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

whoops, meant L95

@@ -11,10 +11,6 @@ neurdflib
click>=6.6.0
funcsigs
configparser
pytest>=3.0
Copy link
Member

@mgxd mgxd Jan 18, 2019

Choose a reason for hiding this comment

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

Do we really need to maintain our dependencies in 2 locations - info, requirements? What are thoughts on replacing this file with a simple:

.

or

.[dev]

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 think that will also install the current repo, not just the dependencies, right?

Copy link
Member

Choose a reason for hiding this comment

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

yep

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 don't really care, but that changes what pip install -r requirements.txt does. Do we know anybody who installs it that way, and what they desire from that?

@yarikoptic Does Debian use requirements.txt for anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mgxd Let's leave changing requirements.txt to another PR, where we can seek a consensus on the proper way to manage it.

Copy link
Member

Choose a reason for hiding this comment

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

thats good with me

@@ -92,7 +92,7 @@ function generate_main_dockerfile() {
conda_install='python=${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}
icu=58.1 libxml2 libxslt matplotlib mkl numpy paramiko
pandas psutil scikit-learn scipy traits=4.6.0' \
pip_install="grabbit==0.1.2 https://github.com/INCF/pybids/tarball/0.6.5" \
pip_install="pytest-xdist" \
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 removed the grabbit/pybids stuff, as it's not installed correctly to test BIDSDataGrabber, anyway. This should be fixed up in #2737.

@effigies effigies merged commit 4468b3a into nipy:master Jan 20, 2019
@effigies effigies deleted the bf/nopytest branch January 20, 2019 17:16
yarikoptic added a commit to yarikoptic/nipype that referenced this pull request Feb 4, 2019
1.1.8 (January 28, 2019)

  * FIX: ANTS LaplacianThickness cmdline opts fixed up (nipy#2846)
  * FIX: Resolve LinAlgError during SVD (nipy#2838)
  * ENH: Add interfaces wrapping DIPY worflows (nipy#2830)
  * ENH: Update BIDSDataGrabber for pybids 0.7 (nipy#2737)
  * ENH: Add FSL `eddy_quad` interface (nipy#2825)
  * ENH: Support tckgen -select in MRtrix3 v3+ (nipy#2823)
  * ENH: Support for BIDS event files (nipy#2845)
  * ENH: CompositeTransformUtil, new ANTs interface (nipy#2785)
  * RF: Move pytest and pytest-xdist from general requirement into tests_required (nipy#2850)
  * DOC: Add S3DataGrabber example (nipy#2849)
  * DOC: Skip conftest module in API generation (nipy#2852)
  * DOC: Hyperlink DOIs to preferred resolver (nipy#2833)
  * MAINT: Install numpy!=1.16.0 from conda in Docker (nipy#2862)
  * MAINT: Drop pytest-xdist requirement, minimum pytest version  (nipy#2856)
  * MAINT: Disable numpy 1.16.0 for Py2.7 (nipy#2855)

* tag '1.1.8': (79 commits)
  MNT: Add @feilong to .zenodo, update ordering
  MNT: Update .mailmap
  MNT: Update .zenodo ordering
  Accept invitation as Zenodo release co-author (see nipy#2864)
  MAINT: Update .mailmap
  BF: allowing bids_event_file as alternate input
  MNT: Update .zenodo ordering
  MNT: Version 1.1.8
  DOC: 1.1.8 changelog
  Update nipype/interfaces/dipy/tracks.py
  Update nipype/interfaces/dipy/reconstruction.py
  MNT: Install numpy!=1.16.0 from conda in Docker
  Add FSL auto test
  remake specs
  Update nipype/interfaces/io.py
  Remove return type named tuple
  Update nipype/info.py
  STY: Whitespace, line length
  Remove out_ prefix from EddyQuad outputs
  Apply minor edits from code review
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants