Skip to content

FIX: Update AFNI pin to current NeuroDebian package #2075

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 3 commits into from
Jun 14, 2017

Conversation

effigies
Copy link
Member

Changes proposed in this pull request

  • Update pinned afni package

See also nipreps/fmriprep#557

@effigies effigies changed the title Update AFNI pin to current NeuroDebian package FIX: Update AFNI pin to current NeuroDebian package Jun 14, 2017
@satra
Copy link
Member

satra commented Jun 14, 2017

@effigies - the afni package should really be updated in neurodebian. it's old at this point and does not contain all the fixes that were made recently.

@effigies
Copy link
Member Author

I agree. But this is more about building nipype/base than pushing Neurodebian to update a package.

(I don't even know what's been updated here. They don't include a changelog.)

@effigies
Copy link
Member Author

Docker build fixed

@satra
Copy link
Member

satra commented Jun 14, 2017

if things look good, could you please push the build to docker hub?

@effigies
Copy link
Member Author

What's the process for doing that? I'd probably need some permissions there.

@oesteban
Copy link
Contributor

Does it make sense removing the version pin? After all, neurodebian does not get updated with new versions of FSL and AFNI that often.

@effigies
Copy link
Member Author

I'm not really sure what the original justification for such a specific pin was; the only thing I can think of is that it acted as documentation. If whatever provenance/documentation needs can be satisfied in a way that doesn't break the build every time things get updated upstream, I'd be happy to go in that direction.

As noted in the other thread, NeuroDebian will break the build every time they push a new version, because they don't maintain old packages. So we should probably have a policy.

That said, I'd like to get this resolved ASAP, so we can run tests again.

@oesteban
Copy link
Contributor

The version pin was inherited from fmriprep and mriqc docker images, where we particularly want to have a strict control of versions, at the cost of some builds that fail after updates of neurodebian.

I don't think we need a control such strict for nipype. I'd remove all version pins of aptitude to make sure it does not stop working.

That said, the protocol for maintaining the nipype/base docker image doesn't exist. The root of the problem is the inability of circle to cache large docker images. However, since 0.13 we are building the base image everytime in circle (which is a good check) and then we do not push it to the docker registry. If we are going to build it (besides/instead of pulling it down), then it would make sense to add a docker push with releases, as it is done with nipype/nipype.

@satra
Copy link
Member

satra commented Jun 14, 2017

@oesteban - i concur with everything you just said :)

@effigies
Copy link
Member Author

Removed APT version pins. ca-certificates is installed with curl in an earlier step.

@codecov-io
Copy link

codecov-io commented Jun 14, 2017

Codecov Report

Merging #2075 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2075   +/-   ##
=======================================
  Coverage   72.17%   72.17%           
=======================================
  Files        1138     1138           
  Lines       57233    57233           
  Branches     8199     8199           
=======================================
  Hits        41306    41306           
  Misses      14637    14637           
  Partials     1290     1290
Flag Coverage Δ
#smoketests 72.17% <ø> (ø) ⬆️
#unittests 69.78% <ø> (ø) ⬆️

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 e02cb4a...ad085ae. Read the comment docs.

@effigies
Copy link
Member Author

Tests passing. Good to go?

@satra satra merged commit 2744d4c into nipy:master Jun 14, 2017
@effigies effigies deleted the update_afni branch June 14, 2017 20:38
@satra satra added this to the 0.14.0 milestone Oct 20, 2017
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.

4 participants