Skip to content
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

FIX: corrects readthedocs build error #2723

Merged
merged 7 commits into from
Oct 11, 2018

Conversation

miykael
Copy link
Collaborator

@miykael miykael commented Oct 9, 2018

Closes #2713.

Small PR that takes care of the readthedocs build error mentioned under #2713.

The problem was that matplotlib version 3.0.0 doesn't include the sphinxext.only_directives anymore.

@codecov-io
Copy link

codecov-io commented Oct 10, 2018

Codecov Report

Merging #2723 into master will decrease coverage by 3.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2723      +/-   ##
==========================================
- Coverage    67.6%   64.28%   -3.33%     
==========================================
  Files         340      338       -2     
  Lines       43153    43102      -51     
  Branches     5351     5348       -3     
==========================================
- Hits        29174    27707    -1467     
- Misses      13277    14314    +1037     
- Partials      702     1081     +379
Flag Coverage Δ
#smoketests ?
#unittests 64.28% <ø> (-0.8%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/nilearn.py 40% <0%> (-56.67%) ⬇️
nipype/utils/spm_docs.py 25.92% <0%> (-44.45%) ⬇️
nipype/interfaces/freesurfer/base.py 49.59% <0%> (-30.9%) ⬇️
nipype/utils/logger.py 59.7% <0%> (-29.86%) ⬇️
nipype/algorithms/rapidart.py 35.39% <0%> (-29.21%) ⬇️
nipype/interfaces/spm/base.py 58.41% <0%> (-29.05%) ⬇️
nipype/utils/provenance.py 55.73% <0%> (-28.99%) ⬇️
nipype/interfaces/fsl/model.py 55.26% <0%> (-25.35%) ⬇️
nipype/testing/fixtures.py 77.33% <0%> (-21.34%) ⬇️
nipype/interfaces/matlab.py 64.64% <0%> (-21.22%) ⬇️
... and 43 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 88de9f8...912039e. Read the comment docs.

@miykael
Copy link
Collaborator Author

miykael commented Oct 10, 2018

All the errors on circleci seem to be due to the spm version issue mentioned under ReproNim/neurodocker#227.

This PR updates pybids version to 0.6.5 with 46cbed6, but I wasn't able to update neurodocker to 0.4.1 here? How do I get the new/corresponding sha256?

Also, can somebody help me to understand why the tests on travis were failing?

@miykael miykael force-pushed the remove_matplotlib_sphinx_extension branch from a33addc to 46cbed6 Compare October 10, 2018 06:50
@effigies
Copy link
Member

The Travis tests are failing because of a coverage change in the pre-release. I think @adelavega can point to the issue, if needed.

@effigies
Copy link
Member

In any event, those pre-release checks shouldn't block merging, if that's the only thing holding you back right now. I think the solution is just to wait for a new coverage RC and Travis will fix itself.

@miykael
Copy link
Collaborator Author

miykael commented Oct 10, 2018

The main issue that I've tried to solve in this PR as well (I perhaps should rather create a new PR to do so), is to update neurodocker to 0.4.0. But this opens up questions, about FreeSurfer installation and license file, etc.

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.

From the builds:

Step 11/18 : ENV FREESURFER_HOME="/opt/freesurfer-6.0.0"     PATH="/opt/freesurfer-6.0.0/bin:$PATH"
...
Step 13/18 : RUN echo "cHJpbnRmICJrcnp5c3p0b2YuZ29yZ29sZXdza2lAZ21haWwuY29tXG41MTcyXG4gKkN2dW12RVYzelRmZ1xuRlM1Si8yYzFhZ2c0RVxuIiA+IC9vcHQvZnJlZXN1cmZlci9saWNlbnNlLnR4dAo=" | base64 -d | sh
 ---> Running in 590ff37e749b
sh: 1: cannot create /opt/freesurfer/license.txt: Directory nonexistent
The command '/bin/sh -c echo "cHJpbnRmICJrcnp5c3p0b2YuZ29yZ29sZXdza2lAZ21haWwuY29tXG41MTcyXG4gKkN2dW12RVYzelRmZ1xuRlM1Si8yYzFhZ2c0RVxuIiA+IC9vcHQvZnJlZXN1cmZlci9saWNlbnNlLnR4dAo=" | base64 -d | sh' returned a non-zero code: 2

Let's update the B64-encoded script to target $FREESURFER_HOME/license_file.txt. Unless the preferred method will be to require users to pass their own license file in?

--base "$NIPYPE_BASE_IMAGE" --pkg-manager "$PKG_MANAGER" \
--label maintainer="The nipype developers https://github.com/nipy/nipype" \
--env MKL_NUM_THREADS=1 OMP_NUM_THREADS=1 \
--user neuro \
--miniconda env_name=neuro \
--miniconda create_env=neuro \
conda_install='python=${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR}' \
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is being used prior to the definition of PYTHON_VERSION_* (L102).

@miykael
Copy link
Collaborator Author

miykael commented Oct 10, 2018

I will revert all the commits concerning docker/generate_dockerfiles.sh as they will be taken care of with #2707.

@effigies effigies merged commit 4f2933e into nipy:master Oct 11, 2018
@effigies effigies added this to the 1.1.4 milestone Oct 11, 2018
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.

3 participants