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

Make parallel tests insensitive to user configuration #8727

Merged
merged 9 commits into from May 23, 2019

Conversation

Projects
None yet
3 participants
@astrofrog
Copy link
Member

commented May 18, 2019

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 astrofrog force-pushed the astrofrog:ignore-cfg branch from ee7f186 to 587be54 May 18, 2019

@astrofrog astrofrog requested review from bsipocz and saimn May 18, 2019

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

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

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

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

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)

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

@codecov

This comment has been minimized.

Copy link

commented May 18, 2019

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

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

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

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

@astrofrog

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

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.

@@ -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')

This comment has been minimized.

Copy link
@saimn

saimn May 19, 2019

Contributor

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

This comment has been minimized.

Copy link
@pllim

pllim May 19, 2019

Member

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?

This comment has been minimized.

Copy link
@astrofrog

astrofrog May 23, 2019

Author Member

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

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

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

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@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.

astrofrog added some commits May 18, 2019

Write out custom configuration to the standard location to ensure tha…
…t the astropy testing framework ignores it, especially in parallel mode.
Set temporary config/cache in conftest.py since this is needed when r…
…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.
Set temporary directories for config/cache in root-level conftest.py …
…since this is needed when running pytest directly, in particular if running ``pytest docs/`` (which therefore ignores any conftest.py inside astropy/``

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

@pllim

pllim approved these changes May 23, 2019

Copy link
Member

left a comment

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

@saimn

saimn approved these changes May 23, 2019

Copy link
Contributor

left a comment

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?

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

15 checks passed

astropy-bot:changelog Changelog entry consistent with milestone
astropy-bot:milestone This pull request has a milestone set.
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl202 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl222 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl300 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpldev Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing b2b0717...edf94a9
Details
codecov/project 86.95% (+<.01%) compared to b2b0717
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details
@pllim

This comment has been minimized.

Copy link
Member

commented May 23, 2019

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... ?

@saimn saimn referenced this pull request May 23, 2019

Open

Cache for test files #8748

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

Merge pull request #8727 from astrofrog/ignore-cfg
Make parallel tests insensitive to user configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.