8000 Deprecate MovieWriterRegistry cache-dirtyness system. by anntzer · Pull Request #13567 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Deprecate MovieWriterRegistry cache-dirtyness system. #13567

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
Jul 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions doc/api/next_api_changes/2019-03-03-AL.rst
Original file line number Diff line number Diff line change
@@ -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.
79 changes: 40 additions & 39 deletions lib/matplotlib/animation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
----------
Expand All @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? Isn't reg.list() just the same as list(reg)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. This preserves API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I can deprecate it too if you don't mind the API change...

"""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()
Expand Down Expand Up @@ -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:
Expand Down
7 changes: 4 additions & 3 deletions lib/matplotlib/rcsetup.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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],

Expand Down
33 changes: 8 additions & 25 deletions lib/matplotlib/tests/test_animation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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", [
Expand Down
0