[WIP] Using Tox by saimn · Pull Request #8248 · astropy/astropy

saimn · 2018-12-07T12:16:30Z

This adds a tox config, with envs both for pytest and setup.py, and use it on Travis and CircleCI (docs).
If run in a python with conda and tox-conda, then a conda env is used.
So this still relies on ci-helpers to install miniconda, but then the other features like pinning versions are not used. Discussion needed here. We could imagine running a script inside the conda env to do what is done currently by ci-helpers, but I'm not sure about what exactly is needed here (it would probably be great to simplify this!).

❯ tox -lv
using tox.ini: /home/simon/dev/astropy/tox.ini
using tox-3.5.3 from /home/simon/.pyenv/versions/3.7.0/lib/python3.7/site-packages/tox/__init__.py
default environments:
py35-pytest     -> run tests with pytest
py35-pytest-all -> run tests with pytest
py35-setup      -> run tests with python setup.py
py35-setup-all  -> run tests with python setup.py
py36-pytest     -> run tests with pytest
py36-pytest-all -> run tests with pytest
py36-setup      -> run tests with python setup.py
py36-setup-all  -> run tests with python setup.py
py37-pytest     -> run tests with pytest
py37-pytest-all -> run tests with pytest
py37-setup      -> run tests with python setup.py
py37-setup-all  -> run tests with python setup.py
docs            -> invoke sphinx-build to build the HTML docs
dev             -> run with numpy and matplotlib dev versions
flake8          -> [no description]

TODO:

Appveyor
need more flexibility with tox-conda to use conda only on a given env ? currently everything is run with conda on Travis
testing the installed version. Currently usedevelop is always used, I will come back to this later.
doctests (how is this done currently?)

Codecov Report

Merging #8248 into master will increase coverage by 1.58%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8248      +/-   ##
==========================================
+ Coverage   86.78%   88.36%   +1.58%     
==========================================
  Files         387      367      -20     
  Lines       58110    54022    -4088     
  Branches     1060      316     -744     
==========================================
- Hits        50428    47734    -2694     
+ Misses       7067     6107     -960     
+ Partials      615      181     -434

Impacted Files	Coverage Δ
astropy/tests/plugins/config.py	`82.35% <0%> (-17.65%)`	⬇️
astropy/table/bst.py	`63.76% <0%> (-14.72%)`	⬇️
astropy/utils/console.py	`62.15% <0%> (-4.79%)`	⬇️
astropy/io/misc/asdf/tags/table/table.py	`90% <0%> (-2%)`	⬇️
astropy/cosmology/core.py	`94.04% <0%> (-1.56%)`	⬇️
astropy/stats/lombscargle/core.py	`94.54% <0%> (-1.05%)`	⬇️
astropy/io/ascii/src/tokenizer.c	`86.77% <0%> (-0.81%)`	⬇️
astropy/tests/runner.py	`76.81% <0%> (-0.46%)`	⬇️
astropy/modeling/utils.py	`83% <0%> (-0.45%)`	⬇️
astropy/utils/xml/writer.py	`81.34% <0%> (-0.34%)`	⬇️
... and 57 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 a865e69...0e7a99c. Read the comment docs.

pllim · 2018-12-07T15:54:15Z

cc @drdavella

astrofrog

This is great! Just a few small comments. It would indeed be nice to see this in use for AppVeyor too.

astrofrog · 2018-12-07T15:07:19Z

+               TOXARGS='--remote-data=astropy'
+               # EVENT_TYPE='cron'
+               # CONDA_DEPENDENCIES="$CONDA_ALL_DEPENDENCIES clang"
+               # CCOMPILER=clang


For now, to keep this configuration equivalent, I think you could do:

EVENT_TYPE='cron' CONDA_DEPENDENCIES=clang CCOMPILER=clang

(I don't think this specific customization of the build belongs in tox.ini)

I don't think it would work: I can pass CC to the tox env, but currently the CONDA_DEPENDENCIES are not installed in the tox-conda envs. There are still installed by ci-helpers in the root env though, which is useless. If we want to keep these customization of the test env by ci-helpers, we could split it in at least two functions: one to just install miniconda, and another that would be called in the tox env.

Also why using conda's clang here ? osx is using clang by default isn't it ?

Sorry I didn't reply to this sooner - I think the motivation for this might have been when we were considering adding some OpenMP functionality, but since we didn't do that in the end I think it's fine to drop this.

mhvk · 2018-12-07T16:27:58Z

Tox seems to be interesting, but it would be good to clearly state what problem we are trying to solve with it, as it is another layer of indirection that may make the testing framework more, not less confusing (I'm certainly confused...).

Also, definitely needs documentation! For instance, it has to be obvious what should go in setup.cfg and what in tox.ini.

Also, with this, the framework for astropy will diverge even more than is presently the case for the default setup for affiliated packages (which uses much more basic Ubuntu setups on travis - which I must admit I find easier to understand than what we do in astropy). It would be good if instead we could move to a model where astropy matches more closely what is done in the affiliated packages. Again good documentation will be key.

drdavella · 2018-12-07T16:37:56Z

Here's an unsolicited opinion: tox should not ever be used to run setup.py test. Instead, it should only ever call pytest directly. A big reason for using tox is that it already handles all of the packaging and environment creation that we try to do in setup.py test. If we buy into tox, we can potentially stop maintaining this functionality entirely. Maybe this answers part of @mhvk's question.

The other major motivation for tox, as I see it, is the ability for push-button reproduction of CI environments. Currently it can be very difficult to reproduce tests that fail on CI. But with tox, we can keep large parts of the test environment constant between CI and local runs, which should make failures much easier to reproduce locally.

Yet another motivation is that the use of tox potentially obsoletes large parts of ci-helpers, which could allow us to substantially reduce the maintenance burden there.

TL;DR
The main motivations for exploring tox are:

allows for push-button reproduction of Python environments on CI and locally
potentially obsoletes large and complicated parts of Astropy's test infrastructure
potentially obsoletes large parts of ci-helpers
Allows us to replace complicated and painful-to-maintain parts of our custom infrastructure with a tool that is very widely used in the rest of the Python community

drdavella · 2018-12-07T16:43:26Z

+    which python
+    pip freeze -l
+    pytest: pytest {posargs}
+    setup: python setup.py test {posargs}


I mention this in the comments, but I don't think that it makes sense to run setup.py test using tox. Basically this means that Astropy is creating yet another test environment within a tox environment, which, in my opinion, is exactly what we'd like to avoid. Maybe in the short term we want to maintain this as a sanity check, but our eventual goal should be to get rid of it.

setup.py test is even actively discouraged in the docs: https://tox.readthedocs.io/en/latest/example/basic.html#integration-with-setup-py-test-command

This really should be simply pytest test {posargs}

drdavella · 2018-12-07T16:45:10Z

+    -rpip-requirements-dev
+commands =
+    pip install git+https://github.com/numpy/numpy.git#egg=numpy
+    pip install git+https://github.com/matplotlib/matplotlib.git#egg=matplotlib


Would it make more sense to move these to the deps section?

This would conflict with pip-requirements-dev I think.

Okay. I understand the reason for using a pip-requirements file but I wonder if you could have finer-grained control over dependencies by just including them specifically in the tox.ini file. Then you could prevent the conflict by using factors.

mhvk · 2018-12-07T16:53:57Z

@drdavella - thanks for the explanation of the use! It would indeed seem good to reduce the extra code we now have to help us test things, etc., especially as so few of us understand how it works. In that respect, you made quite an improvement already by making it possible to run pytest directly. Ideally, the setup.py way of doing it just becomes a nice short-cut (nice in the sense that setup.py --help tells you what is possible).

saimn · 2018-12-07T16:57:20Z

@mhvk - there were many discussions on this recently so I did not take the time to explain more, sorry. The goal is really to simplify things, so don't be afraid ;).
The first goal is to make it easy to run test in a reproducible environment, the same locally and on Travis. Without duplicating dependencies in various places, running tests could be done the same way on Travis, CircleCI, Azure etc, without relying on specific scripts to setup the environments.
The future goal would be to remove setup.py test as tox provides a way to run tests in an isolated environment.

saimn · 2018-12-07T17:03:42Z

@drdavella - about setup.py test, this was a suggestion from @astrofrog to avoid changing too many things at once, which I think is reasonable. There nothing wrong about running setup.py test inside tox I think, we still get the improvement on the dependencies installation. We also need to find a way to test the installed version, which setup.py test does.

mhvk · 2018-12-07T17:06:28Z

@saimn - OK, thanks. Please do link to those discussions at the top...

drdavella · 2018-12-07T17:33:41Z

@saimn yes that's reasonable, and I agree. We should just remember that the eventual goal should be to remove this duplication. Eventually I think it might be reasonable for setup.py test to really just be an alias for tox.main that uses a particular standard environment.

Cadair · 2018-12-07T18:59:45Z

@@ -0,0 +1,68 @@
+[tox]
+envlist = py{35,36,37}-{pytest,setup}{,-all},docs,dev,flake8
+skipsdist = True


Dosen't this mean that astropy dosen't get installed into an isolated environment by tox?

This only means that no package should be built. That plays together with using usedevelop = True to test the project installed in develop mode (a.k.a. pip install --editable <project path>).

Cadair · 2018-12-07T19:00:40Z

+           setuptools >= 30.0.0
+
+[testenv]
+usedevelop = True


Same comment as above about the skipsdist stuff?

astrofrog · 2018-12-07T20:22:39Z

Just to mention another benefit of tox: most of the configuration for CI then lives in a single file regardless of CI platform, which makes it much easier to switch between different CI services and also means that most users (once we do this in the package-template) will just need to worry about editing tox.ini rather than each individual CI config file when e.g. updating dependencies.

astrofrog · 2018-12-07T22:48:49Z

@saimn - I've merged #8198, so could you see if you can use the extras_require instead?

saimn · 2018-12-08T00:36:16Z

Latest news:

updated with extra_requires, which works well, except for bintrees which doesn't compile on py37 with clang. I removed it as it was not in the requirements file, I think it was deprecated in favor of sortedcontainers.
--force-dep works only for packages listed in deps ("--force-dep" fails to parse dependencies from requirements file tox-dev/tox#534, wontfix), we could do with it but I'm trying to replace it with environment variables, which would also also to specify numpy version for the conda package.

saimn · 2018-12-09T23:04:53Z

The pytest 3.7 builds are failing with doctestplus, is that a known issue ? cc @drdavella

______________________________ ERROR collecting  _______________________________
/home/travis/build/saimn/astropy/.tox/py35-setup-all/lib/python3.5/site-packages/_pytest/runner.py:201: in __init__
    self.result = func()
/home/travis/build/saimn/astropy/.tox/py35-setup-all/lib/python3.5/site-packages/_pytest/runner.py:261: in <lambda>
    call = CallInfo(lambda: list(collector.collect()), "collect")
/home/travis/build/saimn/astropy/.tox/py35-setup-all/lib/python3.5/site-packages/_pytest/main.py:475: in collect
    for x in self._collect(arg):
/home/travis/build/saimn/astropy/.tox/py35-setup-all/lib/python3.5/site-packages/_pytest/main.py:519: in _collect
    for x in root._collectfile(pkginit):
E   AttributeError: 'DocTestModulePlus' object has no attribute '_collectfile'

saimn · 2019-02-15T00:00:17Z

I rebased and added the pyXX-...-dev envs.

saimn · 2019-02-15T21:43:52Z

Tests are failing because of a Numpy multiarray failure on Windows, and pytest_plugins on Travis:

Defining 'pytest_plugins' in a non-top-level conftest is no longer supported because it affects the entire directory tree in a non-explicit way.
/home/travis/build/astropy/astropy/astropy/conftest.py
Please move it to a top level conftest file at the rootdir:
/home/travis/build/astropy/astropy

I hope that I did not introduce regression with the rebase..
@drdavella - any idea about the pytest_plugins issue ?

astropy-bot · 2019-07-21T08:01:37Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

astropy-bot · 2019-08-22T08:01:39Z

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

astrofrog · 2019-08-22T12:12:44Z

Let's plan to re-open this once/if the APE on the future astropy-helpers is approved.

astrofrog · 2019-12-05T13:53:16Z

@saimn - I've squashed and included the commits from here in #9726 (and kept you as an author)

pllim added testing Refactoring Work in progress labels Dec 7, 2018

astrofrog requested changes Dec 7, 2018

View reviewed changes

astrofrog mentioned this pull request Dec 7, 2018

Added (back) tox.ini file #8202

Closed

drdavella reviewed Dec 7, 2018

View reviewed changes

mhvk mentioned this pull request Dec 7, 2018

Move a large chunk of the setup.py configuration to setup.cfg #8240

Merged

Cadair reviewed Dec 7, 2018

View reviewed changes

saimn force-pushed the tox branch from a2fed50 to fc80038 Compare December 7, 2018 22:14

saimn force-pushed the tox branch 2 times, most recently from 6d75e27 to 1be5d13 Compare December 8, 2018 00:30

saimn force-pushed the tox branch from 1be5d13 to bd04cc0 Compare December 9, 2018 23:02

saimn force-pushed the tox branch 4 times, most recently from 98ccd83 to 243bde2 Compare December 10, 2018 12:06

saimn added 13 commits February 15, 2019 00:41

Use tox for travis

6643427

Use tox for the docs build

a87005e

Rename pytest envs, fix dev

4952a39

Fix codecov command

8030e3b

Pass CC & CFLAGS to tox and enhance descriptions

597d342

Use extra_requires

210b95e

Try no quoting for force-dep

617cc2e

Try appveyor

4b24e51

Remove bintrees

d807341

Use env var to specify numpy and pytest versions

853a5ed

Use pytest 3.6

6e26ee0

Rename env var to avoid conflict with ci-helpers

961172c

Setup builds without conda

693ee8a

saimn force-pushed the tox branch from bcb371e to 693ee8a Compare February 14, 2019 23:48

saimn added 3 commits February 15, 2019 00:49

Rename dev env

894de97

Add comment about conda_deps

d1adc3f

Remove commented stuff

46bb6b7

astrofrog reviewed Feb 15, 2019

View reviewed changes

Comment thread tox.ini Resolved

astrofrog reviewed Feb 15, 2019

View reviewed changes

Comment thread tox.ini Resolved

Small updates following @astrofrog comments

0e7a99c

astropy-bot Bot added the closed-by-bot Closed by stale bot label Aug 22, 2019

astropy-bot Bot closed this Aug 22, 2019

astrofrog mentioned this pull request Dec 5, 2019

Refactor infrastructure as described in astropy-helpers removal APE #9726

Merged

12 tasks done

saimn deleted the tox branch June 11, 2020 20:03

Conversation

saimn commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pllim commented Dec 7, 2018

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhvk commented Dec 7, 2018

Uh oh!

drdavella commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drdavella Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obestwalter Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Dec 7, 2018

Uh oh!

saimn commented Dec 7, 2018

Uh oh!

saimn commented Dec 7, 2018

Uh oh!

mhvk commented Dec 7, 2018

Uh oh!

drdavella commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Dec 7, 2018

Uh oh!

astrofrog commented Dec 7, 2018

Uh oh!

saimn commented Dec 8, 2018

Uh oh!

saimn commented Dec 9, 2018

Uh oh!

saimn commented Feb 15, 2019

Uh oh!

Uh oh!

Uh oh!

saimn commented Feb 15, 2019

Uh oh!

astropy-bot Bot commented Jul 21, 2019

Uh oh!

astropy-bot Bot commented Aug 22, 2019

Uh oh!

astrofrog commented Aug 22, 2019

saimn commented Dec 7, 2018 •

edited

Loading

codecov Bot commented Dec 7, 2018 •

edited

Loading

drdavella commented Dec 7, 2018 •

edited

Loading

drdavella Dec 7, 2018 •

edited

Loading

obestwalter Jan 9, 2019 •

edited

Loading