From 8d0b07b65f5886d788616784f4defee58ec11ad5 Mon Sep 17 00:00:00 2001 From: Antony Lee Date: Sat, 2 Mar 2019 18:03:31 +0100 Subject: [PATCH] Deprecate MovieWriterRegistry cache-dirtyness system. MovieWriterRegistry has a fairly extensive system to cache whether movie writer classes are available or not. Deprecate it and always perform the check every time: for nearly all classes, the availability check is either an import check (Pillow), or checking for whether an executable is in the $PATH -- this is negligible compared to actually running the executable to encode the movie. The only exception is that ffmpeg actually checks whether it is actually ffmpeg or ubuntu's broken libav. In order to avoid performing that check (... which should still be not so slow) when Pillow is available (Pillow is registered first, so comes first :)), add `MovieWriterRegistry.__iter__` which allows to get the first available writer without checking the availability of the subsequent ones (this behavior is consistent with `MovieWriterRegistry.__getitem__` and `MovieWriterRegistry.list`). --- doc/api/next_api_changes/2019-03-03-AL.rst | 14 ++++ lib/matplotlib/animation.py | 79 +++++++++++----------- lib/matplotlib/rcsetup.py | 7 +- lib/matplotlib/tests/test_animation.py | 33 +++------ 4 files changed, 66 insertions(+), 67 deletions(-) create mode 100644 doc/api/next_api_changes/2019-03-03-AL.rst diff --git a/doc/api/next_api_changes/2019-03-03-AL.rst b/doc/api/next_api_changes/2019-03-03-AL.rst new file mode 100644 index 000000000000..7be2d21fb808 --- /dev/null +++ b/doc/api/next_api_changes/2019-03-03-AL.rst @@ -0,0 +1,14 @@ +Deprecations +```````````` + +The following methods and attributes of the `MovieWriterRegistry` class are +deprecated: ``set_dirty``, ``ensure_not_dirty``, ``reset_available_writers``, +``avail``. + +The ``rcsetup.validate_animation_writer_path`` function is deprecated. + +`MovieWriterRegistry` now always checks the availability of the writer classes +before returning them. If one wishes, for example, to get the first available +writer, without performing the availability check on subsequent writers, it is +now possible to iterate over the registry, which will yield the names of the +available classes. diff --git a/lib/matplotlib/animation.py b/lib/matplotlib/animation.py index 1f3d8b2dee4d..a622827e05cf 100644 --- a/lib/matplotlib/animation.py +++ b/lib/matplotlib/animation.py @@ -98,13 +98,11 @@ def correct_roundoff(x, dpi, n): class MovieWriterRegistry(object): '''Registry of available writer classes by human readable name.''' def __init__(self): - self.avail = dict() self._registered = dict() - self._dirty = False + @cbook.deprecated("3.2") def set_dirty(self): """Sets a flag to re-setup the writers.""" - self._dirty = True def register(self, name): """Decorator for registering a class under a name. @@ -115,32 +113,27 @@ def register(self, name): class Foo: pass """ - def wrapper(writerClass): - self._registered[name] = writerClass - if writerClass.isAvailable(): - self.avail[name] = writerClass - return writerClass + def wrapper(writer_cls): + self._registered[name] = writer_cls + return writer_cls return wrapper + @cbook.deprecated("3.2") def ensure_not_dirty(self): """If dirty, reasks the writers if they are available""" - if self._dirty: - self.reset_available_writers() + @cbook.deprecated("3.2") def reset_available_writers(self): """Reset the available state of all registered writers""" - self.avail = {name: writerClass - for name, writerClass in self._registered.items() - if writerClass.isAvailable()} - self._dirty = False - def list(self): - '''Get a list of available MovieWriters.''' - self.ensure_not_dirty() - return list(self.avail) + @cbook.deprecated("3.2") + @property + def avail(self): + return {name: self._registered[name] for name in self.list()} def is_available(self, name): - '''Check if given writer is available by name. + """ + Check if given writer is available by name. Parameters ---------- @@ -149,19 +142,28 @@ def is_available(self, name): Returns ------- available : bool - ''' - self.ensure_not_dirty() - return name in self.avail - - def __getitem__(self, name): - self.ensure_not_dirty() - if not self.avail: - raise RuntimeError("No MovieWriters available!") + """ try: - return self.avail[name] + cls = self._registered[name] except KeyError: - raise RuntimeError( - 'Requested MovieWriter ({}) not available'.format(name)) + return False + return cls.isAvailable() + + def __iter__(self): + """Iterate over names of available writer class.""" + for name in self._registered: + if self.is_available(name): + yield name + + def list(self): + """Get a list of available MovieWriters.""" + return [*self] + + def __getitem__(self, name): + """Get an available writer class from its name.""" + if self.is_available(name): + return self._registered[name] + raise RuntimeError(f"Requested MovieWriter ({name}) not available") writers = MovieWriterRegistry() @@ -1071,22 +1073,21 @@ class to use, such as 'ffmpeg'. If ``None``, defaults to # If we have the name of a writer, instantiate an instance of the # registered class. if isinstance(writer, str): - if writer in writers.avail: + if writers.is_available(writer): writer = writers[writer](fps, codec, bitrate, extra_args=extra_args, metadata=metadata) else: - if writers.list(): - alt_writer = writers[writers.list()[0]] - _log.warning("MovieWriter %s unavailable; trying to use " - "%s instead.", writer, alt_writer) - writer = alt_writer( - fps, codec, bitrate, - extra_args=extra_args, metadata=metadata) - else: + alt_writer = next(writers, None) + if alt_writer is None: raise ValueError("Cannot save animation: no writers are " "available. Please install ffmpeg to " "save animations.") + _log.warning("MovieWriter %s unavailable; trying to use %s " + "instead.", writer, alt_writer) + writer = alt_writer( + fps, codec, bitrate, + extra_args=extra_args, metadata=metadata) _log.info('Animation.save using %s', type(writer)) if 'bbox_inches' in savefig_kwargs: diff --git a/lib/matplotlib/rcsetup.py b/lib/matplotlib/rcsetup.py index dd1f08120038..580fd2140bc1 100644 --- a/lib/matplotlib/rcsetup.py +++ b/lib/matplotlib/rcsetup.py @@ -927,6 +927,7 @@ def validate_hist_bins(s): " a sequence of floats".format(valid_strs)) +@cbook.deprecated("3.2") def validate_animation_writer_path(p): # Make sure it's a string and then figure out if the animations # are already loaded and reset the writers (which will validate @@ -1454,15 +1455,15 @@ def _validate_linestyle(ls): # Additional arguments for HTML writer 'animation.html_args': [[], validate_stringlist], # Path to ffmpeg binary. If just binary name, subprocess uses $PATH. - 'animation.ffmpeg_path': ['ffmpeg', validate_animation_writer_path], + 'animation.ffmpeg_path': ['ffmpeg', validate_string], # Additional arguments for ffmpeg movie writer (using pipes) 'animation.ffmpeg_args': [[], validate_stringlist], # Path to AVConv binary. If just binary name, subprocess uses $PATH. - 'animation.avconv_path': ['avconv', validate_animation_writer_path], + 'animation.avconv_path': ['avconv', validate_string], # Additional arguments for avconv movie writer (using pipes) 'animation.avconv_args': [[], validate_stringlist], # Path to convert binary. If just binary name, subprocess uses $PATH. - 'animation.convert_path': ['convert', validate_animation_writer_path], + 'animation.convert_path': ['convert', validate_string], # Additional arguments for convert movie writer (using pipes) 'animation.convert_args': [[], validate_stringlist], diff --git a/lib/matplotlib/tests/test_animation.py b/lib/matplotlib/tests/test_animation.py index d587271d5a91..67cdc42f9dcd 100644 --- a/lib/matplotlib/tests/test_animation.py +++ b/lib/matplotlib/tests/test_animation.py @@ -187,27 +187,14 @@ def test_no_length_frames(): def test_movie_writer_registry(): - ffmpeg_path = mpl.rcParams['animation.ffmpeg_path'] - # Not sure about the first state as there could be some writer - # which set rcparams - # assert not animation.writers._dirty assert len(animation.writers._registered) > 0 - animation.writers.list() # resets dirty state - assert not animation.writers._dirty mpl.rcParams['animation.ffmpeg_path'] = "not_available_ever_xxxx" - assert animation.writers._dirty - animation.writers.list() # resets - assert not animation.writers._dirty assert not animation.writers.is_available("ffmpeg") # something which is guaranteed to be available in path # and exits immediately bin = "true" if sys.platform != 'win32' else "where" mpl.rcParams['animation.ffmpeg_path'] = bin - assert animation.writers._dirty - animation.writers.list() # resets - assert not animation.writers._dirty assert animation.writers.is_available("ffmpeg") - mpl.rcParams['animation.ffmpeg_path'] = ffmpeg_path @pytest.mark.skipif( @@ -248,18 +235,14 @@ def test_failing_ffmpeg(tmpdir, monkeypatch): succeeds when called with no arguments (so that it gets registered by `isAvailable`), but fails otherwise, and add it to the $PATH. """ - try: - with tmpdir.as_cwd(): - monkeypatch.setenv("PATH", ".:" + os.environ["PATH"]) - exe_path = Path(str(tmpdir), "ffmpeg") - exe_path.write_text("#!/bin/sh\n" - "[[ $@ -eq 0 ]]\n") - os.chmod(str(exe_path), 0o755) - animation.writers.reset_available_writers() - with pytest.raises(subprocess.CalledProcessError): - make_animation().save("test.mpeg") - finally: - animation.writers.reset_available_writers() + with tmpdir.as_cwd(): + monkeypatch.setenv("PATH", ".:" + os.environ["PATH"]) + exe_path = Path(str(tmpdir), "ffmpeg") + exe_path.write_text("#!/bin/sh\n" + "[[ $@ -eq 0 ]]\n") + os.chmod(str(exe_path), 0o755) + with pytest.raises(subprocess.CalledProcessError): + make_animation().save("test.mpeg") @pytest.mark.parametrize("cache_frame_data, weakref_assertion_fn", [