Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#25305 closed enhancement (fixed)

Adding has_graphviz, has_ffmpeg, has_imagemagick to sage.doctest.external

Reported by: slabbe Owned by:
Priority: major Milestone: sage-8.3
Component: doctest framework Keywords: thursdaysbdx
Cc: Merged in:
Authors: Sébastien Labbé Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: ba9cf98 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by slabbe)

Implement this:

sage: from sage.doctest.external import has_graphviz, has_ffmpeg, has_imagemagick
sage: has_graphviz()   # random
True
sage: has_ffmpeg()   # random
True
sage: has_imagemagick()   # random
True

so that graphviz, ffmpeg, imagemagick are included in the external software to be detected:

$ sage -t --optional=external,sage src/sage/graphs/graph_latex.py src/sage/plot/animate.py
...
Using --optional=external
External software to be detected: cplex,ffmpeg,graphviz,gurobi,
imagemagick,internet,latex,macaulay2,magma,maple,mathematica,
matlab,octave,scilab
...

Change History (32)

comment:1 Changed 19 months ago by slabbe

  • Branch set to u/slabbe/25305
  • Commit set to 99f37acd186afa3ec558e86935fc4e6976f9a9b0
  • Status changed from new to needs_review
  • Summary changed from Adding method sage.external.has_graphviz to Adding method sage.doctest.external.has_graphviz

New commits:

99f37ac25305: adding sage.doctest.external.has_graphviz

comment:2 Changed 19 months ago by slabbe

  • Summary changed from Adding method sage.doctest.external.has_graphviz to Adding sage.doctest.external.has_graphviz

comment:3 Changed 19 months ago by git

  • Commit changed from 99f37acd186afa3ec558e86935fc4e6976f9a9b0 to e09d9b7b0a1c3e4c0614bf8639b0ae1252ea220c

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

e09d9b725305: adding has_ffmpeg and has_imagemagick to sage.doctest.external

comment:4 Changed 19 months ago by slabbe

I added has_ffmpeg and have_imagemagick in the second commit.

For references, the list tests_not_run below contains all tags of tests that are not run by default. The list was obtained from grep "not run" on the log of ./sage -t -p --all --long --show-skipped:

sage: tests_not_run = ['4ti2', 'and', 'axiom', 'beautifulsoup', 'benzene', 'bliss', 'buckygen',
....: 'cbc', 'chomp', 'coxeter3', 'cplex', 'cryptominisat', 'csdp', 'cunningham',
....: 'database', 'database_cremona_ellcurve', 'database_gap',
....: 'database_jones_numfield', 'database_kohel', 'database_mutation_class',
....: 'database_odlyzko_zeta', 'database_pari', 'database_stein_watkins',
....: 'database_symbolic_data', 'debug', 'fes', 'ffmpeg', 'fricas', 'frobby',
....: 'gambit', 'gap3', 'gap_packages', 'gdb', 'giacpy_sage', 'ginv', 'glucose',
....: 'gmp', 'gmpy2', 'gnuplot', 'gperftools', 'graphviz', 'growing', 'gurobi',
....: 'hmisc', 'imagemagick', 'internet', 'java', 'kash', 'latex', 'latte_int',
....: 'libbraiding', 'libhomfly', 'libjpeg', 'lie', 'lrs', 'lrslib', 'macaulay2',
....: 'magma', 'maple', 'mathematica', 'mathematicafrontend', 'matlab', 'mcqd',
....: 'meataxe', 'mupad', 'nonexistent_lp_solver', 'octave', 'ore_algebra', 'phc',
....: 'plantri', 'polymake', 'polytopes_db_4d', 'py3', 'pynormaliz', 'python_igraph',
....: 'qepcad', 'randomly', 'required', 'requires', 'rgraphics', 'rsat', 'runsnake',
....: 'scilab', 'sirocco', 'sloane_database', 'surf', 'tdlib', 'the', 'tides',
....: 'time', 'topcom', 'webbrowser']

And this is a tentative list of all "external" packages:

sage: S = set(package_versions('standard'))
sage: O = set(package_versions('optional'))
sage: E = set(package_versions('experimental'))
sage: set(tests_not_run) - (S|O|E)
{'and',
 'axiom',
 'beautifulsoup',
 'chomp',
 'cplex',
 'cunningham',
 'database',
 'debug',
 'fes',
 'ffmpeg',
 'ginv',
 'glucose',
 'gnuplot',
 'gperftools',
 'graphviz',
 'growing',
 'gurobi',
 'hmisc',
 'imagemagick',
 'internet',
 'java',
 'kash',
 'latex',
 'libjpeg',
 'lrs',
 'macaulay2',
 'magma',
 'maple',
 'mathematica',
 'mathematicafrontend',
 'matlab',
 'mupad',
 'nonexistent_lp_solver',
 'octave',
 'phc',
 'polytopes_db_4d',
 'py3',
 'randomly',
 'required',
 'requires',
 'rgraphics',
 'rsat',
 'runsnake',
 'scilab',
 'sloane_database',
 'the',
 'time',
 'webbrowser'}

Maybe we want to detect some more external packages...

Last edited 19 months ago by slabbe (previous) (diff)

comment:5 Changed 19 months ago by slabbe

  • Summary changed from Adding sage.doctest.external.has_graphviz to Adding has_graphviz, has_ffmpeg, has_imagemagick to sage.doctest.external

comment:6 Changed 19 months ago by git

  • Commit changed from e09d9b7b0a1c3e4c0614bf8639b0ae1252ea220c to fe6163f0d36ea1e36588e3eff0101024dc0dd4b2

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

fe6163f25305: adding has_ffmpeg and has_imagemagick to sage.doctest.external

comment:7 Changed 19 months ago by slabbe

  • Description modified (diff)

comment:8 Changed 19 months ago by slabbe

  • Description modified (diff)

comment:9 follow-ups: Changed 19 months ago by vdelecroix

Shouldn't this use the recently merged #20382?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 19 months ago by vdelecroix

Replying to vdelecroix:

Shouldn't this use the recently merged #20382?

Actually, I think that all the has_xxx stuff would better be features.

comment:11 Changed 19 months ago by slabbe

  • Description modified (diff)

comment:12 Changed 19 months ago by slabbe

  • Branch changed from u/slabbe/25305 to u/slabbe/25305-using-features
  • Commit changed from fe6163f0d36ea1e36588e3eff0101024dc0dd4b2 to cb6e0887112a2cd8be4233ba36961a37b20cef41

New commits:

cb6e08825305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external

comment:13 in reply to: ↑ 9 Changed 19 months ago by slabbe

Replying to vdelecroix:

Shouldn't this use the recently merged #20382?

Ok as you wish. I am now using find_executable from the Features stuff instead of the have_program from the misc stuff.

comment:14 in reply to: ↑ 10 ; follow-up: Changed 19 months ago by slabbe

Replying to vdelecroix:

Actually, I think that all the has_xxx stuff would better be features.

Why better? To me adding three new has_key functions to the external.py module makes things better. But changing has_maple in a way that it uses features does not make anything better.

The way the external.py module is coded right now is that the presence of an external software with name tag is detected with a method whose name is has_tag. The goal of this ticket is not to redesign all of this external.py module. So I suggest that further uses of feature inside of external.py be postpone to another ticket if this is at all pertinent.

Needs review!

comment:15 in reply to: ↑ 14 Changed 19 months ago by vdelecroix

  • Keywords thursdaysbdx added
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_info

Replying to slabbe:

Replying to vdelecroix:

Actually, I think that all the has_xxx stuff would better be features.

Why better? To me adding three new has_key functions to the external.py module makes things better. But changing has_maple in a way that it uses features does not make anything better.

I just realized that it is not possible for has_maple. The maple interface allows subtle configurations such as

  • an alternative name for the executable
  • a remote execution through ssh

This is not the kind of things a feature is aware of.

The way the external.py module is coded right now is that the presence of an external software with name tag is detected with a method whose name is has_tag. The goal of this ticket is not to redesign all of this external.py module. So I suggest that further uses of feature inside of external.py be postpone to another ticket if this is at all pertinent.

In your situation, you are just checking for the presence of executable in the path. This is exactly what sage.features.Executable is about. The main difference between `has_XXX_ and a feature is that

  • a feature is not only about doctest
  • a feature can run small tests
  • meaningful error messages
  • this is not run twice in the same session

What I wanted to suggest with my message is to introduce sage.features.graphviz, sage.features.ffmpeg and sage.features.imagemagick and simply use

def has_ffmpeg():
    from sage.features.ffmpeg import FFmpeg
    return FFmpeg.is_present()

The software FFmpeg is indeed used in other parts of Sage (e.g. animate) and it would help to have it as a feature.

comment:16 Changed 19 months ago by slabbe

  • Branch changed from u/slabbe/25305-using-features to u/slabbe/25305-with-Executable-class
  • Commit changed from cb6e0887112a2cd8be4233ba36961a37b20cef41 to 034678bda18e59ebfff62dce7c813ad6ef249147
  • Status changed from needs_info to needs_review

I now creates the files for ffmpeg, imagemagick and graphviz in the feature module and I am using these to solve the problem described in this ticket.

I suggest that the use of the FFmpeg().require() in animate.py etc be postpone to an indenpendent ticket.


New commits:

034678b25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external

comment:17 Changed 19 months ago by git

  • Commit changed from 034678bda18e59ebfff62dce7c813ad6ef249147 to 9686e9705b77420610a6ed978392bafd8240690a

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

9686e9725305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external

comment:18 Changed 19 months ago by git

  • Commit changed from 9686e9705b77420610a6ed978392bafd8240690a to eeaa92f39f423a879e68bd7f0c35f11e563befeb

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

eeaa92f25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external

comment:19 Changed 19 months ago by slabbe

I fixed some doctests and removed import os,subprocess in the previous two forced pushed.

comment:20 Changed 19 months ago by slabbe

Needs review!

comment:21 Changed 19 months ago by vdelecroix

  • Status changed from needs_review to needs_work

Image magick is a suite and not a unique executable:

  • magick
  • magick-script
  • convert
  • mogrify
  • identify
  • composite
  • montage
  • compare
  • etc

comment:22 Changed 19 months ago by slabbe

Can you provide an example where a command from imagemagick other than convert is used in the sage source code?

$ sage -grep mogrify
$ sage -grep magick-script
$ sage -grep magick
sage/misc/latex.py:    http://www.imagemagick.org
sage/misc/latex.py:            print("http://www.imagemagick.org to download these programs.")
sage/misc/latex.py:            print("Go to http://www.imagemagick.org to download it.")
sage/misc/latex.py:            print("Go to http://www.imagemagick.org to download it.")
sage/combinat/words/paths.py:            See www.imagemagick.org, for example.
sage/graphs/graph_latex.py:- convert: http://www.imagemagick.org (the ImageMagick suite)
sage/plot/animate.py:- `ImageMagick <http://www.imagemagick.org>`_
sage/plot/animate.py:              See www.imagemagick.org and www.ffmpeg.org for more information.
sage/plot/animate.py:        - `ImageMagick <http://www.imagemagick.org>`_
sage/plot/animate.py:See www.imagemagick.org and www.ffmpeg.org for more information."""
sage/plot/animate.py:See www.imagemagick.org and www.ffmpeg.org for more information."""
sage/plot/animate.py:              See www.imagemagick.org and www.ffmpeg.org for more information.

etc.

comment:23 Changed 19 months ago by git

  • Commit changed from eeaa92f39f423a879e68bd7f0c35f11e563befeb to cb740337eb81fbdf00180ffececca0cef2edb875

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

4a7e6b125305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external
84ded4225305: added doc about only convert being currently checked
cb7403325305: fix doctests

comment:24 Changed 19 months ago by slabbe

  • Status changed from needs_work to needs_review

Branch rebased on top of 8.3.beta2.

Added doc about the fact that currently only the presence of convert is checked.

I also fix the doctest that was failing on the patchbot.

Needs review.

comment:25 Changed 18 months ago by vdelecroix

You forgot the copyright.

The rest looks ok.

comment:26 Changed 18 months ago by vdelecroix

  • Status changed from needs_review to needs_work

comment:27 Changed 18 months ago by git

  • Commit changed from cb740337eb81fbdf00180ffececca0cef2edb875 to ba9cf98f587a0304642946797b995f6790835aad

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

d4c5fec25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external
871254225305: added doc about only convert being currently checked
baa0bae25305: fix doctests
ba9cf9825305: added licence to newly created files

comment:28 Changed 18 months ago by slabbe

  • Status changed from needs_work to needs_review

Rebased the branch on top of 8.3.beta4.

Added a commit which adds the licence to the newly created files.

Needs review.

comment:29 Changed 18 months ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:30 Changed 18 months ago by slabbe

Thank you for the review

comment:31 Changed 18 months ago by vbraun

  • Branch changed from u/slabbe/25305-with-Executable-class to ba9cf98f587a0304642946797b995f6790835aad
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:32 Changed 18 months ago by slelievre

  • Commit ba9cf98f587a0304642946797b995f6790835aad deleted

I noticed that some of the doctests have

# optional -- graphviz

while some other doctests have

# optional: graphviz

Are both styles equivalent?

Note: See TracTickets for help on using tickets.