MAINT: Refactor the converted-image cache #7764

Open
wants to merge 4 commits into
from

Projects

Needs Review in Migrate to `pytest`

5 participants

@jkseppan
Member
jkseppan commented Jan 7, 2017

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 jkseppan added the Testing label Jan 7, 2017
@tacaswell tacaswell requested a review from Kojoley Jan 7, 2017
@jkseppan
Member
jkseppan commented Jan 8, 2017

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
Member
QuLogic commented Jan 8, 2017

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
Member
jkseppan commented Jan 8, 2017

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
Member
QuLogic commented Jan 8, 2017

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

@jkseppan jkseppan Factor the converted-image cache out of compare.py
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.
6c9146e
@jkseppan
Member
jkseppan commented Jan 8, 2017

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
Member

Yes, nose has declared its self un-maintained.

@codecov-io
codecov-io commented Jan 8, 2017 edited

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
Member

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 added some commits Jan 21, 2017
@jkseppan jkseppan Import cbook again
Otherwise this breaks after merging to master
ae1370f
@jkseppan jkseppan Lock the cache directory
In case multiple processes use it at the same time.
Don't delete the lock file in expire().
1f75e69
@jkseppan jkseppan Communicate node-specific cache reports to master if using xdist
3fd65fc
@jkseppan
Member

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
Contributor
anntzer commented Jan 23, 2017

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".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment