[MRG+2] MAINT: Refactor the converted-image cache #7764

jkseppan · 2017-01-07T22:12:48Z

There is a cache of png files keyed by the MD5 hashes of corresponding svg and pdf files, which helps reduce test suite running times for svg and pdf files that stay exactly the same from one run to the next.

This patch enables caching of test results, not only expected results, which is only useful if the tests are mostly deterministic (see #7748). It adds reporting of cache misses, which can be helpful
in getting tests to stay deterministic, and expiration since the test results are going to change more often than the expected results.

jkseppan · 2017-01-08T08:31:23Z

On my test host (a single core linode) merging #7748 into this reduced the test time on a warm cache from 349 to 272 seconds.

Before:

Image conversion cache hit rate: 1477/2102
----------------------------------------------------------------------
Ran 5315 tests in 349.495s

After:

Image conversion cache hit rate: 2094/2102
----------------------------------------------------------------------
Ran 5315 tests in 271.976s

QuLogic · 2017-01-08T08:42:32Z

Since that's a new file, just don't list it in default_test_modules, then you don't have to worry about the optimized case because it'll only be run through pytest.

jkseppan · 2017-01-08T08:49:14Z

Oh, is there a decision to deprecate testing with nose? I'm probably not on top of all the conversations now that most of them happen on github tickets instead of the devel mailing list.

QuLogic · 2017-01-08T09:15:59Z

There's ongoing work in #6731; presumably will ramp up after 2.0 is out.

jkseppan · 2017-01-08T21:32:04Z

I removed the file from default_test_modules and went back to using bare assert. Rebased to avoid useless back-and-forth commits in the history.

tacaswell · 2017-01-08T21:33:53Z

Yes, nose has declared its self un-maintained.

codecov-io · 2017-01-08T23:16:26Z

Current coverage is 63.82% (diff: 100%)

Merging #7764 into master will increase coverage by 1.70%

@@             master      #7764   diff @@
==========================================
  Files           174        177      +3   
  Lines         56028      63194   +7166   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
+ Hits          34805      40336   +5531   
- Misses        21223      22858   +1635   
  Partials          0          0

Powered by Codecov. Last update 115bade...6c9146e

jkseppan · 2017-01-21T17:45:45Z

This doesn't play well with pytest-xdist. At least the cache statistics don't get relayed from the slave processes, and I suspect the cache object might not get created either.

jkseppan · 2017-01-22T10:43:32Z

Works with pytest-xdist in my experiments... but now it looks like nose might not be picking up the plugin. Perhaps not worth investigating if we're dropping nose support soon?

anntzer · 2017-01-23T21:52:13Z

FWIW Inkscape also provides a --shell mode that may be useful, if starting/stopping the process turns out to contribute significantly to the total time (which the man page suggests):

       --shell With this parameter, Inkscape will enter an interactive command line shell
               mode. In this mode, you type in commands at the prompt and Inkscape executes
               them, without you having to run a new copy of Inkscape for each command. This
               feature is mostly useful for scripting and server uses: it adds no new
               capabilities but allows you to improve the speed and memory requirements of
               any script that repeatedly calls Inkscape to perform command line tasks (such
               as export or conversions). Each command in shell mode must be a complete valid
               Inkscape command line but without the Inkscape program name, for example
               "file.svg --export-pdf=file.pdf".

phobson

This looks good. I'm 99.9% sure this won't affect and downstream libraries (IOW, pytest-mpl). But it would be nice to get @astrofrog 's eyes on this as well.

jkseppan · 2017-01-24T07:16:48Z

The Inkscape shell mode would probably improve performance. With Ghostscript you can probably do something similar. I wonder: is it possible with pytest to run part of the test asynchronously, so it would wait on the result from Inkscape while starting the next test case?

NelleV

I haven't finish the review (this patch is long, so several passes over it are in order).

I have two major comments.

We don't depend on nose for the testing anymore. I am not sure how you write a plugin for pytest. It's probably just a question of removing the import and minor fixes.
This PR adds many new modules and classes to our public API. I think we need to keep this internal, in order to be able to refactor this if need be, without having to keep the API.

I have a bunch of minor comments.

NelleV · 2017-01-25T17:58:13Z

lib/matplotlib/testing/compare.py

@@ -76,40 +70,6 @@ def compare_float(expected, actual, relTol=None, absTol=None):
    return msg or None


-def get_cache_dir():


This is a backward incompatible change. While I am happy removing it, we need to deprecate it.

Done in 05b5fb5.

NelleV · 2017-01-25T18:00:37Z

lib/matplotlib/testing/conversion_cache.py

@@ -0,0 +1,200 @@
+"""


I think we need to make this module private, as well as all the classes in it. We shouldn't expose this API (at least just yet) to users.

Done in 9ba81dd, bb22d11.

NelleV · 2017-01-25T18:01:32Z

lib/matplotlib/testing/conversion_cache.py

+from matplotlib import checkdep_inkscape
+
+
+class ConversionCache(object):


Let's make this private as well :)

Done in bb22d11.

NelleV · 2017-01-25T18:02:31Z

lib/matplotlib/testing/conversion_cache.py

+    Parameters
+    ----------
+    directory : str, optional
+        Files are stored in this directory, defaults to `'test_cache'` in


I think it helps readability to replace the comma with a dot.

Done in a8fcb75.

NelleV · 2017-01-25T18:03:02Z

lib/matplotlib/testing/conversion_cache.py

+    directory : str, optional
+        Files are stored in this directory, defaults to `'test_cache'` in
+        the overall Matplotlib cache directory.
+    max_size : int, optional


👍 this is super useful

NelleV · 2017-01-25T18:20:04Z

lib/matplotlib/testing/conversion_cache.py

+            self.cachedir = self.get_cache_dir()
+        self.ensure_cache_dir()
+        if not isinstance(max_size, int):
+            raise ValueError("max_size is %s, expected int" % type(max_size))


"max_size is of type %s"?

Done in a8fcb75.

NelleV · 2017-01-25T18:21:54Z

lib/matplotlib/testing/conversion_cache.py

+    """
+
+    def __init__(self, directory=None, max_size=int(1e8)):
+        self.gets = set()


I find gets and hits quite obscure argument names. If the class is made private, we can easily fix this later.

Now it's private so the names are easier to change later, but I wonder if you have a proposal for better names? I think "cache hit" is a common term and "get" has a nice symmetry with "hit".

NelleV · 2017-01-25T18:24:47Z

lib/matplotlib/testing/conversion_cache.py

+        if version_tag:
+            md5.update(version_tag)
+        else:
+            warnings.warn(("Don't know the external converter for %s, cannot "


As a user, if I see this message, I assume the "solution" is to delete the cache directory?

Added some more explanation in a8fcb75.

NelleV · 2017-01-25T18:25:21Z

lib/matplotlib/testing/conversion_cache.py

+            `r['gets']` is the set of files queried,
+            `r['hits']` is the set of files found in the cache
+        """
+        return dict(hits=self.hits, gets=self.gets)


👍 this is going to be super useful as well.

NelleV · 2017-01-25T18:27:47Z

lib/matplotlib/testing/nose/plugins/conversion_cache.py

+                        unicode_literals)
+
+from pprint import pprint
+from nose.plugins import Plugin


We don't depend on nose anymore. I am not sure how you convert this to a pytest compatible plugin.

jkseppan · 2017-01-25T19:48:28Z

Thanks for the review @NelleV! This already works with pytest, all the required code is in conftest.py. I made a plugin for nose because there was already some nose customization via plugins, and added hooks in conftest.py because that was how the existing pytest customization was done. In fact I think there is some problem in the nose plugin because it outputs strange coverage results, but we can just remove it if nose is otherwise gone.

NelleV · 2017-01-25T19:51:23Z

I am in favor of removing the nose plugin :)

anntzer · 2017-01-25T20:07:06Z

lib/matplotlib/testing/nose/plugins/conversion_cache.py

+from ... import conversion_cache as ccache
+
+
+class ConversionCache(Plugin):


Looks like you have two classes both named ConversionCache. While technically OK I guess it'd be more legible to rename e.g. this one as ConversionCachePlugin (or similar).

(Missed the discussion about removing the nose plugin, which would make this comment obsolete :-))

This nose plugin class is getting deleted anyway, per the other comments.

anntzer · 2017-01-25T20:13:55Z

@jkseppan re: asynchronous tests: https://pypi.python.org/pypi/pytest-asyncio? (I guess it's time for me to finally understand how asyncio works. But it looks like only the conversion and comparison stage would need to be adapted to it (not sure).)

anntzer · 2017-01-25T22:04:12Z

lib/matplotlib/testing/_conversion_cache.py

+        self.gets.add(filename)
+        hash_value = self._get_file_hash(filename)
+        cached_file = os.path.join(self.cachedir, hash_value + self.cached_ext)
+        with cbook.Locked(self.cachedir):


Probably not a problem right now but it may be preferable to at some point use (and thus, first, implement...) per-file locking rather than per directory; see #7776 (comment).

I tried to hold the locks as briefly as possible, so for example the hashes get computed without holding the lock and only the shutil.copyfile call happens with the lock held. The expire method is the exception, but that one really needs to lock the whole directory to guarantee its file-size computations are accurate. The TeX cache, which #7776 is about, stays locked for the duration of a whole LaTeX run, which can easily take seconds.

anntzer · 2017-01-25T22:14:26Z

lib/matplotlib/testing/_conversion_cache.py

+        cached_file = os.path.join(self.cachedir, hash_value + self.cached_ext)
+        with cbook.Locked(self.cachedir):
+            if os.path.exists(cached_file):
+                shutil.copyfile(cached_file, newname)


Should we {sym,hard}link instead of copying?
(On Windows, admin rights are needed to symlink but not to hardlink. Python only added Windows support for either in Py3.2 (https://docs.python.org/3.6/library/os.html#os.link, https://docs.python.org/3.6/library/os.html#os.symlink).

That would probably be faster and save a little disk space, but it would introduce the possibility to change the cached file later by modifying the linked file, which could lead to hard-to-find bugs. It's unlikely that the testing code would do this on purpose, but perhaps some day someone will write a recursive directory-deletion function that truncates the files first. (I have written such a function myself elsewhere because of... a long story.)

NelleV

Just some small remarks.

also, do you mind rebasing with master to get the new appveyor stuff in? It's totally optional :)

NelleV · 2017-01-26T23:13:04Z

lib/matplotlib/testing/compare.py

@@ -160,16 +121,39 @@ def comparable_formats():
    return ['png'] + list(converter)


-def convert(filename, cache):
+@cbook.deprecated('2.1', addendum='Use _ConversionCache instead')


Should we advocate for the use of a private class/function or just inform the user of its disappearance?

Ok, removed the addenda.

NelleV · 2017-01-26T23:14:33Z

lib/matplotlib/testing/_conversion_cache.py

+
+    """
+
+    def __init__(self, directory=None, max_size=int(1e8)):


Maybe document self.hits and self.gets here?
(I don't have better names yet, thought I haven't really thought about it extensively)

Added some comments.

QuLogic

nose will need to be removed.

QuLogic · 2017-01-30T07:01:35Z

lib/matplotlib/tests/test_cache.py

+
+
+def test_cache_basic():
+    tmpdir = tempfile.mkdtemp()


Use tmpdir as an argument to the function to use the pytest fixture. Note that it's a py.path.local object, not a string or path.

Done. It does look like a useful object.

QuLogic · 2017-01-30T07:01:56Z

lib/matplotlib/tests/test_cache.py

+
+
+def test_cache_expire():
+    tmpdir = tempfile.mkdtemp()


Use tmpdir as a parameter to use the pytest fixture.

QuLogic · 2017-01-30T07:09:02Z

lib/matplotlib/tests/test_cache.py

+@mock.patch('matplotlib.testing._conversion_cache.cbook.mkdirs',
+            side_effect=OSError)
+def test_cache_mkdir_error(mkdirs):
+    tmpdir = tempfile.mkdtemp()


Use tmpdir as a parameter to use the pytest fixture.

QuLogic · 2017-01-30T07:09:20Z

lib/matplotlib/tests/test_cache.py

+@mock.patch('matplotlib.testing._conversion_cache.os.access',
+            side_effect=[False])
+def test_cache_unwritable_error(access):
+    tmpdir = tempfile.mkdtemp()


Use tmpdir as a parameter to use the pytest fixture.

QuLogic · 2017-01-30T07:09:51Z

lib/matplotlib/tests/test_cache.py

+import stat
+import tempfile
+
+from nose.tools import raises


Remove nose.

QuLogic · 2017-01-30T07:13:26Z

lib/matplotlib/tests/test_cache.py

+            assert copy.read() == 'generated from the pdf file'
+        assert cache.report() == \
+            {'gets': {intmp('fake.pdf'), intmp('fake.svg')},
+             'hits': set([intmp('fake.pdf')])}


How come this isn't a set literal like the other ones?

Fixed. I had probably copied this from above and edited it from the empty set.

QuLogic · 2017-01-30T07:15:00Z

lib/matplotlib/tests/test_cache.py

+        pass
+
+
+@raises(_CacheError)


Use pytest.raises context manager instead of this decorator.

QuLogic · 2017-01-30T07:15:05Z

lib/matplotlib/tests/test_cache.py

+        shutil.rmtree(tmpdir)
+
+
+@raises(_CacheError)


Use pytest.raises context manager instead of this decorator.

QuLogic · 2017-01-30T07:57:21Z

lib/matplotlib/testing/_conversion_cache.py

+        try:
+            self.converter_version['.pdf'] = \
+                checkdep_ghostscript()[1].encode('utf-8')
+        except:


Bare excepts are dangerous. What is this actually trying to catch?

QuLogic · 2017-01-30T07:57:42Z

lib/matplotlib/testing/_conversion_cache.py

+        try:
+            self.converter_version['.svg'] = \
+                checkdep_inkscape().encode('utf-8')
+        except:


This way py.test lib/matplotlib/tests/test_artist.py --conversion-cache-report-misses -n 2 works for me even though the options don't get documented in "py.test --help".

jkseppan · 2017-02-20T13:02:47Z

I guess it's progress that now it fails on Travis too, so both CI engines are getting the same configuration. The remaining mystery is why it fails there while working for me locally.

tacaswell · 2017-02-20T22:05:32Z

differences in pytest version?

QuLogic · 2017-02-20T22:33:06Z

Do you run just pytest or pytest path/to/something? The former is what CI does, and it doesn't work for me if I do that. If I do pytest lib/matplotlib, then it also fails because pytest_addoption is called twice from the regular tests and the sphinx tests. But If I do pytest lib/matplotlib/tests (or any of the other two tests paths), then it works.

QuLogic · 2017-02-20T22:22:56Z

lib/matplotlib/tests/test_cache.py

+        assert not os.path.exists(fname('cache', 'onemore.png'))
+
+    finally:
+        tmpdir.remove(rec=1)


Why do this instead of letting the fixture clean up?

I didn't see in the documentation that it promises to do any cleanup. But it seems that the base directory gets cleaned up after three test runs.

QuLogic · 2017-02-21T03:18:42Z

Do you think it would be possible/helpful to use pytest's builtin cache instead?

jkseppan · 2017-02-21T19:41:12Z

@QuLogic You're right, I've been running pytest path/to/file to avoid running the whole test suite when testing, and it fails when I omit the path. Apparently the conftest.py files in subdirectories are only picked up correctly if there is a path. Is there a reason that the top-level configuration file was removed?

jkseppan · 2017-02-21T19:46:55Z

@QuLogic I don't see much savings that are possible by using the pytest cache, unless it manages LRU deletion for us, and I don't see that mentioned. The get/set methods serialize the values using JSON, which probably increases the size of the data by escaping it inside a string. The makedir method seems to just offer a way to make a directory, which we can do without the API.

It seems that the tmpdir fixture will take care of them after three test runs. http://doc.pytest.org/en/latest/tmpdir.html#the-default-base-temporary-directory

jkseppan · 2017-02-22T08:51:43Z

It seems pytest_addoption only works in top-level conftest.py files or plugins: pytest-dev/pytest#1427

It only works there, see pytest-dev/pytest#1427

jkseppan · 2017-02-22T10:15:25Z

When the pytest_addoption call is at the top level, Travis tests seem to be passing but Appveyor fails with "E ValueError: Plugin already registered: C:\projects\matplotlib\conftest.py=<module 'conftest' from 'C:\projects\matplotlib\conftest.py'>"

QuLogic · 2017-02-22T21:49:59Z

Yes, the top-level conftest.py was removed because a) we needed fixtures to work on the installed version and b) AppVeyor didn't like having both... For the installed case, you can add a default to getting the option so it works without the option enabled, but I still have no idea how to get the top-level to work right.

tacaswell · 2017-08-29T01:27:39Z

This has grown merge conflicts.

jklymak · 2020-09-12T19:52:25Z

marked as stale. Assume this is obsolete and should closed?

jkseppan · 2020-09-13T11:40:49Z

I suppose? I don't really remember the details of this PR.

QuLogic · 2020-09-25T03:58:45Z

@anntzer replaced this in #15932 (though more of a rewrite in spirit than a rebase)

jkseppan added the topic: testing label Jan 7, 2017

tacaswell modified the milestones: 2.2 (next next feature release), 2.1 (next point release) Jan 7, 2017

tacaswell requested a review from Kojoley January 7, 2017 22:16

jkseppan force-pushed the test-caching branch from 871a62a to 6c9146e Compare January 8, 2017 21:30

phobson approved these changes Jan 24, 2017

View reviewed changes

NelleV previously requested changes Jan 25, 2017

View reviewed changes

anntzer reviewed Jan 25, 2017

View reviewed changes

astrofrog mentioned this pull request Jan 26, 2017

Check whether any updates are needed following Matplotlib changes matplotlib/pytest-mpl#41

Closed

NelleV reviewed Jan 29, 2017

View reviewed changes

NelleV changed the title ~~MAINT: Refactor the converted-image cache~~ [MRG+1] MAINT: Refactor the converted-image cache Jan 29, 2017

QuLogic requested changes Jan 30, 2017

View reviewed changes

jkseppan force-pushed the test-caching branch from a36436d to 936f73b Compare January 30, 2017 16:11

jkseppan added 2 commits February 20, 2017 14:44

There is no nose plugin any more

6007b37

Import the pytest hooks in the other conftest.py files

e52cdd2

This way py.test lib/matplotlib/tests/test_artist.py --conversion-cache-report-misses -n 2 works for me even though the options don't get documented in "py.test --help".

jkseppan force-pushed the test-caching branch from bb9a1a3 to e52cdd2 Compare February 20, 2017 12:47

QuLogic reviewed Feb 21, 2017

View reviewed changes

Omit unnecessary tmpdir deletions

e5184bb

It seems that the tmpdir fixture will take care of them after three test runs. http://doc.pytest.org/en/latest/tmpdir.html#the-default-base-temporary-directory

jkseppan added 2 commits February 22, 2017 11:40

Move pytest_addoption call to top-level conftest.py

f47cbe4

It only works there, see pytest-dev/pytest#1427

pep8 fix

996f22b

tacaswell modified the milestones: 2.1.1 (next bug fix release), 2.1 (next point release) Aug 29, 2017

tacaswell modified the milestones: 2.1.1 (next bug fix release), 2.2 (next feature release) Oct 9, 2017

anntzer mentioned this pull request Dec 13, 2019

Use test cache for test result images too. #15932

Merged

6 tasks

jklymak added the stale label Sep 12, 2020

jkseppan closed this Sep 13, 2020

jkseppan deleted the test-caching branch September 13, 2020 11:41

QuLogic removed the stale label Sep 25, 2020

story645 removed this from the future releases milestone Oct 6, 2022

		@@ -76,40 +70,6 @@ def compare_float(expected, actual, relTol=None, absTol=None):
		return msg or None


		def get_cache_dir():

		from matplotlib import checkdep_inkscape


		class ConversionCache(object):

		from ... import conversion_cache as ccache


		class ConversionCache(Plugin):

		pass


		@raises(_CacheError)

		shutil.rmtree(tmpdir)


		@raises(_CacheError)

[MRG+2] MAINT: Refactor the converted-image cache #7764

[MRG+2] MAINT: Refactor the converted-image cache #7764

Conversation

jkseppan commented Jan 7, 2017

jkseppan commented Jan 8, 2017

QuLogic commented Jan 8, 2017

jkseppan commented Jan 8, 2017

QuLogic commented Jan 8, 2017

jkseppan commented Jan 8, 2017

tacaswell commented Jan 8, 2017

codecov-io commented Jan 8, 2017 • edited Loading

Current coverage is 63.82% (diff: 100%)

jkseppan commented Jan 21, 2017

jkseppan commented Jan 22, 2017

anntzer commented Jan 23, 2017

phobson left a comment

Choose a reason for hiding this comment

jkseppan commented Jan 24, 2017 via email

NelleV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkseppan commented Jan 25, 2017

NelleV commented Jan 25, 2017

anntzer Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelleV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkseppan commented Feb 20, 2017

tacaswell commented Feb 20, 2017

QuLogic commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic commented Feb 21, 2017

jkseppan commented Feb 21, 2017

jkseppan commented Feb 21, 2017

jkseppan commented Feb 22, 2017

jkseppan commented Feb 22, 2017

codecov-io commented Jan 8, 2017 •

edited

Loading

anntzer Jan 25, 2017 •

edited

Loading

jkseppan commented Sep 13, 2020 •

edited

Loading