Do not fail when `java` is available but `jmol` is not. #36769

tornaria · 2023-11-25T19:20:30Z

If java is not available, sagemath will fallback to tachyon gracefully.

When java is available, sagemath will assume jmol is available and error if not. This commit fixes the issue by implementing a method is_jmol_available() to replace is_jvm_available().

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have created tests covering the changes.

mkoeppe · 2023-11-25T20:21:58Z

Would you mind rewriting it to go through sage.features?

tornaria · 2023-11-25T22:10:17Z

Would you mind rewriting it to go through sage.features?

I don't see what problem would that solve.

mkoeppe · 2023-11-25T22:14:40Z

It could make the feature jmol available, suitable for conditionalizing doctests using # needs jmol.

tornaria · 2023-11-25T22:29:29Z

It could make the feature jmol available, suitable for conditionalizing doctests using # needs jmol.

And what problem would that solve?

mkoeppe · 2023-11-25T22:34:38Z

Path to #31027?

tornaria · 2023-11-25T22:51:30Z

Path to #31027?

Three years old and counting. In fact jmol is optional already. I don't have it installed and nothing ever failed for this reason, until today (something installed some java for some reason; sage still works, is only doctesting that fails).

I'd rather this PR be merged now (ideally for 10.2) and whoever wants can rework this as a feature for #31027. The check is very easy: if you run java -jar {JMOL_DIR}/JmolData.jar -i it will succeed if jmol is available and works, and fail if it isn't.

mkoeppe · 2023-11-26T00:48:45Z

I don't have time to test this at the moment, but you may want to look at what's going on in @tobiasdiez's conda workflow here

tornaria · 2023-11-26T16:08:54Z

I don't have time to test this at the moment, but you may want to look at what's going on in @tobiasdiez's conda workflow here

It seems just stdout or stderr output. I'll just silence the subprocess() call. My version of JmolData.jar claims -i silences it, and it does. Apparently conda version doesn't.

In fact, it's a bit overkill, just checking if the jar file exists is enough to solve my issue, so if the current version doesn't work I'll just do that.

tornaria · 2023-11-27T00:54:40Z

The failure is an unrelated timeout. Something very strange is happening, I was able to kind of reproduce this as follows:

$ sage -tp 16 --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py{,,,,,,,}{,}
too many failed tests, not using stored timings
Running doctests with ID 2023-11-26-21-39-35-a86f2520.
Running with SAGE_LOCAL='/usr' and SAGE_VENV='/usr'
Using --optional=pip,sage
Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,cvxopt,cvxopt,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_cubic_hecke,database_jones_numfield,database_knotinfo,dvipng,fpylll,gap_package_atlasrep,gap_package_design,gap_package_grape,gap_package_guava,gap_package_hap,gap_package_polycyclic,gap_package_qpa,gap_package_quagroup,gfan,graphviz,imagemagick,ipython,jupymake,kenzo,latte_int,lrcalc_python,lrslib,mcqd,meataxe,mpmath,msolve,nauty,networkx,numpy,palp,pandoc,pdf2svg,pdftocairo,pexpect,phitigra,pillow,plantri,polytopes_db,polytopes_db_4d,pplpy,primecountpy,ptyprocess,pynormaliz,pyparsing,python_igraph,requests,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.sat,sage.schemes,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,scipy,singular,sphinx,sympy,tdlib
Doctesting 16 files using 16 threads.
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 21.60 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 26.92 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 30.49 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 29.89 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 30.73 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 33.24 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 34.18 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 33.80 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 35.24 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 35.11 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 41.31 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 42.23 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 42.29 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 48.41 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 48.93 s]
sage -t --long --random-seed=286735480429121101562228604801325644303 src/sage/rings/tests.py
    [62 tests, 212.75 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 215.0 seconds
    cpu time: 494.5 seconds
    cumulative wall time: 747.1 seconds
Features detected for doctesting: sage.modules,sage.rings.finite_rings,sage.rings.number_field,sage.rings.padics,sage.symbolic

Box is 8 core / 8 thread, otherwise unloaded. I did 16 threads to stress it. Testing this file just once takes ~16 seconds so he first 15 jobs are "kind of" reasonable. The last one is not, and is close to the 300s timeout. I fixed the seed, but maybe this test has another random not controlled by seed and is very slow sometimes.

I think this supports doubling the "hard" timeout as in #36223 (hi @orlitzky), but also having a soft timeout that will keep testing BUT COMPLAIN LOUDLY (not just a warning). And maybe making the soft timeout smaller in general except a a few specific files can increase the soft timeout. The point is, for a file that normally takes 16s, it is very bad that sometimes takes > 300s.

I was about to give compliments for CI getting better, and then this... 🤣

tornaria · 2023-11-27T01:06:36Z

Quoting #36031 (comment):

Marking this as a blocker in the hope it can sneak into 10.2 with the other open blocker

tornaria · 2023-12-03T15:03:43Z

This should be an easy review. All this is doing is, in addition to check java is available, also check the jmol jar file is available using os.path.isfile(self.jmolpath()).

orlitzky · 2023-12-04T03:25:48Z

I agree with the change but we're dropping the cygwin path handling in #36779 and this will conflict (you were here first, but the other one is already positively reviewed).

Having it go through sage.features might be nice for #31027 but this is a bugfix as-is.

tornaria · 2023-12-04T11:43:03Z

I couldn't care less about either sage.features or cygwin.

I agree with the change but we're dropping the cygwin path handling in #36779 and this will conflict (you were here first, but the other one is already positively reviewed).

Having it go through sage.features might be nice for #31027 but this is a bugfix as-is.

Just merge this PR in #36779.

orlitzky · 2023-12-04T12:01:26Z

Just merge this PR in #36779.

If it weren't already positively-reviewed, I would, but I prefer not to annoy innocent bystanders by undoing their already-completed work.

tornaria · 2023-12-04T12:14:56Z

Just merge this PR in #36779.

If it weren't already positively-reviewed, I would, but I prefer not to annoy innocent bystanders by undoing their already-completed work.

There, I fixed that for you.

If java is not available, sagemath will fallback to tachyon gracefully. When java is available, sagemath will assume jmol is available and error if not. This commit fixes the issue by implementing a method `is_jmol_available()` to replace `is_jvm_available()`.

github-actions · 2023-12-04T14:16:16Z

Documentation preview for this PR (built with commit c7670d0; changes) is ready! 🎉

tornaria · 2023-12-10T12:39:49Z

Not in 10.2, not in 10.3.beta0, not in 10.3.beta1. Is priority "blocker" good for anything?

…not. If java is not available, sagemath will fallback to tachyon gracefully. When java is available, sagemath will assume jmol is available and error if not. This commit fixes the issue by implementing a method `is_jmol_available()` to replace `is_jvm_available()`. ### 📝 Checklist - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have created tests covering the changes. URL: sagemath#36769 Reported by: Gonzalo Tornaría Reviewer(s):

mkoeppe · 2023-12-10T17:28:00Z

Is priority "blocker" good for anything?

I think the only time when Volker's script takes the priority label into consideration is during the release candidate phase. Otherwise it's done in numerical order by PR numbers. That's certainly order-theoretically sound.

It was only needed for Cygwin (to handle path conversions), and we're dropping Cygwin support for real now. Depends on sagemath#36769 URL: sagemath#36779 Reported by: Michael Orlitzky Reviewer(s): Dima Pasechnik

tornaria added p: critical / 2 s: needs review labels Nov 25, 2023

tornaria force-pushed the fix-missing-jmol branch from c08bd58 to 3f433e2 Compare November 26, 2023 15:42

tornaria force-pushed the fix-missing-jmol branch from 3f433e2 to b1e5577 Compare November 26, 2023 20:13

tornaria added the p: blocker / 1 label Nov 27, 2023

tornaria added this to the sage-10.2 milestone Nov 27, 2023

tornaria requested review from kiwifb, orlitzky and antonio-rojas December 3, 2023 15:00

mkoeppe removed this from the sage-10.2 milestone Dec 3, 2023

tornaria mentioned this pull request Dec 4, 2023

Remove the pycygwin package #36779

Merged

dimpase force-pushed the fix-missing-jmol branch from b1e5577 to c7670d0 Compare December 4, 2023 13:37

dimpase added s: positive review and removed s: needs review labels Dec 4, 2023

tornaria removed the p: critical / 2 label Dec 6, 2023

vbraun merged commit 5d6ca00 into sagemath:develop Dec 14, 2023
15 of 20 checks passed

mkoeppe added this to the sage-10.3 milestone Dec 14, 2023

github-actions bot removed the s: positive review label Dec 14, 2023

tornaria deleted the fix-missing-jmol branch December 14, 2023 14:21

This was referenced Dec 25, 2023

Random timeout in rings/test #36961

Closed

Introduce os-dependent doctest tags # known bug: macos, # known bug: linux #36960

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not fail when `java` is available but `jmol` is not. #36769

Do not fail when `java` is available but `jmol` is not. #36769

tornaria commented Nov 25, 2023

mkoeppe commented Nov 25, 2023

tornaria commented Nov 25, 2023

mkoeppe commented Nov 25, 2023

tornaria commented Nov 25, 2023

mkoeppe commented Nov 25, 2023

tornaria commented Nov 25, 2023

mkoeppe commented Nov 26, 2023

tornaria commented Nov 26, 2023

tornaria commented Nov 27, 2023

tornaria commented Nov 27, 2023

tornaria commented Dec 3, 2023

orlitzky commented Dec 4, 2023

tornaria commented Dec 4, 2023

orlitzky commented Dec 4, 2023

tornaria commented Dec 4, 2023

github-actions bot commented Dec 4, 2023

tornaria commented Dec 10, 2023

mkoeppe commented Dec 10, 2023

Do not fail when java is available but jmol is not. #36769

Do not fail when java is available but jmol is not. #36769

Conversation

tornaria commented Nov 25, 2023

📝 Checklist

mkoeppe commented Nov 25, 2023

tornaria commented Nov 25, 2023

mkoeppe commented Nov 25, 2023

tornaria commented Nov 25, 2023

mkoeppe commented Nov 25, 2023

tornaria commented Nov 25, 2023

mkoeppe commented Nov 26, 2023

tornaria commented Nov 26, 2023

tornaria commented Nov 27, 2023

tornaria commented Nov 27, 2023

tornaria commented Dec 3, 2023

orlitzky commented Dec 4, 2023

tornaria commented Dec 4, 2023

orlitzky commented Dec 4, 2023

tornaria commented Dec 4, 2023

github-actions bot commented Dec 4, 2023

tornaria commented Dec 10, 2023

mkoeppe commented Dec 10, 2023

Do not fail when `java` is available but `jmol` is not. #36769

Do not fail when `java` is available but `jmol` is not. #36769