8000 Don't crash when trying to recover from animation failures. · matplotlib/matplotlib@258c53b · GitHub
[go: up one dir, main page]
Skip to content

Commit 258c53b

Browse files
anntzeranntzer
committed
Don't crash when trying to recover from animation failures.
Previously, if an animation-generating subprocess (e.g. ffmpeg) failed while data was piped in, we'd get a ValueError because we'd communicate() with the subprocess once in grab_frame(), to generate a first error, and again at cleanup time, in the finally: clause, but that second time, the subprocess' stdout and stderr had already been closed. Instead, don't do anything in grab_frame() and let the finally: clause handle the errors. Also replace the nondescript RuntimeError by a CalledProcessError, which is more descriptive, generates a proper error message by itself ("Command 'foo' returned non-zero exit status 42") and is more easily introspectable (it includes stdout, stderr as attributes). Given that we would previously sometimes throw a ValueError instead of the intended RuntimeError, I think the API change is worth it. Un-xfail a test.
1 parent 46ba49a commit 258c53b

File tree

3 files changed

+32
-41
lines changed

3 files changed

+32
-41
lines changed

‎doc/api/next_api_changes/2018-10-02-AL-animation.rst

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
Exception on failing animations changed
2+
```````````````````````````````````````
3+
4+
Previously, subprocess failures in the animation framework would raise either
5+
in a `RuntimeError` or a `ValueError` depending on when the error occurred.
6+
They now raise a `subprocess.CalledProcessError` with attributes set as
7+
documented by the exception class.

‎lib/matplotlib/animation.py

Lines changed: 21 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -364,22 +364,14 @@ def grab_frame(self, **savefig_kwargs):
364364
command that saves the figure.
365365
'''
366366
_log.debug('MovieWriter.grab_frame: Grabbing frame.')
367-
try:
368-
# re-adjust the figure size in case it has been changed by the
369-
# user. We must ensure that every frame is the same size or
370-
# the movie will not save correctly.
371-
self.fig.set_size_inches(self._w, self._h)
372-
# Tell the figure to save its data to the sink, using the
373-
# frame format and dpi.
374-
self.fig.savefig(self._frame_sink(), format=self.frame_format,
375-
dpi=self.dpi, **savefig_kwargs)
376-
except (RuntimeError, IOError) as e:
377-
out, err = self._proc.communicate()
378-
_log.info('MovieWriter -- Error running proc:\n%s\n%s', out, err)
379-
raise IOError('Error saving animation to file (cause: {0}) '
380-
'Stdout: {1} StdError: {2}. It may help to re-run '
381-
'with logging level set to '
382-
'DEBUG.'.format(e, out, err))
367+
# re-adjust the figure size in case it has been changed by the
368+
# user. We must ensure that every frame is the same size or
369+
# the movie will not save correctly.
370+
self.fig.set_size_inches(self._w, self._h)
371+
# Tell the figure to save its data to the sink, using the
372+
# frame format and dpi.
373+
self.fig.savefig(self._frame_sink(), format=self.frame_format,
374+
dpi=self.dpi, **savefig_kwargs)
383375

384376
def _frame_sink(self):
385377
'''Returns the place to which frames should be written.'''
@@ -396,15 +388,15 @@ def cleanup(self):
396388
# Use the encoding/errors that universal_newlines would use.
397389
out = TextIOWrapper(BytesIO(out)).read()
398390
err = TextIOWrapper(BytesIO(err)).read()
399-
_log.debug("MovieWriter stdout:\n%s", out)
400-
_log.debug("MovieWriter stderr:\n%s", err)
391+
_log.log(
392+
logging.WARNING if self._proc.returncode else logging.DEBUG,
393+
"MovieWriter stdout:\n%s", out)
394+
_log.log(
395+
logging.WARNING if self._proc.returncode else logging.DEBUG,
396+
"MovieWriter stderr:\n%s", err)
401397
if self._proc.returncode:
402-
raise RuntimeError('Error creating movie, return code {}\n'
403-
'stdout:\n'
404-
'{}\n'
405-
'stderr:\n'
406-
'{}\n'
407-
.format(self._proc.returncode, out, err))
398+
raise subprocess.CalledProcessError(
399+
self._proc.returncode, self._proc.args, out, err)
408400

409401
@classmethod
410402
def bin_path(cls):
@@ -511,17 +503,11 @@ def grab_frame(self, **savefig_kwargs):
511503
'''
512504
# Overloaded to explicitly close temp file.
513505
_log.debug('MovieWriter.grab_frame: Grabbing frame.')
514-
try:
515-
# Tell the figure to save its data to the sink, using the
516-
# frame format and dpi.
517-
with self._frame_sink() as myframesink:
518-
self.fig.savefig(myframesink, format=self.frame_format,
519-
dpi=self.dpi, **savefig_kwargs)
520-
521-
except RuntimeError:
522-
out, err = self._proc.communicate()
523-
_log.info('MovieWriter -- Error running proc:\n%s\n%s', out, err)
524-
raise
506+
# Tell the figure to save its data to the sink, using the
507+
# frame format and dpi.
508+
with self._frame_sink() as myframesink:
509+
self.fig.savefig(myframesink, format=self.frame_format,
510+
dpi=self.dpi, **savefig_kwargs)
525511

526512
def finish(self):
527513
# Call run here now that all frame grabbing is done. All temp files

‎lib/matplotlib/tests/test_animation.py

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import os
22
from pathlib import Path
3+
import subprocess
34
import sys
45

56
import numpy as np
@@ -237,13 +238,10 @@ def test_cleanup_temporaries(method_name, tmpdir):
237238
assert list(Path(str(tmpdir)).iterdir()) == []
238239

239240

240-
# Currently, this fails with a ValueError after we try to communicate() twice
241-
# with the Popen.
242-
@pytest.mark.xfail
243241
@pytest.mark.skipif(os.name != "posix", reason="requires a POSIX OS")
244242
def test_failing_ffmpeg(tmpdir, monkeypatch):
245243
"""
246-
Test that we correctly raise an OSError when ffmpeg fails.
244+
Test that we correctly raise a CalledProcessError when ffmpeg fails.
247245
248246
To do so, mock ffmpeg using a simple executable shell script that
249247
succeeds when called with no arguments (so that it gets registered by
@@ -252,12 +250,12 @@ def test_failing_ffmpeg(tmpdir, monkeypatch):
252250
try:
253251
with tmpdir.as_cwd():
254252
monkeypatch.setenv("PATH", ".:" + os.environ["PATH"])
255-
exe_path = Path(tmpdir, "ffmpeg")
253+
exe_path = Path(str(tmpdir), "ffmpeg")
256254
exe_path.write_text("#!/bin/sh\n"
257 5AF3 255
"[[ $@ -eq 0 ]]\n")
258256
os.chmod(str(exe_path), 0o755)
259257
animation.writers.reset_available_writers()
260-
with pytest.raises(OSError):
258+
with pytest.raises(subprocess.CalledProcessError):
261259
make_animation().save("test.mpeg")
262260
finally:
263261
animation.writers.reset_available_writers()

0 commit comments

Comments
 (0)
0