#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: |
Description (last modified by )
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
- 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
comment:2 Changed 4 years ago by
- Summary changed from Adding method sage.doctest.external.has_graphviz to Adding sage.doctest.external.has_graphviz
comment:3 Changed 4 years ago by
- Commit changed from 99f37acd186afa3ec558e86935fc4e6976f9a9b0 to e09d9b7b0a1c3e4c0614bf8639b0ae1252ea220c
Branch pushed to git repo; I updated commit sha1. New commits:
e09d9b7 | 25305: adding has_ffmpeg and has_imagemagick to sage.doctest.external
|
comment:4 Changed 4 years ago by
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...
comment:5 Changed 4 years ago by
- 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
- Commit changed from e09d9b7b0a1c3e4c0614bf8639b0ae1252ea220c to fe6163f0d36ea1e36588e3eff0101024dc0dd4b2
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fe6163f | 25305: adding has_ffmpeg and has_imagemagick to sage.doctest.external
|
comment:7 Changed 4 years ago by
- Description modified (diff)
comment:8 Changed 4 years ago by
- Description modified (diff)
comment:9 follow-ups: ↓ 10 ↓ 13 Changed 4 years ago by
Shouldn't this use the recently merged #20382?
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 14 Changed 4 years ago by
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
- Description modified (diff)
comment:12 Changed 4 years ago by
- Branch changed from u/slabbe/25305 to u/slabbe/25305-using-features
- Commit changed from fe6163f0d36ea1e36588e3eff0101024dc0dd4b2 to cb6e0887112a2cd8be4233ba36961a37b20cef41
New commits:
cb6e088 | 25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external
|
comment:13 in reply to: ↑ 9 Changed 4 years ago by
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: ↓ 15 Changed 4 years ago by
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
- 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 theexternal.py
module makes things better. But changinghas_maple
in a way that it usesfeatures
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 nametag
is detected with a method whose name ishas_tag
. The goal of this ticket is not to redesign all of thisexternal.py
module. So I suggest that further uses of feature inside ofexternal.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
- 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:
034678b | 25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external
|
comment:17 Changed 4 years ago by
- Commit changed from 034678bda18e59ebfff62dce7c813ad6ef249147 to 9686e9705b77420610a6ed978392bafd8240690a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9686e97 | 25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external
|
comment:18 Changed 4 years ago by
- Commit changed from 9686e9705b77420610a6ed978392bafd8240690a to eeaa92f39f423a879e68bd7f0c35f11e563befeb
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
eeaa92f | 25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external
|
comment:19 Changed 4 years ago by
I fixed some doctests and removed import os,subprocess
in the previous two forced pushed.
comment:20 Changed 4 years ago by
Needs review!
comment:21 Changed 4 years ago by
- 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
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
- Commit changed from eeaa92f39f423a879e68bd7f0c35f11e563befeb to cb740337eb81fbdf00180ffececca0cef2edb875
comment:24 Changed 4 years ago by
- 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
You forgot the copyright.
The rest looks ok.
comment:26 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:27 Changed 4 years ago by
- Commit changed from cb740337eb81fbdf00180ffececca0cef2edb875 to ba9cf98f587a0304642946797b995f6790835aad
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d4c5fec | 25305: adding has_ffmpeg, has_imagemagick, has_graphviz to sage.doctest.external
|
8712542 | 25305: added doc about only convert being currently checked
|
baa0bae | 25305: fix doctests
|
ba9cf98 | 25305: added licence to newly created files
|
comment:28 Changed 4 years ago by
- 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
- Status changed from needs_review to positive_review
comment:30 Changed 4 years ago by
Thank you for the review
comment:31 Changed 4 years ago by
- 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
- Commit ba9cf98f587a0304642946797b995f6790835aad deleted
I noticed that some of the doctests have
# optional -- graphviz
while some other doctests have
# optional: graphviz
Are both styles equivalent?
New commits:
25305: adding sage.doctest.external.has_graphviz