Skip to content

Updating contributing and testing (closes #2445) #2482

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.

Sign up for GitHub

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 7 commits into from
Mar 9, 2018
Merged

Updating contributing and testing (closes #2445) #2482

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
595a70d
updating testing doc and a few changes to contributing.md
djarecka Mar 3, 2018
f5ed23c
adding links
djarecka Mar 3, 2018
9803db7
changing a link about PR
djarecka Mar 3, 2018
7ea3525
suggestions from Satra
djarecka Mar 5, 2018
86ecf33
suggestions from Chris M.
djarecka Mar 7, 2018
5dd3807
resolving a conflict
djarecka Mar 7, 2018
201a2ef
sty: add clarity for debian
satra Mar 9, 2018
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 32 additions & 19 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ Welcome to the Nipype repository! We're excited you're here and want to contribu

These guidelines are designed to make it as easy as possible to get involved. If you have any questions that aren't discussed below, please let us know by opening an [issue][link_issues]!

Before you start you'll need to set up a free [GitHub][link_github] account and sign in. Here are some [instructions][link_signupinstructions].
Before you start you'll need to set up a free [GitHub][link_github] account and sign in, here are some [instructions][link_signupinstructions].
Copy link
Member

Choose a reason for hiding this comment

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

The comma you added doesn't really work. You can use a period as before, or switch to a semicolon (;) and keep the lowercase "here".
If you are not familiar with version control system and Git,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "version control systems such as git,".
you will find introduction and links to tutorials [here](http://www.reproducibleimaging.org/module-reproducible-basics/02-vcs/).
Copy link
Member

Choose a reason for hiding this comment

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

Linking with "here" as the text can usually be improved by picking a more descriptive word or phrase and centering the text around that. One option:

[introductions and tutorials](<URL>) may be found on [ReproducibleImaging.org](https://www.reproducibleimaging.org/).

Already know what you're looking for in this guide? Jump to the following sections:
* [Understanding issue labels](#issue-labels)
Expand Down Expand Up @@ -47,30 +49,37 @@ This allows other members of the Nipype development team to confirm that you are

**2. [Fork][link_fork] the [Nipype repository][link_nipype] to your profile.**

This is now your own unique copy of Nipype.
This is now your own unique copy of Nipype repository.
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to add "repository", you'll need "the" before "Nipype".
Changes here won't affect anyone else's work, so it's a safe space to explore edits to the code!

Make sure to [keep your fork up to date][link_updateupstreamwiki] with the master repository.
You can clone your Nipype repository in order to create a local copy of the code on your machine.
In your local Nipype directory, run `pip install -e .[dev]`.
This will add your version of nipype to your local python environment and
install dependencies needed for development.
Copy link
Member

Choose a reason for hiding this comment

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

I would add the justification first.

To install your version of Nipype, and the dependencies needed for development, in your Python environment, run `pip install -e ".[dev]"` from your local Nipype directory.

(I'm adding the quotes because zsh tries to interpret the brackets. I'm not sure if any other shells do, but the above line is slightly safer.)

Make sure to [keep your fork up to date][link_updateupstreamwiki] with the original Nipype repository.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can introduce/reinforce the "upstream" vocabulary by replacing original with "upstream".
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to not used work "upstream" on purpose, just to minimize number of new words (you don't need to have "upstream" to keep your master up to date). However the link is about "upstream", so I would just add an extra link that describes it:
"Make sure to keep your fork up to date with the original Nipype repository, one way of doing it is to configure a new remote upstream repository and [syncing your fork with the upstream][link_updateupstreamwiki].
Copy link
Member

Choose a reason for hiding this comment

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

That seems fine. Again, though, the comma is doing a bit too much work. I would make it a new sentence at "One way".

Also, you're switching tenses in this sentence with "doing", "to configure" and "syncing". As a suggestion (with some other slight rewording): "One way to do this is to configure a new remote named "upstream" and to sync your fork with the upstream repository."

**3. Make the changes you've discussed.**

If you're adding a new tool from an existing neuroimaging toolkit (e.g., 3dDeconvolve from AFNI), check out the [guide for adding new interfaces to Nipype][link_new_interfaces].
If you're adding a new tool from an existing neuroimaging toolkit (e.g., 3dDeconvolve from AFNI),
check out the [guide for adding new interfaces to Nipype][link_new_interfaces].

To confirm that your changes worked as intended, [clone your fork][link_cloning] to create a local directory.
In this local directory, run `python setup.py develop`.
This will add your version of nipype to your local python environment.
When you are working on your changes, you should test frequently if you are not breaking the existing code,
Copy link
Member

Choose a reason for hiding this comment

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

"test frequently to ensure you are not breaking the existing code."
more on testing you will find [the testing section of Nipype documentation](http://nipype.readthedocs.io/en/latest/devel/testing_nipype.html).
Copy link
Member

Choose a reason for hiding this comment

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

For more on testing, please see [the testing section ...](...).

Then, in this local nipype directory, run `make check-before-commit`. If you get no errors, you're ready to submit your changes!
Before pushing your changes to GitHub run `make check-before-commit`, this will remove trailing spaces, create new auto tests,
Copy link
Member

Choose a reason for hiding this comment

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

Comma between "Github" and "run". New sentence at "This will remove..."
test entire package and build the documentation.
Copy link
Member

Choose a reason for hiding this comment

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

"test the entire package, and build the documentation."
If you get no errors, you're ready to submit your changes!

**4. Submit a [pull request][link_pullrequest].**
It's a good practice to create [a new branch](https://help.github.com/articles/about-branches/)
of the repository for a new set of changes.

Submit a new pull request for your changes, using the tags outlined in the [tagging pull requests section](#tagging-pull-requests).

A member of the development team will review your changes to confirm that they can be merged into the main codebase.
**4. Submit a [pull request][link_pullrequest].**

## Tagging Pull Requests
A new pull request for your changes should be created from your fork of the repository.

Pull requests should be submitted early and often! When opening a pull request, please use one of the following prefixes:
When opening a pull request, please use one of the following prefixes:


* **[ENH]** for enhancements
Expand All @@ -81,11 +90,14 @@ Pull requests should be submitted early and often! When opening a pull request,
* **[REF]** for refactoring existing code

<br>

If, when you submit, your pull request is not yet ready to be merged, please also include the **[WIP]** prefix. This tells the development team that your pull request is a "work-in-progress", and that you plan to continue working on it.
Pull requests should be submitted early and often (please don't mix too many unrelated changes within one PR)!
If your pull request is not yet ready to be merged, please also include the **[WIP]** prefix (you can remove it once your PR is ready to be merged).
This tells the development team that your pull request is a "work-in-progress", and that you plan to continue working on it.

Review and discussion on new code can begin well before the work is complete, and the more discussion the better!
In the worst case scenario, if the development team decides to pursue a different path than you've outlined, they'll close the pull request. That's really not so bad! :smile:
Copy link
Member

Choose a reason for hiding this comment

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

I might retain this line, unless it's generally agreed that it's unwanted. I think there's some value in indicating that some changes may not be accepted, especially in the context of encouraging discussion and agreement prior to putting in a huge amount of effort on the code.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed it, since the idea that some (more or less abstract) team just closes the PR sounded to me (taking a perspective of person who is new to open source) rather harsh and discouraging ;-) but it's very subjective opinion...

what about "Review and discussion on new code can begin well before the work is complete, and the more discussion the better! The development team can prefer a different path than you've outlined, so it's better to discuss it and get approval at the early stage of your work."
Copy link
Member

Choose a reason for hiding this comment

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

Sounds great, but I would use "The development team may ..." rather than "The development team can...".
This provides the opportunity to check with the development team the path you've outlined.

One your PR is ready a member of the development team will review your changes to confirm that they can be merged into the main codebase.

## Notes for New Code

Expand All @@ -98,8 +110,9 @@ Do not log this, as it creates redundant/confusing logs.

#### Testing
New code should be tested, whenever feasible.
Bug fixes should include regression tests, and any new behavior should at least get minimal exercise.
If you're not sure what this means for your code, ask!
Bug fixes should include an example that exposes the issue.
Any new features should have tests that show at least a minimal example.
If you're not sure what this means for your code, please ask in your pull request.

## Recognizing contributions

Expand Down Expand Up @@ -132,7 +145,7 @@ You're awesome. :wave::smiley:
[link_good_first_issue]: https://github.com/nipy/nipype/issues?q=is%3Aopen+is%3Aissue+label%3Agood-first-issue
[link_enhancement]: https://github.com/nipy/nipype/labels/enhancement

[link_pullrequest]: https://help.github.com/articles/creating-a-pull-request/
[link_pullrequest]: https://help.github.com/articles/creating-a-pull-request-from-a-fork/
[link_fork]: https://help.github.com/articles/fork-a-repo/
[link_pushpullblog]: https://www.igvita.com/2011/12/19/dont-push-your-pull-requests/
[link_updateupstreamwiki]: https://help.github.com/articles/syncing-a-fork/
Expand Down
81 changes: 25 additions & 56 deletions doc/devel/testing_nipype.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,30 +27,18 @@ After cloning::

cd nipype
pip install -r requirements.txt
python setup.py develop

or::

cd nipype
pip install -r requirements.txt
pip install -e .[tests]

pip install -e .[dev]


Test implementation
-------------------

Nipype testing framework is built upon `pytest <http://doc.pytest.org/en/latest/>`_.
By the time these guidelines are written, Nipype implements 17638 tests.

After installation in developer mode, the tests can be run with the
following simple command at the root folder of the project ::

make tests
following command at the root folder of the project ::

If ``make`` is not installed in the system, it is possible to run the tests using::

py.test --doctest-modules --cov=nipype nipype
pytest -v --doctest-modules nipype


A successful test run should complete in 10-30 minutes and end with
Expand All @@ -75,69 +63,50 @@ where ``$pathtomatlabdir`` is the path to your matlab installation and
``$platform`` is the directory referring to x86 or x64 installations
(typically ``glnxa64`` on 64-bit installations).

Skip tests
Skipped tests
~~~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

fix the heading lengths for restructured text.

Nipype will skip some tests depending on the currently available software and data
dependencies. Installing software dependencies and downloading the necessary data
will reduce the number of skip tests.
will reduce the number of skipped tests.

Some tests in Nipype make use of some images distributed within the `FSL course data
A few tests in Nipype make use of some images distributed within the `FSL course data
<http://fsl.fmrib.ox.ac.uk/fslcourse/>`_. This reduced version of the package can be downloaded `here
<https://files.osf.io/v1/resources/nefdp/providers/osfstorage/57f472cf9ad5a101f977ecfe>`_.
To enable the tests depending on these data, just unpack the targz file and set the :code:`FSL_COURSE_DATA` environment
variable to point to that folder.
variable to point to that folder.
Note, that the test execution time can increase significantly with these additional tests.


Xfail tests
Xfailed tests
~~~~~~~~~~~

Some tests are expect to fail until the code will be changed or for other reasons.


Avoiding any MATLAB calls from testing
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

On unix systems, set an empty environment variable::

export NIPYPE_NO_MATLAB=

This will skip any tests that require matlab.


Testing Nipype using Docker
---------------------------

As of :code:`nipype-0.13`, Nipype is tested inside Docker containers. First, install the
`Docker Engine <https://docs.docker.com/engine/installation/>`_.
Nipype has one base docker image called nipype/base:latest, and several additional test images
for various Python versions.

The base nipype image is built as follows::
Nipype is tested inside Docker containers and users can use nipype images to test local versions.
First, install the `Docker Engine <https://docs.docker.com/engine/installation/>`_.
Nipype has one base docker image called nipype/base:latest, that contains several useful tools
Copy link
Member

Choose a reason for hiding this comment

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

nipype/nipype:base
(FreeSurfer, AFNI, FSL, ANTs, etc.), and additional test images
for specific Python versions: py27 for Python 2.7 and py36 for Python 3.6.

cd path/to/nipype/
docker build -t nipype/base:latest -f docker/base.Dockerfile .
Users can pull the nipype image for Python 3.6 as follows::

docker pull nipype/base:py36
Copy link
Member

Choose a reason for hiding this comment

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

nipype/nipype:py36

This base image contains several useful tools (FreeSurfer, AFNI, FSL, ANTs, etc.),
but not nipype.
In order to test a local version of nipype you can run test within container as follows::

It is possible to fetch a built image from the latest master branch of nipype
using::
docker run -it -v $PWD:/src/nipype --rm nipype/nipype:py36 py.test -v --doctest-modules /src/nipype/nipype

docker run -it --rm nipype/nipype:master

Additional comments
-------------------

The docker run command will then open the container and offer a bash shell for the
developer.

For building a continer for running nipype in Python 3.6::
If the project is tested both on your local OS and within a Docker container you might have to remove all
Copy link
Member

Choose a reason for hiding this comment

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

Comma after "container".
``__pycache__`` directory before changing between system.
Copy link
Member

Choose a reason for hiding this comment

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

systems?

cd path/to/nipype/
docker build -f Dockerfile -t nipype/nipype_test:py36 .
docker run -it --rm -e FSL_COURSE_DATA="/root/examples/nipype-fsl_course_data" \
-v ~/examples:/root/examples:ro \
-v ~/scratch:/scratch \
-w /root/src/nipype \
nipype/nipype_test:py36 /usr/bin/run_pytests.sh

The last examples assume that the example data is downladed into ~/examples and
the ~/scratch folder will be created if it does not exist previously.

Copy link
Member

Choose a reason for hiding this comment

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

No blank lines at the end of the file.