Skip to content

MAINT: Sort dependencies alphabetically #2961

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 5 commits into from
Jul 17, 2019

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Jul 16, 2019

Splitting up #2959 in digestible bits.

This would be great to merge quickly if possible.

Summary

Fixes # .

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

Acknowledgment

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

@oesteban oesteban requested a review from effigies July 16, 2019 21:47
@effigies
Copy link
Member

This is fine by me, but I think the idea was to give a rough order of perceived importance of the dependency. Is the change just to make it easier to search and see if a dependency is or isn't in the list?

@oesteban
Copy link
Contributor Author

oesteban commented Jul 16, 2019

Is the change just to make it easier to search and see if a dependency is or isn't in the list?

Yes, that is the idea.

@satra, I'd say to remove those dependencies in a separate PR, just in case tests start failing...

@kaczmarj any fast way to instruct neurodocker to run all pip installs with --no-cache-dir?
EDIT: it is actually conda, not pip.

@satra
Copy link
Member

satra commented Jul 16, 2019

I'd say to remove those dependencies in a separate PR, just in case tests start failing

unfortunately i don't think we have a test for this. which i'll try to add. however this is not about removing, but the order of listing the dependencies. so neurdflib has to come after prov.

right now the tests are failing for other reasons.

@effigies
Copy link
Member

This might be fixed by using a newer neurodocker.

@kaczmarj
Copy link
Collaborator

@oesteban - neurodocker should include some conda clean call after conda install. at least version 0.5.0 does. is the cache present in the container?

@codecov-io
Copy link

codecov-io commented Jul 17, 2019

Codecov Report

Merging #2961 into master will decrease coverage by 3.36%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2961      +/-   ##
==========================================
- Coverage    67.6%   64.24%   -3.37%     
==========================================
  Files         344      342       -2     
  Lines       43797    43743      -54     
  Branches     5471     5468       -3     
==========================================
- Hits        29609    28101    -1508     
- Misses      13478    14554    +1076     
- Partials      710     1088     +378
Flag Coverage Δ
#smoketests ?
#unittests 64.24% <100%> (-0.8%) ⬇️
Impacted Files Coverage Δ
nipype/info.py 88.23% <100%> (-5.8%) ⬇️
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 50% <0%> (-30.51%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35% <0%> (-29.42%) ⬇️
nipype/interfaces/spm/base.py 57.94% <0%> (-29.14%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.35%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
nipype/testing/fixtures.py 77.33% <0%> (-21.34%) ⬇️
... and 42 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 b1eb3ee...772d0c8. Read the comment docs.

@oesteban
Copy link
Contributor Author

@kaczmarj 0.5.0 is failing too

@kaczmarj
Copy link
Collaborator

@oesteban - i will try to look into this tomorrow

@effigies
Copy link
Member

The issue is caused by conda/conda-package-handling#34. We need to set the working directory to something owned by the neuro user.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Need to restore neurdflib to post-prov. Updating the RTD requirements while we're at it.

Build failure addressed in #2789.
Please revert Neurodocker 0.5.0 update or remove it in a rebase; nipype builds don't work with it. See #2963 (comment).

@oesteban
Copy link
Contributor Author

@effigies, I added your suggestions within the merge upstream/master commit. I hope that's fine.

@oesteban oesteban requested a review from effigies July 17, 2019 18:49
Co-Authored-By: Chris Markiewicz <[email protected]>
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Seems fine. @satra, any concerns apart from prov before neurdflib?

@satra
Copy link
Member

satra commented Jul 17, 2019

@effigies - looks good to me. the one thing about rtd is that it was limited to the dependencies required for doc building, but it's fine to have everything. we should definitely check, once this is merged that rtd does build.

@oesteban oesteban merged commit 42b1239 into nipy:master Jul 17, 2019
@oesteban oesteban deleted the maint/reorder-deps branch July 17, 2019 21:53
@effigies
Copy link
Member

Docs built fine. https://readthedocs.org/projects/nipype/builds/9387007/

Somewhat unclear why they aren't built with every commit to master. I had to manually trigger the build.

@satra
Copy link
Member

satra commented Jul 17, 2019

i clicked on a few buttons (specifically enabled github integration - which i thought was already enabled). let's see if that makes a difference.

oesteban added a commit to oesteban/nipype that referenced this pull request Jul 31, 2019
@effigies effigies added this to the 1.2.1 milestone Aug 16, 2019
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 this pull request may close these issues.

5 participants