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

Travis run the test in minimal and full config #570

Merged
merged 6 commits into from
Dec 8, 2015

Conversation

jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Dec 7, 2015

This commit asks Travis to run the tests twice: once with all the requirements installed (SETUP=='full'), and once with none of the optional dependencies installed (SETUP=='minimal'). The minimal setup, has to run without the MKL as the conda mkl package pulls scipy with it.

The build fails which is expected. Indeed, until #568 get merged, some tests fail if scipy or netcdf are not installed.

Fix #483

- [email protected]
- MDA_DOCDIR=package/doc/html
matrix:
Copy link
Member

Choose a reason for hiding this comment

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

I prefer if this was at the top of the file where we define the python versions we use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 08/12/15 00:00, kain88-de wrote:

In .travis.yml
#570 (comment):

\ No newline at end of file

  • global:
    • secure="HIj3p+p2PV8DBVg/KGUx6n83KwB0ASE5FwOn0SMB9zxnzAqe8sapwdBQdMdq0s XB7xT1spJqRxuxOMVEVn35BNLu7bxMLfa4287C8YXcomnvmv9xruxAsjsIewnNQ80vtPVbQddBPxa4 jKbqgPby5QhhAP8KANAqYe44pIV70fY="
    • GH_DOC_BRANCH=develop
    • GH_REPOSITORY=github.com/MDAnalysis/mdanalysis.git
    • GIT_CI_USER=TravisCI
    • MDA_DOCDIR=package/doc/html
  • matrix:

I prefer if this was at the top of the file where we define the python
versions we use.


Reply to this email directly or view it on GitHub
https://github.com/MDAnalysis/mdanalysis/pull/570/files#r46891970.

I prefer it at the top too. But it was at the end so I left it there. I
can move it up.

Copy link
Member

Choose a reason for hiding this comment

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

By all means, move it up. I might have added it and I just followed other examples without much thinking.

@richardjgowers
Copy link
Member

This is awesome, thanks. It looks like there's just a few things that need decorating with knownfail

- source activate pyenv
- conda install --yes python=$TRAVIS_PYTHON_VERSION cython biopython matplotlib networkx netcdf4
- if [[ $SETUP == 'full' ]]; then conda install --yes python=$TRAVIS_PYTHON_VERSION cython biopython matplotlib networkx netcdf4; fi
- if [[ $SETUP == 'minimal' ]]; then conda install --yes python=$TRAVIS_PYTHON_VERSION cython biopython networkx; fi
Copy link
Member

Choose a reason for hiding this comment

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

minimal should work without cython.

Copy link
Member

Choose a reason for hiding this comment

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

Hm depending on when #560 get's merged maybe we can also say that a 'full' setup means we cythonize the files again and in the minimal setup we use the shipped *.c files.

This commit asks Travis to run the tests twice: once with all the
requirements installed (SETUP=='full'), and once with none of the
optional dependencies installed (SETUP=='minimal').

Note that the minimal setup, runs without the MKL as the conda mkl
package pulls scipy with it.
Tests often fail in ways that seem related to timeout issue. Indeed,
Travis can be slow sometime. This commit increases the timeout for tests
from 120 seconds to 300 seconds.
Build the full setup with and without cythonize; build the minimal setup
only without cythonize.
This makes the file easier to understand as a reader see where the
variable come from before they are used.
@jbarnoud
Copy link
Contributor Author

jbarnoud commented Dec 8, 2015

I rebased against develop to account for #560. I also adapted the matrix to follow @richardjgowers suggestions in #560; 3 versions are built:

  • full install with cythonize
  • full install without cythonize
  • minimal install without cythonize

I also removed cython from the packages installed in the minimal setup, and I moved the environment variable at the top of the file to improve readability.

@kain88-de
Copy link
Member

I don't think we need to run 3 versions. All the cython-compiled C-files will also be tested in the minimal install since none of them is optional (and they shouldn't be since then people who have installed cython but not the optional dependencies might have problems).

And also consider that we will have double the number of build-jobs as soon as we support python 3 as well. With this in mind and travis asking nicely that we keep the build-matrix minimal I think we can skip full install without cythonize.

Please take into account that Travis CI is an open source service and we rely on worker boxes provided by the community. So please only specify as big a matrix as you actually need.

@jbarnoud
Copy link
Contributor Author

jbarnoud commented Dec 8, 2015

I don't mind removing the full install without cythonize. It is just a matter of removing one line. What are the cases where the build with cythonize can pass while the build whithout fails? Or vice-versa?

@kain88-de
Copy link
Member

It can happen that someone doesn't update the cython generated C-files. If a bug was fixed in the pyx files that can lead to the tests failing for that bug.

@richardjgowers
Copy link
Member

I don't mind either way, so I'll let @kain88-de decide

Remove the build with full install and no cythonize as suggested by
@kain88-de in MDAnalysis#570.
@jbarnoud
Copy link
Contributor Author

jbarnoud commented Dec 8, 2015

If @kain88-de decides, then I remove the useless build 😛

kain88-de added a commit that referenced this pull request Dec 8, 2015
 Travis run the test in minimal and full config
@kain88-de kain88-de merged commit 05fdf57 into MDAnalysis:develop Dec 8, 2015
@jbarnoud jbarnoud deleted the matrix-PR branch January 19, 2016 10:11
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