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

CI: Build docs outside of container #1606

Merged
merged 7 commits into from
May 3, 2019

Conversation

effigies
Copy link
Member

@effigies effigies commented May 3, 2019

Changes proposed in this pull request

This should speed up builds somewhat by avoiding using the container in the documentation build and avoiding the container build in doc(s)/* branches.

Documentation that should be reviewed

@effigies effigies force-pushed the doc/independent_doc_build branch from 447bf0d to 16960f6 Compare May 3, 2019 18:13
@effigies effigies requested a review from oesteban May 3, 2019 18:24
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

I suspect we also want to update the Makefile to treat warnings as errors.

This is an incredible optimization 👍

.circleci/config.yml Outdated Show resolved Hide resolved
@effigies
Copy link
Member Author

effigies commented May 3, 2019

Locally I'm getting this failure:

Warning, treated as error:
/Users/markiewicz/Projects/crn/fmriprep/venv/lib/python3.6/site-packages/fmriprep/workflows/base.py:docstring of fmriprep.workflows.base.init_fmriprep_wf:7:Exception occurred in plotting index-1
 from /Users/markiewicz/Projects/crn/fmriprep/docs/api/index.rst:
Traceback (most recent call last):
  File "/Users/markiewicz/Projects/crn/fmriprep/venv/lib/python3.6/site-packages/nipype/sphinxext/plot_workflow.py", line 485, in run_code
    exec(code, ns)
  File "<string>", line 38, in <module>
  File "/Users/markiewicz/Projects/crn/fmriprep/venv/lib/python3.6/site-packages/fmriprep/workflows/base.py", line 218, in init_fmriprep_wf
    err_on_aroma_warn=err_on_aroma_warn,
  File "/Users/markiewicz/Projects/crn/fmriprep/venv/lib/python3.6/site-packages/fmriprep/workflows/base.py", line 462, in init_single_subject_wf
    skull_strip_template=skull_strip_template,
TypeError: init_anat_preproc_wf() got an unexpected keyword argument 'output_spaces'
make: *** [html] Error 2

Looks like what @markushs is seeing over in #1603.

@oesteban
Copy link
Member

oesteban commented May 3, 2019

smriprep version?

@effigies
Copy link
Member Author

effigies commented May 3, 2019

smriprep version?

v0.1.1. Whatever builds with pip install ".[doc]".

@effigies
Copy link
Member Author

effigies commented May 3, 2019

It's very unclear to me what LINKS_REQUIRES is supposed to be doing. Does anything respect it?

fmriprep/__about__.py Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
[build-system]
requires = ["pip>=18.1", "setuptools>=40.8", "wheel", "numpy"]
Copy link
Member Author

Choose a reason for hiding this comment

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

@oesteban This should allow us to install purely with pip install ., as pip will parse this first to make sure it has the build dependencies.

pip 18.1 allows us to use a dependency that looks like "package @ URL", so we can handle all dependencies directly in install_requires.

Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant discussion: https://stackoverflow.com/a/53412651

Possibly the reason this is failing for me is that pip removed dependency_links in version 19.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Looks like you can't actually set a minimum version on pip in this way. pypa/pip#4145

So this would mean we need to prepare for users with old pip, which is likely on most HPCs.

@oesteban
Copy link
Member

oesteban commented May 3, 2019

smriprep 0.2.0 has just posted. I would not spend a lot of time fighting pip.

'numpy',
'pandas',
'psutil>=5.4',
'pybids<0.8.0a0,>=0.7.1',
'pyyaml',
'scikit-image',
'smriprep @ git+https://github.com/poldracklab/smriprep.git@f1cfc37bcdc346549dbf1d037cdade3a3b32d5de',
'smriprep @ ',
Copy link
Member

Choose a reason for hiding this comment

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

this was potentially working. please note that comma breaking up the string concatenation.

@oesteban oesteban merged commit 517d50b into nipreps:master May 3, 2019
@effigies effigies deleted the doc/independent_doc_build branch May 3, 2019 22:47
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.

None yet

2 participants