Make parallel tests insensitive to user configuration #8727

astrofrog · 2019-05-18T20:00:27Z

This is an attempt to fix #8691 - I discovered that when run in parallel mode, the user configuration is not ignored.

The first step here is to add a regression test by writing out to the config file before running the parallel tests on CircleCI. This failed, so I'm now pushing some fixes.

It turns out there are several places where we need to set the config/cache to be temporary directories, not just in the test runner:

astropy/conftest.py - we need to set the path to the temporary config/cache directories here for cases when running the tests in parallel mode using e.g.

python setup.py test --parallel

This is because pytest starts up new subprocesses, which aren't aware of the temporary directories set in the test runner.

conftest.py - the temporary paths need to be set here for when pytest is run directly on an astropy package, e.g.

pytest astropy/io/votable

docs/conftest.py - we also need to set them here for when running the tests with python setup.py test -t docs/... since then the root-level conftest.py gets ignored (since the tests run from outside the source directory, and astropy/conftest.py gets ignored since we are only testing things in docs

This will become simpler some day if we switch to only using pytest directly.

Fixes #8154

astrofrog · 2019-05-18T20:47:32Z

@saimn - could you check if this fixes #8691 for you?

astrofrog · 2019-05-18T21:23:28Z

The Matplotlib developer build can be ignored, this is due I think to the 3.1.x release today (will fix in a separate PR)

codecov · 2019-05-18T22:09:13Z

Codecov Report

Merging #8727 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8727      +/-   ##
==========================================
+ Coverage   86.95%   86.95%   +<.01%     
==========================================
  Files         399      399              
  Lines       59355    59355              
  Branches     1100     1100              
==========================================
+ Hits        51613    51615       +2     
+ Misses       7101     7099       -2     
  Partials      641      641

Impacted Files	Coverage Δ
astropy/tests/runner.py	`77.62% <ø> (ø)`	⬆️
astropy/config/configuration.py	`83.78% <0%> (+0.38%)`	⬆️
astropy/config/paths.py	`65.32% <0%> (+0.8%)`	⬆️

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 b2b0717...edf94a9. Read the comment docs.

saimn · 2019-05-18T22:18:25Z

Sadly this doesn't fix #8691, but I agree that this looks a good idea.

astrofrog · 2019-05-18T22:21:18Z

@saimn - huh weird it does fix it for me locally... (and I could reproduce your issue). Just to be clear, you still get the test failure with:

python setup.py test -R -j 2 -t docs/timeseries/index.rst

? or are you running the tests differently? And can you confirm that you don't have any user configuration set?

saimn · 2019-05-18T22:28:53Z

@astrofrog - Yes, this command and no user configuration. How did you manage to reproduce?

astrofrog · 2019-05-18T22:30:15Z

@saimn - well I had to set the user configuration to include max_width and then run in parallel mode (which didn't ignore the user config). And this gave the behavior of working in serial mode and not parallel mode. Are you still seeing the issue only in parallel mode?

saimn · 2019-05-18T22:38:37Z

@astrofrog - ok, I agree that there is an potential issue with local config in parallel mode, which is good to fix, but in my case this is not the problem. In my case it seems that the COLUMNS environment variable is set in parallel mode (#8691 (comment)), which makes terminal_size to return the real terminal size. I don't yet understand where this variable comes from.

astrofrog · 2019-05-18T22:44:48Z

@saimn - ah ok I see. Regardless of why xdist does that, we should probably make sure we set COLUMNS and LINES to constant values in the conftest.py files since it seems that's something we'd want to ignore, especially if a user had those set by default?

saimn · 2019-05-18T23:10:31Z

Reading a bit more (https://unix.stackexchange.com/questions/215584/whats-the-name-of-the-environment-variable-with-current-terminal-width), COLUMNS (and LINES) is only a shell variable and not exported into the environment. So they are not supposed to be set manually, the question is why in this specific case it is available as an environment variable.

astrofrog · 2019-05-19T10:14:39Z

Thanks for looking into this further! So I think the way forward is to still consider this PR since it addresses #8154, and then to not consider #8691 as critical for v3.2 since it's a reasonably advanced use case and doesn't seem to happen systematically on all machines. Let's continue to discuss that and explore possibilities in #8691.

saimn · 2019-05-19T21:58:01Z

astropy/conftest.py

@@ -52,6 +65,9 @@ def pytest_unconfigure(config):
        matplotlib.rcParams.update(matplotlibrc_cache)
        matplotlibrc_cache.clear()

+    os.environ.pop('XDG_CONFIG_HOME')
+    os.environ.pop('XDG_CACHE_HOME')
+


Why is this needed here but not in the other conftest files ?

I don't know how I feel about playing with env var in general in conftest. Let's say user sets custom vars, then run tests, conftest messes them up, now user complains that the these vars are not what they set anymore. Can this happen?

The reason it's needed here and not in the other conftest.py files is because this is the only one which might be used inside a persistent python session such as IPython. The other ones will only matter if you run pytest, and the Python process will end at the end of the tests. Any changes to os.environ don't change the variables beyond the Python process:

Python 3.7.1 (v3.7.1:260ec2c36a, Oct 20 2018, 03:13:28) [Clang 6.0 (clang-600.0.57)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import os >>> os.environ['FOO'] = 'BAR' >>> $ echo $FOO $

However, in this specific conftest.py file I should probably indeed save any pre-existing variables and reset then in unconfigure.

saimn · 2019-05-19T22:10:36Z

Looks good to me, just a small question. Btw when running the tests a few days ago with pytest I found my user cache was deleted by the test_download_cache test (saimn@b5a4da5) :), so I'm happy to see a better fix.

It may be a bit sad to have to download all test files each time though. Could we instead put the test files in a specify directory in ~/.astropy ?

astrofrog · 2019-05-23T09:32:42Z

@saimn @pllim - I've now added code to restore the variables if the user is testing from e.g. IPython. I've also added a note to the other conftest.py files to explain why we don't need to unset the variables.

Good question about avoiding downloading all the files every time. Strictly speaking it's probably cleaner since it ensures an isolated environment, but if we wanted to avoid that we could change just the config but not the cache path for the tests.

…t the astropy testing framework ignores it, especially in parallel mode.

…unning the tests in parallel mode, as this uses subprocesses which otherwise don't know about the temporary config/cache set in the test runner. We need to keep the temporary config/cache in the test runner too though since this matters when running the tests in e.g. IPython.

…since this is needed when running pytest directly, in particular if running ``pytest docs/`` (which therefore ignores any conftest.py inside astropy/``

…th ``python setup.py test -t docs/...``

…not ignored.

…ecated in recent versions of pytest)

pllim

Thanks for the detailed comments; I find them very helpful! As long as tests pass and @saimn is happy with this, LGTM.

saimn

It is a clear improvement so let's go for it.

It would be good to find a way to avoid downloading all the files every time, to reduce bandwidth usage. And these files are not supposed to change often. But we would need to do this for the non-parallel case as well, and maybe put the files in a specific directory?

pllim · 2019-05-23T15:22:44Z

It would be good to find a way to avoid downloading all the files every time

@saimn , feel free to open a new issue about it lest we forget. Maybe the problem would solve itself if we switch to git lfs... ?

Make parallel tests insensitive to user configuration

astrofrog added testing Work in progress labels May 18, 2019

astrofrog force-pushed the ignore-cfg branch from ee7f186 to 587be54 Compare May 18, 2019 20:42

astrofrog requested review from bsipocz and saimn May 18, 2019 20:44

astrofrog added this to the v3.2 milestone May 18, 2019

astrofrog removed the Work in progress label May 18, 2019

astrofrog mentioned this pull request May 18, 2019

TST: Investigate timeseries doctest failure with scipy 1.3.0rc2 #8695

Closed

astrofrog force-pushed the ignore-cfg branch from a30e5e2 to 8d148fe Compare May 18, 2019 22:09

astrofrog added the Ready-for-final-review label May 19, 2019

saimn reviewed May 19, 2019

View reviewed changes

astrofrog added 6 commits May 23, 2019 12:07

Write out custom configuration to the standard location to ensure tha…

6328482

…t the astropy testing framework ignores it, especially in parallel mode.

Set temporary directories for config/cache in root-level conftest.py …

6734237

…since this is needed when running pytest directly, in particular if running ``pytest docs/`` (which therefore ignores any conftest.py inside astropy/``

Updated comment

861817e

Added a conftest.py in docs/ since this is needed if running tests wi…

c0f7337

…th ``python setup.py test -t docs/...``

Write out user config on Travis that will cause the tests to fail if …

7ae04f8

…not ignored.

astrofrog added 3 commits May 23, 2019 12:07

Added changelog entry

483b912

Don't use pytest_plugins in a non-top-level conftest.py (this is depr…

9ab45d3

…ecated in recent versions of pytest)

Restore XDG_CONFIG_HOME and XDG_CACHE_HOME after tests where relevant.

edf94a9

astrofrog force-pushed the ignore-cfg branch from f7baeb8 to edf94a9 Compare May 23, 2019 11:07

pllim approved these changes May 23, 2019

View reviewed changes

saimn approved these changes May 23, 2019

View reviewed changes

saimn merged commit 7c4fe13 into astropy:master May 23, 2019

saimn mentioned this pull request May 23, 2019

Cache for test files #8748

Open

bsipocz pushed a commit that referenced this pull request May 27, 2019

Merge pull request #8727 from astrofrog/ignore-cfg

ad0eaa0

Make parallel tests insensitive to user configuration

saimn mentioned this pull request Nov 12, 2019

Persist astropy cache #9557

Open

pllim mentioned this pull request Jun 11, 2020

Remove unnecessary remote_data in (mostly) coordinates. #10474

Merged

saimn mentioned this pull request Jun 3, 2021

Caching of files for testing with remote_data #11811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make parallel tests insensitive to user configuration #8727

Make parallel tests insensitive to user configuration #8727

astrofrog commented May 18, 2019 •

edited

astrofrog commented May 18, 2019

astrofrog commented May 18, 2019 •

edited

codecov bot commented May 18, 2019 •

edited

saimn commented May 18, 2019

astrofrog commented May 18, 2019 •

edited

saimn commented May 18, 2019

astrofrog commented May 18, 2019 •

edited

saimn commented May 18, 2019

astrofrog commented May 18, 2019

saimn commented May 18, 2019

astrofrog commented May 19, 2019

saimn May 19, 2019

pllim May 19, 2019

astrofrog May 23, 2019

saimn commented May 19, 2019

astrofrog commented May 23, 2019

pllim left a comment

saimn left a comment

pllim commented May 23, 2019

Make parallel tests insensitive to user configuration #8727

Make parallel tests insensitive to user configuration #8727

Conversation

astrofrog commented May 18, 2019 • edited

astrofrog commented May 18, 2019

astrofrog commented May 18, 2019 • edited

codecov bot commented May 18, 2019 • edited

Codecov Report

saimn commented May 18, 2019

astrofrog commented May 18, 2019 • edited

saimn commented May 18, 2019

astrofrog commented May 18, 2019 • edited

saimn commented May 18, 2019

astrofrog commented May 18, 2019

saimn commented May 18, 2019

astrofrog commented May 19, 2019

saimn May 19, 2019

Choose a reason for hiding this comment

pllim May 19, 2019

Choose a reason for hiding this comment

astrofrog May 23, 2019

Choose a reason for hiding this comment

saimn commented May 19, 2019

astrofrog commented May 23, 2019

pllim left a comment

Choose a reason for hiding this comment

saimn left a comment

Choose a reason for hiding this comment

pllim commented May 23, 2019

astrofrog commented May 18, 2019 •

edited

astrofrog commented May 18, 2019 •

edited

codecov bot commented May 18, 2019 •

edited

astrofrog commented May 18, 2019 •

edited

astrofrog commented May 18, 2019 •

edited