Opened 4 years ago

Closed 4 years ago

Last modified 4 years 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, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

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 4 years 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 4 years ago by slabbe

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

comment:3 Changed 4 years 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 4 years ago by slabbe

I added has_ffmpeg and have_imagemagick in the second commit. For references, here are all 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:

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

Among them many are optional packages that are already automatically detected. What are the other external packages that we should also test for?

Version 0, edited 4 years ago by slabbe (next)

comment:5 Changed 4 years 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 4 years 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 4 years ago by slabbe

  • Description modified (diff)

comment:8 Changed 4 years ago by slabbe

  • Description modified (diff)

comment:9 follow-ups: Changed 4 years ago by vdelecroix

Shouldn't this use the recently merged #20382?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years 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 4 years ago by slabbe

  • Description modified (diff)

comment:12 Changed 4 years 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 4 years 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 4 years 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 4 years 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 4 years 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 4 years 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 4 years 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 4 years ago by slabbe

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

comment:20 Changed 4 years ago by slabbe

Needs review!

comment:21 Changed 4 years 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 4 years 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 4 years 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 4 years 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 4 years ago by vdelecroix

You forgot the copyright.

The rest looks ok.

comment:26 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to needs_work

comment:27 Changed 4 years 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 4 years 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 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:30 Changed 4 years ago by slabbe

Thank you for the review

comment:31 Changed 4 years 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 4 years 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.