search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 471 forks source link

ffmpeg/imagemagick features need feature checks #33092

Closed orlitzky closed 2 years ago

orlitzky commented 2 years ago

We have optional tests that make use of these packages, for example:

sage: a.show(format="webm", iterations=1)  # optional -- ffmpeg
sage: with open(td + 'wave.gif', 'rb') as f: print(b'!\xf9\x04\x08\x14\x00' in f.read())  # optional -- ImageMagick

These tests are run whenever the corresponding "feature" is available, but the feature checks look only for the convert and ffmpeg programs. Both imagemagick and ffmpeg can be built without support for (say) webm files, making the tests above fail. To avoid spurious failures, the features should test for the necessary file format support, likely in the is_functional() method.

CC: @seblabbe

Component: packages: optional

Author: Sébastien Labbé

Branch: 1678c7f

Reviewer: Julian Rüth, Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/33092

mkoeppe commented 2 years ago
comment:1

I don't think we need to tighten spkg-configure.m4 -- this does not influence the ffmpeg optional tag at all.

These tags are added by runtime Feature checks, not via spkg-configure.m4

mkoeppe commented 2 years ago
comment:2

(this code is from #32174)

orlitzky commented 2 years ago

Description changed:

--- 
+++ 
@@ -8,6 +8,5 @@
 sage: with open(td + 'wave.gif', 'rb') as f: print(b'!\xf9\x04\x08\x14\x00' in f.read())  # optional -- ImageMagick

-These tests are now run whenever the corresponding system package is installed... but the system package check looks only for the convert and ffmpeg programs. Both imagemagick and ffmpeg can be built without support for (say) webm files, making the tests above fail. To avoid spurious failures, the ./configure check should test for any features that the test suite uses. +These tests are run whenever the corresponding "feature" is available, but the feature checks look only for the convert and ffmpeg programs. Both imagemagick and ffmpeg can be built without support for (say) webm files, making the tests above fail. To avoid spurious failures, the features should test for the necessary file format support, likely in the is_functional() method.

-(Really, I would prefer it if sage didn't try to enable optional packages that I haven't asked for, but that's another thing entirely.)

mkoeppe commented 2 years ago
comment:4

If these doctests are the only ways that we "need" these features, perhaps it would be enough to just mark them as "not tested"?

seblabbe commented 2 years ago
comment:5

There are like 79 optional imagemagick doctests and 19 optional ffmpeg doctests:

$ git grep "optional -* ImageMagick" | wc
     76     678    7649
$ git grep "optional -* imagemagick" | wc
      3      23     305
$ git grep "optional -* ffmpeg" | wc
     19     151    1895

We can mark two of them as "not tested" if they do not work with all versions of ffmpeg/imagemagick. But I still think it is a good thing to test that the remaining 79 + 19 - 2 doctests still work on all versions of ffmpeg.

My opinion is that either we expect doctests to work and we test them or we expect doctests not to work and we do not test them.

mkoeppe commented 2 years ago
comment:6

Replying to @seblabbe:

We can mark two of them as "not tested" if they do not work with all versions of ffmpeg/imagemagick. But I still think it is a good thing to test that the remaining 79 + 19 - 2 doctests still work on all versions of ffmpeg.

I agree, I meant these 2 failing ones only.

orlitzky commented 2 years ago
comment:7

Those two were only examples. If I turn off all the features that Gentoo lets me turn off on my desktop (some are required by other packages),

sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/combinat/crystals/mv_polytopes.py  # 1 doctest failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/combinat/tiling.py  # 8 doctests failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/combinat/words/paths.py  # 7 doctests failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/plot/animate.py  # 66 doctests failed
sage -t --long --warn-long 97.6 --random-seed=324387541623744523058195579638222709880 src/sage/plot/plot3d/tachyon.py  # 3 doctests failed
seblabbe commented 2 years ago
comment:8

I see. Can you provide the log of the failing doctests here or somewhere?

orlitzky commented 2 years ago

Attachment: ffmpeg-imagemagick.log

orlitzky commented 2 years ago
comment:9

Replying to @seblabbe:

I see. Can you provide the log of the failing doctests here or somewhere?

Sure, I just attached it.

seblabbe commented 2 years ago
comment:10

Thank you for the log.

I see the error message convert convert: No decode delegate for this image format (00000000.png). Can you provide what is the output of convert -list Format on this machine? I wonder if the output of such command could be used to check that the feature works in the feature code?

Also I see:

subprocess.CalledProcessError: Command 'cd "/home/mjo/.sage/temp/gantu/27410/dir_787qb00e/"; sage-native-execute convert -dispose Background -delay 35 -loop 3 *.png "/home/mjo/.sage/temp/gantu/27410/dir_m_q6dl3q/my_animation.gif"' returned non-zero exit status 1.

which does not say much about the error. I think the check_call(cmd, shell=True) should be replaced in the method _gif_from_imagemagick by something which returns more info when it fails. I did something like that in the ticket #20343 (see method def pdf). We could do the same here.

orlitzky commented 2 years ago
comment:11

It looks like most of the disabled formats are absent from the list, but SVG and SVGZ (also disabled) appear with mode ---:

$ convert -list Format
   Format L  Mode  Description
--------------------------------------------------------------------------------
      3FR S  r--  Hasselblad Photo RAW
     8BIM P  rw-  Photoshop resource format
 8BIMTEXT P  rw-  Photoshop resource text format
8BIMWTEXT P  rw-  Photoshop resource wide text format
     APP1 P  rw-  Raw application information
 APP1JPEG P  rw-  Raw JPEG binary data
      ART S  rw-  PFS: 1st Publisher
      ARW S  r--  Sony Alpha DSLR RAW
      AVS U  rw+  AVS X image
        B S  rw+  Raw blue samples
      BMP P  rw-  Microsoft Windows bitmap image
     BMP2 P  -w-  Microsoft Windows bitmap image v2
     BMP3 P  -w-  Microsoft Windows bitmap image v3
      BRF S  -w-  BRF ASCII Braille format
        C S  rw+  Raw cyan samples
    CACHE U  ---  Magick Persistent Cache image format
     CALS S  rw-  Continuous Acquisition and Life-cycle Support Type 1 image
            Specified in MIL-R-28002 and MIL-PRF-28002
  CAPTION P  r--  Image caption
      CIN S  rw-  Cineon Image File
     CMYK S  rw+  Raw cyan, magenta, yellow, and black samples
    CMYKA S  rw+  Raw cyan, magenta, yellow, black, and opacity samples
      CR2 S  r--  Canon Photo RAW
      CRW S  r--  Canon Photo RAW
      CUR S  r--  Microsoft Cursor Icon
      CUT S  r--  DR Halo
      DCM S  r--  Digital Imaging and Communications in Medicine image
            See http://medical.nema.org/ for information on DICOM.
      DCR S  r--  Kodak Photo RAW
      DCX S  rw+  ZSoft IBM PC multi-page Paintbrush
      DNG S  r--  Adobe Digital Negative
      DPX P  rw-  SMPTE 268M-2003 (DPX 2.0)
            See http://www.smtpe.org/ for information on DPX.
     EPDF P  rw-  Encapsulated Portable Document Format
      EPI P  rw-  Adobe Encapsulated PostScript Interchange format
      EPS P  rw-  Adobe Encapsulated PostScript
     EPS2 P  -w-  Adobe Level II Encapsulated PostScript
     EPS3 P  -w+  Adobe Level III Encapsulated PostScript
     EPSF P  rw-  Adobe Encapsulated PostScript
     EPSI P  rw-  Adobe Encapsulated PostScript Interchange format
      ERF S  r--  Epson RAW Format
     EXIF P  rw-  Exif digital camera binary data
      FAX P  rw+  Group 3 FAX (Not TIFF Group3 FAX!)
     FITS S  rw-  Flexible Image Transport System
  FRACTAL S  r--  Plasma fractal image
        G S  rw+  Raw green samples
      GIF P  rw+  CompuServe graphics interchange format (version 89a)
    GIF87 P  rw-  CompuServe graphics interchange format (version 87a)
 GRADIENT P  r--  Gradual passing from one shade to another
     GRAY S  rw+  Raw gray samples
    GRAYA S  rw+  Raw gray samples + alpha
HISTOGRAM P  -w-  Histogram of the image
      HRZ S  r--  HRZ: Slow scan TV
     HTML S  -w-  Hypertext Markup Language and a client-side image map
      ICB S  rw+  Truevision Targa image
      ICC P  rw-  ICC Color Profile
      ICM P  rw-  ICC Color Profile
      ICO S  r--  Microsoft Icon
     ICON S  r--  Microsoft Icon
 IDENTITY P  r--  Hald CLUT identity image
    IMAGE P  r--  GraphicsMagick Embedded Image
     INFO S  -w+  Image descriptive information and statistics
     IPTC P  rw-  IPTC Newsphoto
 IPTCTEXT P  rw-  IPTC Newsphoto text format
IPTCWTEXT P  rw-  IPTC Newsphoto text format
   ISOBRL S  -w-  ISO/TR 11548-1 format
  ISOBRL6 S  -w-  ISO/TR 11548-1 format 6dot
        K S  rw+  Raw black samples
      K25 S  r--  Kodak Photo RAW
      KDC S  r--  Kodak Photo RAW
    LABEL P  r--  Image label
        M S  rw+  Raw magenta samples
      M2V S  -w+  MPEG Video Stream
      MAC S  r--  Mac Paint
      MAP U  rw-  Colormap intensities and indices
      MAT S  rw+  MATLAB Level 4.0-6.0 image formats
    MATTE S  -w+  MATTE raw opacity format
      MEF S  r--  Mamiya Photo RAW
     MIFF P  rw+  Magick Image File Format (GraphicsMagick 1.3.36)
     MONO S  rw-  Bi-level bitmap in least-significant-byte first order
      MPC U  rw+  Magick Persistent Cache image format
     MPEG S  -w+  MPEG Video Stream
      MPG S  -w+  MPEG Video Stream
      MRW S  r--  Minolta Photo RAW
      MTV U  rw+  MTV Raytracing image format
      MVG S  rw-  Magick Vector Graphics
      NEF S  r--  Nikon Electronic Format
     NULL P  rw-  Constant image of uniform color
        O S  rw+  Raw opacity samples
      ORF S  r--  Olympus Photo RAW
      OTB S  rw-  On-the-air bitmap
       P7 S  rw+  Xv thumbnail format
      PAL S  rw-  16bit/pixel interleaved YUV
     PALM U  r--  Palm pixmap
      PAM P  rw+  Portable Arbitrary Map format
      PBM P  rw+  Portable bitmap format (black/white)
      PCD S  rw-  Photo CD
     PCDS S  rw-  Photo CD
      PCL S  -w+  Page Control Language
      PCT S  rw-  Apple Macintosh QuickDraw/PICT
      PCX S  rw-  ZSoft IBM PC Paintbrush
      PDB U  rw+  Palm Database ImageViewer Format
      PDF P  rw+  Portable Document Format
      PEF S  r--  Pentax Electronic File
      PFA P  ---  Postscript Type 1 font (ASCII)
      PFB P  ---  Postscript Type 1 font (binary)
      PGM P  rw+  Portable graymap format (gray scale)
    PICON S  rw-  Personal Icon
     PICT S  rw-  Apple Macintosh QuickDraw/PICT
      PIX S  r--  Alias/Wavefront RLE image format
   PLASMA S  r--  Plasma fractal image
      PNM P  rw+  Portable anymap
      PPM P  rw+  Portable pixmap format (color)
  PREVIEW S  -w-  Show a preview an image enhancement, effect, or f/x
       PS P  rw+  Adobe PostScript
      PS2 P  -w+  Adobe Level II PostScript
      PS3 P  -w+  Adobe Level III PostScript
      PWP U  r--  Seattle Film Works
        R S  rw+  Raw red samples
      RAF S  r--  Fuji Photo RAW
      RAS S  rw+  SUN Rasterfile
      RGB S  rw+  Raw red, green, and blue samples
     RGBA S  rw+  Raw red, green, blue, and matte samples
      RLA U  r--  Alias/Wavefront image
      RLE U  r--  Utah Run length encoded image
      SCT U  r--  Scitex HandShake
      SFW U  r--  Seattle Film Works
      SGI S  rw-  Irix RGB image
    SHTML S  -w-  Hypertext Markup Language and a client-side image map
      SR2 S  r--  Sony Photo RAW
      SRF S  r--  Sony Photo RAW
  STEGANO S  r--  Steganographic image
      SUN S  rw+  SUN Rasterfile
      SVG S  ---  Scalable Vector Graphics
     SVGZ S  ---  Scalable Vector Graphics (ZIP compressed)
     TEXT S  rw+  ASCII Text
      TGA S  rw+  Truevision Targa image
     TILE P  r--  Tile image with a texture
            Use the syntax "-size WIDTHxHEIGHT TILE:imagename" to tile the
            specified tile image over a canvas image of size WIDTHxHEIGHT.
      TIM S  r--  PSX TIM
    TOPOL S  r--  TOPOL X Image
      TTF P  ---  TrueType font
      TXT S  rw+  ASCII Text
     UBRL S  -w-  Unicode Text format
    UBRL6 S  -w-  Unicode Text format 6dot
      UIL U  -w-  X-Motif UIL table
     UYVY S  rw-  16bit/pixel interleaved YUV
      VDA S  rw+  Truevision Targa image
    VICAR S  rw-  VICAR rasterfile format
      VID S  rw+  Visual Image Directory
     VIFF S  rw+  Khoros Visualization image
      VST S  rw+  Truevision Targa image
     WBMP S  rw-  Wireless Bitmap (level 0) image
      WPG U  r--  Word Perfect Graphics
      X3F S  r--  Foveon X3 (Sigma/Polaroid) RAW
      XBM S  rw-  X Windows system bitmap (black/white)
       XC P  r--  Constant image uniform color
      XCF S  r--  GIMP image
      XMP P  rw-  Adobe XML metadata
      XPM S  rw-  X Windows system pixmap (color)
       XV S  rw+  Khoros Visualization image
        Y S  rw+  Raw yellow samples
      YUV S  rw-  CCIR 601 4:1:1 or 4:2:2 (8-bit only)

 Meaning of 'L': P=Primary, S=Stable, U=Unstable
seblabbe commented 2 years ago
comment:12

The content of the second column is not the same in my case:

$ convert -list Format
   Format  Module    Mode  Description
-------------------------------------------------------------------------------
      3FR  DNG       r--   Hasselblad CFV/H3D39II
      3G2  MPEG      r--   Media Container
      3GP  MPEG      r--   Media Container
      AAI* AAI       rw+   AAI Dune image
[...]
   YCbCrA* YCbCr     rw+   Raw Y, Cb, Cr, and alpha samples
      YUV* YUV       rw-   CCIR 601 4:1:1 or 4:2:2

* native blob support
r read support
w write support
+ support for multiple images

Most of the doctests are doing a conversion (bunch of pngs) -> gif.

Do you think something based on

convert -list Format | grep PNG

could be a good test to detect that convert *.png output.gif works?

For info, for me it gives this:

      JNG* PNG       rw-   JPEG Network Graphics
      MNG* PNG       rw+   Multiple-image Network Graphics (libpng 1.6.37)
      PNG* PNG       rw-   Portable Network Graphics (libpng 1.6.37)
           See http://www.libpng.org/ for details about the PNG format.
    PNG00* PNG       rw-   PNG inheriting bit-depth, color-type from original if possible
    PNG24* PNG       rw-   opaque or binary transparent 24-bit RGB (zlib 1.2.11)
    PNG32* PNG       rw-   opaque or transparent 32-bit RGBA
    PNG48* PNG       rw-   opaque or binary transparent 48-bit RGB
    PNG64* PNG       rw-   opaque or transparent 64-bit RGBA
     PNG8* PNG       rw-   8-bit indexed with optional binary transparency
orlitzky commented 2 years ago
comment:13

Replying to @seblabbe:

Most of the doctests are doing a conversion (bunch of pngs) -> gif.

Do you think something based on

convert -list Format | grep PNG

I suppose, but you would need to somehow test for read support of PNGs and write support of GIFs. Personally I would just try to convert a 1x1 pixel from png to gif and see if it works.

seblabbe commented 2 years ago
comment:14

Good idea, I will post a branch soon.

seblabbe commented 2 years ago

Commit: e8dbdf9

seblabbe commented 2 years ago

New commits:

ef3179033092: adding method is_functional to imagemagick feature
e8dbdf933092: print log when an error with convert command occurs in animate.py
seblabbe commented 2 years ago

Branch: u/slabbe/33092

seblabbe commented 2 years ago

Author: Sébastien Labbé

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

020e10133092: print log when an error with convert command occurs in animate.py
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from e8dbdf9 to 020e101

seblabbe commented 2 years ago
comment:17

Needs review!

Also, I would be curious to see the following outputs with the current branch for your machine:

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().is_present()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().is_functional()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().require()

Also this allows to run tests on the 5 failings files and list the skipped doctects:

sage -tp --long --show-skipped src/sage/combinat/crystals/mv_polytopes.py src/sage/combinat/tiling.py src/sage/combinat/words/paths.py src/sage/plot/animate.py src/sage/plot/plot3d/tachyon.py

For comparison, for me, it gives this:

Doctesting 5 files using 8 threads.
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/plot/plot3d/tachyon.py
    1 test not run due to known bugs
    0 tests not run because we ran out of time
    [406 tests, 13.74 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/combinat/crystals/mv_polytopes.py
    0 tests not run because we ran out of time
    [67 tests, 24.64 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/combinat/words/paths.py
    2 internet tests not run
    1 test for not implemented functionality not run
    0 tests not run because we ran out of time
    [521 tests, 64.60 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/plot/animate.py
    4 not tested tests not run
    0 tests not run because we ran out of time
    [125 tests, 103.21 s]
sage -t --long --random-seed=292498441106400212647883427867965055382 src/sage/combinat/tiling.py
    5 not tested tests not run
    0 tests not run because we ran out of time
    [489 tests, 211.99 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 212.1 seconds
    cpu time: 271.1 seconds
    cumulative wall time: 418.2 seconds
Features detected for doctesting: ffmpeg,imagemagick
seblabbe commented 2 years ago
comment:19

Most of the errors in the attached log were coming from imagemagick. I wait to get new inputs before touching ffmpeg features.

orlitzky commented 2 years ago
comment:20

I get "not functional",

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().is_present()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().is_functional()
FeatureTestResult('imagemagick', False)
sage: ImageMagick().require()

but it looks like the # optional - ImageMagick tests are still being run. For example,

File "src/sage/plot/animate.py", line 168, in sage.plot.animate.Animation
Failed example:
    a[:5]             # optional -- ImageMagick
Expected:
    Animation with 5 frames
Got:
    Command 
       'sage-native-execute convert -dispose Background -delay 20 -loop 0 *.png "/home/mjo/.sage/temp/gantu/4697/tmp_tlrqhxiz.gif"'
    returned non-zero exit status 1.
    Here is the content of the stderr:convert convert: No decode delegate for this image format (00000000.png).
    Here is the content of the stdout:
    Animation with 5 frames
seblabbe commented 2 years ago
comment:21

Replying to @orlitzky:

I get "not functional",

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().is_present()
FeatureTestResult('imagemagick', True)
sage: ImageMagick().is_functional()
FeatureTestResult('imagemagick', False)
sage: ImageMagick().require()

but it looks like the # optional - ImageMagick tests are still being run. For example,

I thought it was always the case that is_present() returns False when is_functional() returns False. But the above is a counterexample to this (it is a good thing, because it is more logical like that). I think my thinking was mislead by the way _is_present method is coded for the class Executable which is not consistent with the other type of features like the one here, see https://github.com/sagemath/sagetrac-mirror/blob/develop/src/sage/features/__init__.py?h=develop#n442

I think doctests are run for a feature when is_present() returns True which explains why the feature imagemagick are tested in your case. It should also check that it is functional as well.

Also, require() currently only check that is_present() returns True. This is the other reason I was thinking that is_present() should return False if is_functional() return False.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 020e101 to bd568fe

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

bd568fe33092: move the is_functional method of class ImageMagick(JoinFeature) to a method of class convert(Excutable)
seblabbe commented 2 years ago
comment:23

Okay, I found an alternate way of doing it. I moved the is_functional method into a class Executable for the convert as it is done within the file features/graphviz.py.

@orlitzky, can you test the same thing again?

Maybe changes needs to be done with respect to is_present(), is_functional() and require(). In particular, I have the following questions for @mkoeppe:

mkoeppe commented 2 years ago
comment:24

Replying to @seblabbe:

Maybe changes needs to be done with respect to is_present(), is_functional() and require(). In particular, I have the following questions for @mkoeppe:

  • why Feature.require() does not check that is_functional() is True?
  • should is_present() always return False when is_functional return False (even if the vocabulary that is used should logically allow it)? But this discussion should be done in another ticket.

I have opened #33114 for this discussion. I don't have special insights regarding the original design (done by @saraedum and revised by @jdemeyer) but I think it is likely that the design needs to be refined now that we are bringing it to more systematic use.

saraedum commented 2 years ago
comment:25

I don't think you are cleaning up the temporary files.

Have you considered using with TemporaryDirectory instead? See https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory.

This also lets you create a named file in the temporary directory and so you don't need to run with shell=True which might be considered a security issue by some.

saraedum commented 2 years ago

Reviewer: Julian Rüth

orlitzky commented 2 years ago
comment:27

Better now:

sage: from sage.features.imagemagick import ImageMagick
sage: ImageMagick().require()
---------------------------------------------------------------------------
FeatureNotPresentError                    Traceback (most recent call last)
<ipython-input-2-501a65b4daea> in <module>
----> 1 ImageMagick().require()

~/src/sage.git/local/var/lib/sage/venv-python3.9/lib/python3.9/site-packages/sage/features/__init__.py in require(self)
    204         presence = self.is_present()
    205         if not presence:
--> 206             raise FeatureNotPresentError(self, presence.reason, presence.resolution)
    207 
    208     def __repr__(self):

FeatureNotPresentError: imagemagick is not available.
Running convert on a sample file returned non-zero exit status 1
No equivalent system packages for gentoo are known to Sage.
To install imagemagick using the Sage package manager, you can try to run:
  !sage -i imagemagick
No equivalent system packages for pip are known to Sage.
Further installation instructions might be available at https://www.imagemagick.org/.

And the test failures are gone.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

d5e5ab033092: improved the look of stderr and stdout when an error occurs
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from bd568fe to d5e5ab0

seblabbe commented 2 years ago
comment:29

Replying to @orlitzky:

And the test failures are gone.

All of them? Even the one listed in the description of this ticket related to ffmpeg?

I added a small commit to improve the way errors are printed. I also added the stderr output to the output you obtain when ImageMagick().require() fails.

orlitzky commented 2 years ago
comment:30

Replying to @seblabbe:

Replying to @orlitzky:

And the test failures are gone.

All of them? Even the one listed in the description of this ticket related to ffmpeg?

No, not the ffmpeg ones, obviously =)

I double-checked all of the # optional - ImageMagick tests and it looks like the only other format we use is ppm, but imagemagick support for ppm is apparently impossible to disable. So the existing test for a PNG conversion is probably all we need.

seblabbe commented 2 years ago
comment:31

Replying to @orlitzky:

No, not the ffmpeg ones, obviously =)

Ok! Please, can you attach the log of the remaining failures?

seblabbe commented 2 years ago
comment:32

Replying to @saraedum:

I don't think you are cleaning up the temporary files.

Indeed, I am not. Should I?

Have you considered using with TemporaryDirectory instead? See https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory.

Well the files are created in the current temporary folder SAGE_TMP within DOT_SAGE folder using the methods implemented in the misc folder: from sage.misc.temporary_file import tmp_filename. This method is already using the tempfile module within some temporary directory with some SageMath salt. It is used all over the place in sage:

$ git grep tmp_filename | wc
    264    1325   20892

And I don't remember any of its usage to contain any line which deletes the temporary files created (do you know one such place?). The temporary folder and the temporary files for instance all pictures generated by sage during the session gets deleted when sage closes as shown in the example below:

sage: SAGE_TMP
l'/home/slabbe/.sage/temp/labbe-laptop/1043566'
sage: !ls /home/slabbe/.sage/temp/labbe-laptop/1043566
ecl
sage: plot(sin(x),0,10)
Launched png viewer for Graphics object consisting of 1 graphics primitive
sage: !ls /home/slabbe/.sage/temp/labbe-laptop/1043566
ecl  tmp_w8bb8pxh.png
sage:                                                                           
Exiting Sage (CPU time 0m1.39s, Wall time 0m44.98s).

$ ls /home/slabbe/.sage/temp/labbe-laptop/1043566
ls: impossible d'accéder à '/home/slabbe/.sage/temp/labbe-laptop/1043566': Aucun fichier ou dossier de ce type

Can you explain what is wrong with the current approach?

orlitzky commented 2 years ago
comment:33

I spoke too soon. I re-enabled PNG support in my imagemagick and now these fail:

File "src/sage/plot/animate.py", line 565, in sage.plot.animate.Animation.?
Failed example:
    with open(td + 'my_animation.gif', 'rb') as f: print(b'\x21\xf9\x04\x08\x23\x00' in f.read())  # optional -- ImageMagick
Expected:
    True
Got:
    False
File "src/sage/plot/animate.py", line 1105, in sage.plot.animate.Animation.save
Failed example:
    with open(td + 'wave.gif', 'rb') as f: print(b'!\xf9\x04\x08\x14\x00' in f.read())  # optional -- ImageMagick
Expected:
    True
Got:
    False
File "src/sage/plot/animate.py", line 1112, in sage.plot.animate.Animation.save
Failed example:
    with open(td + 'wave.gif', 'rb') as f: print(b'!\xf9\x04\x08\x23\x00' in f.read())  # optional -- ImageMagick
Expected:
    True
Got:
    False

I wonder what those tests are trying to check. Maybe instead we should just check for the magic "GIF8" bits at the beginning of the file...

seblabbe commented 2 years ago
comment:34

Replying to @orlitzky:

I wonder what those tests are trying to check.

These tests are there since the very beginning of Sage. I think the idea is that it is difficult to test that a GIF is correct as we do not want a human to look at them each time. The idea is to only check that some binary string appear in the middle of the created gif file.

So if it is False, my first advice would be to actually look at the animation with your eyes, because possibly they are broken. Are they?

Maybe instead we should just check for the magic "GIF8" bits at the beginning of the file...

Depends on the answer to the first question.

orlitzky commented 2 years ago
comment:35

Replying to @seblabbe:

So if it is False, my first advice would be to actually look at the animation with your eyes, because possibly they are broken. Are they?

No, they look fine.

orlitzky commented 2 years ago
comment:36

Replying to @orlitzky:

I wonder what those tests are trying to check. Maybe instead we should just check for the magic "GIF8" bits at the beginning of the file...

So long as we're already using imagemagick, something like

$ identify -format "%m" /home/mjo/.sage/temp/gantu/29047/dir_b77_l8ae/my_animation.gif
GIFGIFGIFGIFGIFGIFGIFGIFGIF

is nice and easy.

saraedum commented 2 years ago
comment:37

Replying to @seblabbe:

Replying to @saraedum:

I don't think you are cleaning up the temporary files.

Indeed, I am not. Should I?

Ok. Maybe that's the SageMath way of doing things. (I guess all this functionality is from Python <3.2 when there was no good support for temporary directories in Python.) If SageMath is cleaning up already, that's probably fine then. (For me $SAGE_TMP is annoying because it is often located on a remote drive...)

Can you explain what is wrong with the current approach?

I was originally just very confused about the *.png. I don't think you need this. You can just convert the file that you created directly instead, can't you? This will also not interact with other .png files that happen to exist in this directory.

I would really try to get rid of the shell=True, though. Apart from security concerns, shell=True makes things break in all kinds of weird ways in strangely configured environments.

seblabbe commented 2 years ago
comment:38

Replying to @saraedum:

Ok. Maybe that's the SageMath way of doing things. (I guess all this functionality is from Python <3.2 when there was no good support for temporary directories in Python.) If SageMath is cleaning up already, that's probably fine then. (For me $SAGE_TMP is annoying because it is often located on a remote drive...)

Well even older, the tmp_filename stuff dates back to the very early sagemath stuff on Python 2.x. After 15 years, it is possible that this can be done in a better way in 2022. But I would suggest to improve that in another ticket. Feel free to open one.

Can you explain what is wrong with the current approach?

I was originally just very confused about the *.png. I don't think you need this. You can just convert the file that you created directly instead, can't you? This will also not interact with other .png files that happen to exist in this directory.

The *.png was taken from the sage/plot/animate.py where a bunch of PNGs are turned into a animation. It is true here that I can replace the *.png by the only one file which is targeted here. Good idea. I will do this.

I would really try to get rid of the shell=True, though. Apart from security concerns, shell=True makes things break in all kinds of weird ways in strangely configured environments.

Ok, I will check if it works without. If it's there, it was for a reason I think. But let me check this tomorrow.

saraedum commented 2 years ago
comment:39

Ok, I will check if it works without. If it's there, it was for a reason I think. But let me check this tomorrow.

The shell=True is only there to make the *.png work I think.

orlitzky commented 2 years ago

Attachment: ffmpeg.log

orlitzky commented 2 years ago
comment:40

I attached another log file with just the remaining ffmpeg failures.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

7a2fdfd33092: now with shell=False
78fc81933092: adding is_functional method to feature ffmpeg
396142333092: improved the way errors are worded and returned
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from d5e5ab0 to 3961423

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

df0a2d633092: adding is_functional method to feature ffmpeg
9810e5833092: improved the way errors are worded and returned