Handle utils test warnings by pllim · Pull Request #9569 · astropy/astropy

pllim · 2019-11-08T15:42:14Z

Description

This pull request is to address test warnings emitted by utils. @aarchiba should have a look as she added most of these tests in #9182.

This is a part of #7928

aarchiba · 2019-11-08T16:10:38Z

I can't speak for the timer stuff but the rest should probably be turned into explicit checks that appropriate warnings are raised by specific lines. If the cache is readonly, for example, we want to confirm that operations that would normally write to it raise a relevant warning, and operations that only require reading don't warn.

pllim · 2019-11-09T19:26:08Z



-class TestCStack():
+class TestDStack():


@taldcroft , this okay?

Yes, thanks.

pllim · 2019-11-09T19:27:12Z

    assert not is_url_in_cache(nonexistent)


+@pytest.mark.filterwarnings('ignore:Not clearing data cache - cache inaccessible due to FileNotFoundError')


@aarchiba , seems like #9575 didn't cache this one. Is handling it like this okay?

I get no warnings on my machine, at least, though it's not tested in CI. No reason this should emit a warning - clear_download_cache should work fine (no warnings) on a nonexistent cache, and it shouldn't be running on a nonexistent cache here anyway. But if you can produce a warning, let me know and I'll try to track it down?

pllim · 2019-11-10T21:52:22Z

@aarchiba , here is what I get when running the test on my machine locally with python setup.py test -P utils and turning warning into error:

platform linux -- Python 3.7.3, pytest-5.0.1, py-1.8.0, pluggy-0.12.0

Running tests with Astropy version 4.1.dev26889.
Running tests in astropy/utils docs/utils.

Date: 2019-11-10T16:41:26

Platform: Linux-3.10.0-1062.1.2.el7.x86_64-x86_64-with-redhat-7.7-Maipo

Executable: .../miniconda/envs/py37/bin/python

Full Python Version: 
3.7.3 | packaged by conda-forge | (default, Jul  1 2019, 21:52:21) 
[GCC 7.3.0]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Package versions: 
Numpy: 1.17.3
Scipy: 1.3.1
Matplotlib: 3.1.1
h5py: not available
Pandas: 0.24.2
Cython: 0.29.13
Scikit-image: not available
asdf: 2.4.0.dev63+gb67cb02
astropy-helpers: 3.2rc1

Using Astropy options: remote_data: none.

rootdir: /tmp/astropy-test-5dwkmwkz/lib/python3.7/site-packages, inifile: setup.cfg
plugins: doctestplus-0.4.0, remotedata-0.3.1, openfiles-0.3.2, arraydiff-0.3,
astropy-header-0.1.1, forked-1.1.3, xdist-1.30.0,
ci-watson-0.0.0.dev76+g4b73d3b5, asdf-2.4.0.dev63+gb6

=== FAILURES ===
___ test_check_download_cache ___

hashorurl = None, pkgname = 'astropy'

    def clear_download_cache(hashorurl=None, pkgname='astropy'):
        # ...
                else:  # it's a path
                    filepath = os.path.join(dldir, hashorurl)
                    if not _is_inside(filepath, dldir):
                        # Should this be ValueError? IOError?
                        raise RuntimeError(
                            f"attempted to use clear_download_cache on the path "
                            f"{filepath} outside the data cache directory {dldir}")
                    if os.path.exists(filepath):
                        # Convert to list because we'll be modifying it as we go
                        for k, v in list(url2hash.items()):
                            if v == filepath:
                                del url2hash[k]
                        os.unlink(filepath)
>                       return

astropy/utils/data.py:1439: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <contextlib._GeneratorContextManager object at 0x7f26b40fc080>, type = None, value = None, traceback = None

    def __exit__(self, type, value, traceback):
        if type is None:
            try:
>               next(self.gen)

.../miniconda/envs/py37/lib/python3.7/contextlib.py:119: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

pkgname = 'astropy', write = True

    @contextlib.contextmanager
    def _cache(pkgname, write=False):
        # ...
        if write:
            with _cache_lock(pkgname, need_write=True), \
                    shelve.open(urlmapfn, flag="c") as url2hash:
>               yield dldir, url2hash

astropy/utils/data.py:1626: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <shelve.DbfilenameShelf object at 0x7f26b40fc3c8>, type = None, value = None, traceback = None

    def __exit__(self, type, value, traceback):
>       self.close()

.../miniconda/envs/py37/lib/python3.7/shelve.py:138: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <shelve.DbfilenameShelf object at 0x7f26b40fc3c8>

    def close(self):
        if self.dict is None:
            return
        try:
>           self.sync()

.../miniconda/envs/py37/lib/python3.7/shelve.py:144: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <shelve.DbfilenameShelf object at 0x7f26b40fc3c8>

    def sync(self):
        if self.writeback and self.cache:
            self.writeback = False
            for key, entry in self.cache.items():
                self[key] = entry
            self.writeback = True
            self.cache = {}
        if hasattr(self.dict, 'sync'):
>           self.dict.sync()

.../miniconda/envs/py37/lib/python3.7/shelve.py:172: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <dbm.dumb._Database object at 0x7f26b40fc240>

    def _commit(self):
        # CAUTION:  It's vital that _commit() succeed, and _commit() can
        # be called from __del__().  Therefore we must never reference a
        # global in this routine.
        if self._index is None or not self._modified:
            return  # nothing to do
    
        try:
            self._os.unlink(self._bakfile)
        except OSError:
            pass
    
        try:
            self._os.rename(self._dirfile, self._bakfile)
        except OSError:
            pass
    
>       with self._io.open(self._dirfile, 'w', encoding="Latin-1") as f:
E       FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-lim/pytest-75/test_check_download_cache0/astropy/download/py3/urlmap.dir'

.../miniconda/envs/py37/lib/python3.7/dbm/dumb.py:135: FileNotFoundError

During handling of the above exception, another exception occurred:

tmpdir = local('/tmp/pytest-of-lim/pytest-75/test_check_download_cache0'), temp_cache = None, valid_urls = <generator object valid_urls.<locals>._valid_urls at 0x7f26b4b4b138>
invalid_urls = <generator object invalid_urls.<locals>._invalid_urls at 0x7f26b411c0c0>

    def test_check_download_cache(tmpdir, temp_cache, valid_urls, invalid_urls):
        testurl, testurl_contents = next(valid_urls)
        testurl2, testurl2_contents = next(valid_urls)
    
        zip_file_name = os.path.join(tmpdir, "the.zip")
>       clear_download_cache()

astropy/utils/tests/test_data.py:1009: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

hashorurl = None, pkgname = 'astropy'

    def clear_download_cache(hashorurl=None, pkgname='astropy'):
        # ...
        except OSError as e:
            msg = 'Not clearing data cache - cache inaccessible due to '
            estr = '' if len(e.args) < 1 else (': ' + str(e))
>           warn(CacheMissingWarning(msg + e.__class__.__name__ + estr))
E           astropy.utils.data.CacheMissingWarning:
E           Not clearing data cache - cache inaccessible due to FileNotFoundError:
E           [Errno 2] No such file or directory:
E '/tmp/pytest-of-lim/pytest-75/test_check_download_cache0/astropy/download/py3/urlmap.dir'

astropy/utils/data.py:1446: CacheMissingWarning

pllim · 2019-11-10T22:00:24Z


    zip_file_name = os.path.join(tmpdir, "the.zip")
-    clear_download_cache()
+    with pytest.warns(CacheMissingWarning, match='Not clearing data cache - '


@aarchiba , I made it much more explicit now. So if indeed it is just my machine acting weird, the jobs would fail in CI.

This is still likely to be trouble - it will fail on any machine (like mine) that doesn't emit the warning. There's a filterwarnings context manager that would succeed in ignoring the warning but I think it's better to prevent the warning being emitted - it is fine if clear_download_cache() is called when no cache exists, and we shouldn't be emitting warnings.

Wait. I think what's going on is maybe that the shelve is opened, then the on-disk file is deleted, then the shelve is closed, and this backend doesn't like that. Which is not unreasonable. But I can catch that exception and let it go without a warning, because this only happens with a no-argument clear_download_cache().

pllim · 2019-11-10T22:47:54Z

This is weird, seems like that warning is not emitted in CircleCI 32-bit job and Travis CI Python 3.8 job, but the other Travis CI jobs are seemed to catch it. (The other CircleCI jobs are irrelevant here.)

aarchiba · 2019-11-11T00:33:53Z

There is some difference based on which dbm backend is used by shelve - gdbm versus dbm versus dumb.py. It might be possible to force all machines everywhere to use dumb.py, which would resolve certain problems - for example if your astropy installation creates a gdbm cache index but then you try to run astropy in conda, which doesn't provide gdbm, astropy will choke. This would have the side effect of making the shelve behaviour much more uniform (but also slower - though I'm not sure speed is much of a concern for the cache). I can also poke around and try to ensure the warning is not emitted when it's not necessary (the case here) but it'll be tricky unless I can arrange to control the database backend (I have ideas). I guess I should create an issue about database format.

pllim · 2019-11-11T01:09:18Z

Thanks for the insight, @aarchiba ! Maybe I'll just not handling this one warning for now in this PR.

If necessary, to get #7928 in for 4.0, I can put in the blind pytest.mark.filterwarnings again on it at the last minute.

astrofrog

This looks uncontroversial now that the clear_download_cache warning has been deferred.

You might want to consider squashing the last two commits to avoid adding and removing changes related to the deferred changes?

for a future PR. Remove unused import.

pllim · 2019-11-11T16:11:49Z

I cancelled the rest of CircleCI tests, as they are unnecessary and the queue is too long.

Handle utils test warnings

pllim added testing utils Affects-dev PRs and issues that do not impact an existing Astropy release labels Nov 8, 2019

pllim added this to the v4.0 milestone Nov 8, 2019

pllim requested a review from mhvk November 8, 2019 15:42

aarchiba reviewed Nov 8, 2019

View reviewed changes

Comment thread astropy/utils/tests/test_data.py Resolved

This comment has been minimized.

Sign in to view

aarchiba mentioned this pull request Nov 9, 2019

Test for warnings when writing read-only cache #9575

Merged

pllim mentioned this pull request Nov 9, 2019

Change name of table.cstack to dstack #9566

Merged

pllim force-pushed the utils-tst-warn branch from d372c61 to c0ab82d Compare November 9, 2019 19:25

pllim commented Nov 9, 2019

View reviewed changes

pllim requested a review from taldcroft November 9, 2019 19:27

pllim added the table label Nov 9, 2019

pllim force-pushed the utils-tst-warn branch from c0ab82d to 798a8ef Compare November 10, 2019 21:59

pllim commented Nov 10, 2019

View reviewed changes

astrofrog approved these changes Nov 11, 2019

View reviewed changes

pllim added 3 commits November 11, 2019 10:33

Handle utils test warnings

cc181e5

Rename table test to TestDStack, xref astropy#9566

18d681e

Defer test_check_download_cache warning handling

1fa9e0c

for a future PR. Remove unused import.

pllim force-pushed the utils-tst-warn branch from 0408da2 to 1fa9e0c Compare November 11, 2019 15:34

pllim added the zzz 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Nov 11, 2019

pllim merged commit 3bb8685 into astropy:master Nov 11, 2019

pllim deleted the utils-tst-warn branch November 11, 2019 16:12

bsipocz pushed a commit that referenced this pull request Nov 18, 2019

Merge pull request #9569 from pllim/utils-tst-warn

f229724

Handle utils test warnings

		assert not is_url_in_cache(nonexistent)


		@pytest.mark.filterwarnings('ignore:Not clearing data cache - cache inaccessible due to FileNotFoundError')



		class TestCStack():
		class TestDStack():

Conversation

pllim commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

aarchiba commented Nov 8, 2019

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

pllim Nov 9, 2019

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

pllim Nov 9, 2019

Choose a reason for hiding this comment

Uh oh!

aarchiba Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

pllim commented Nov 10, 2019

Uh oh!

pllim Nov 10, 2019

Choose a reason for hiding this comment

Uh oh!

aarchiba Nov 11, 2019

Choose a reason for hiding this comment

Uh oh!

pllim commented Nov 10, 2019

Uh oh!

aarchiba commented Nov 11, 2019

Uh oh!

pllim commented Nov 11, 2019

Uh oh!

astrofrog left a comment • 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!

pllim commented Nov 11, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pllim commented Nov 8, 2019 •

edited

Loading

astrofrog left a comment •

edited

Loading