Skip to content

Use test cache for test result images too. #15932

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

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Dec 13, 2019

For image comparison tests in svg/pdf/ps formats, the result images are
converted to png for comparison. Previously the conversion results were
cached for the baseline images, but not for the test-generated images
(because of non-deterministic svg/pdf/etc. results, due to
hash-salting, dict ordering, etc.).

Now that the test-generated images are generally deterministic, we can
enable the cache for baseline images as well. This speeds up
pytest -k '[svg]' by ~30% (81s initially -> 55s on a seeded cache) and
pytest -k '[pdf]' by ~10% (62s -> 55s) (there are too few (e)ps image
comparison tests to see an effect). Also add logging regarding the
cache which may help troubleshooting determinacy problems.

This is a much simpler version of PR#7764, which added more sophisticated
reporting of cache hits and misses, as well as cache invalidation (I
think the cache can reasonably be cleaned manually, at least for now).

A simple cache eviction mechanism prevents the cache from growing
without bounds, limiting it to 2x the size of the baseline_images
directory.

This is a much simpler version of PR7764, which added more sophisticated
reporting of cache hits and misses and cache eviction.

Supersedes (mostly) #7764 (@jkseppan).

(Note that the gain in speed is less than reported than in #7764 (comment), because I have also optimized the svg and gs converters in the meantime by running a single process and interacting with it over stdin/stdout, rather that launching a new instance of inkscape/ghostscript for each image.)

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell
Copy link
Member

If we have a few tests that are not deterministic, will this cause the cache to increase without bound?

I think that we cache these folders on at least travis so we would want to make sure we don't start spending more time moving around invalid images than we save by not computing them.

@anntzer
Copy link
Contributor Author

anntzer commented Dec 13, 2019

Fair enough, added a simple (automatic) cache eviction mechanism.

@jklymak jklymak marked this pull request as draft September 24, 2020 15:57
@jklymak
Copy link
Member

jklymak commented Sep 24, 2020

I don't fully follow this, but it seems useful. But needs a rebase, and probably a review from @tacaswell or @QuLogic

For image comparison tests in svg/pdf/ps formats, the result images are
converted to png for comparison.  Previously the conversion results were
cached for the baseline images, but not for the test-generated images
(because of non-deterministic svg/pdf/etc. results, due to
hash-salting, dict ordering, etc.).

Now that the test-generated images are generally deterministic, we can
enable the cache for baseline images as well.  This speeds up
`pytest -k '[svg]'` by ~30% (81s initially -> 55s on a seeded cache) and
`pytest -k '[pdf]'` by ~10% (62s -> 55s) (there are too few (e)ps image
comparison tests to see an effect).  Also add logging regarding the
cache which may help troubleshooting determinacy problems.

A simple cache eviction mechanism prevents the cache from growing
without bounds, limiting it to 2x the size of the baseline_images
directory.

This is a much simpler version of PR7764, which added more sophisticated
reporting of cache hits and misses and cache eviction.
@anntzer
Copy link
Contributor Author

anntzer commented Sep 24, 2020

rebased

@anntzer anntzer marked this pull request as ready for review September 24, 2020 18:16
@tacaswell tacaswell merged commit 64950dd into matplotlib:master Sep 25, 2020
@tacaswell tacaswell added this to the v3.4.0 milestone Sep 25, 2020
@anntzer anntzer deleted the testcache branch September 25, 2020 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants