Opened 5 years ago

Closed 4 years ago

#18904 closed enhancement (fixed)

Automatically test optional non-packages (CPLEX, Gurobi, Maple?, ..)

Reported by: ncohen Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: doctest framework Keywords:
Cc: vbraun, jdemeyer, dcoudert Merged in:
Authors: Nathann Cohen Reviewers: Kwankyu Lee
Report Upstream: N/A Work issues:
Branch: public/18904 (Commits) Commit: b7213efaf806139f42c9f6b8d3b354c4dbf05047
Dependencies: Stopgaps:

Description (last modified by ncohen)

With this branch, the default value of --optional in sage -t is larger than the list of installed new-style optional packages.

In particular, cplex and gurobi (which are not optional packages as they are proprietary) are added to the list (some tests are too slow to be run with glpk, e.g. #18871).

More can be added to that list. If you know a better way to implement it, I am all ears.

Nathann

sage-devel thread: https://groups.google.com/d/topic/sage-devel/M98nD7833KM/discussion

Currently handles Gurobi, Cplex, Maple, Matlab, Macaulay2, Octave, Scilab.

(mathematica cannot be included before #18908 is fixed)

Change History (67)

comment:1 Changed 5 years ago by ncohen

  • Branch set to public/18904
  • Commit set to ef53635ae85dbc5afdfd2128c7238bd404321d97
  • Status changed from new to needs_review

New commits:

ef53635trac #18904: Automatically test optional non-packages (CPLEX, Gurobi, Maple?, ..)

comment:2 follow-up: Changed 5 years ago by vbraun

IMHO you should have to explicitly specify the optional packages that we don't distribute (and, therefore, don't know whether they are of a suitable version or working at all, or that the user has a license to start them, etc).

comment:3 in reply to: ↑ 2 Changed 5 years ago by ncohen

IMHO you should have to explicitly specify the optional packages that we don't distribute (and, therefore, don't know whether they are of a suitable version or working at all, or that the user has a license to start them, etc).

I am not sure that I understand what you propose. Do you want to have somewhere a list of the packages that we do nto distribute, along with a list of functions testing if they can be used (e.g. detect licenses)? In which file, and how?

For CPLEX and Gurobi, the tests I used handle the case when the license is wrong.

sage: MixedIntegerLinearProgram(solver='gurobi')
...
MIPSolverException: 'Gurobi: HostID mismatch (licensed to 52332a80, hostid is 230063bf) (GRB_ERROR_NO_LICENSE)'

Nathann

comment:4 follow-up: Changed 5 years ago by dimpase

say NO to blind copy/paste! Do

           for sol in ["cplex", "gurobi"]:
                try:
                    MixedIntegerLinearProgram(solver=sol)
                        options.optional.add(sol)
                    except:
                        pass

comment:5 in reply to: ↑ 4 Changed 5 years ago by ncohen

say NO to blind copy/paste! Do

Proposing a commit is the trendy way to say *I care*.

Last edited 5 years ago by ncohen (previous) (diff)

comment:6 follow-up: Changed 5 years ago by ncohen

Er... I could have done better: "Nothing says 'I care' like 'here is a commit'".

comment:7 in reply to: ↑ 6 ; follow-up: Changed 5 years ago by dimpase

But I'd need write access to your brain then...

Last edited 5 years ago by ncohen (previous) (diff)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by ncohen

But I'd need write access to your brain then...

I am sure that you can manage with a write access to public/*.

comment:9 in reply to: ↑ 8 Changed 5 years ago by dimpase

Replying to ncohen:

But I'd need write access to your brain then...

I am sure that you can manage with a write access to public/*.

do you pull from public opinion at all?

comment:10 Changed 5 years ago by git

  • Commit changed from ef53635ae85dbc5afdfd2128c7238bd404321d97 to 091ed73139ddc886260854e59499eb0e5eca9e91

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

091ed73trac #18904: More optional softwares

comment:11 follow-up: Changed 5 years ago by ncohen

More interfaces. As I needed a loop for the others, I also converted the MILP test as a loop.

Still ugly, so if somebody has a nice idea to rewrite it better.. :-/

Nathann

comment:12 Changed 5 years ago by git

  • Commit changed from 091ed73139ddc886260854e59499eb0e5eca9e91 to 9a011eb0a49a3041ae8574e9ce760a8556516417

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

9a011ebtrac #18904: More optional softwares

comment:13 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

More interfaces. As I needed a loop for the others, I also converted the MILP test as a loop.

Still ugly, so if somebody has a nice idea to rewrite it better.. :-/

not tested (don't know how to test that script in sage/doctests):

        def tryer(s):
            exec('from sage.interfaces.'+s+' import '+s)
            try:
                eval(s+'(1)')
                options.optional.add(s)
            except:
                pass

        for stuff in ['matlab', 'maple', 'macaulay2', 'octave', 'scilab']:
            tryer(stuff)

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by ncohen

not tested (don't know how to test that script in sage/doctests):

I thought that I would be hung if I attempted something like that.

Nathann

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

not tested (don't know how to test that script in sage/doctests):

I thought that I would be hung if I attempted something like that.

no, to the contrary - cool programmers don't write runnable code, they write code that writes runnable code :-)

comment:16 in reply to: ↑ 15 Changed 5 years ago by ncohen

no, to the contrary - cool programmers don't write runnable code, they write code that writes runnable code :-)

Well, considering how ugly my version is, perhaps it is comparatively better. Let's see what Jeroen and Volker think of it.

Nathann

comment:17 Changed 5 years ago by ncohen

  • Description modified (diff)

comment:18 Changed 5 years ago by ncohen

  • Description modified (diff)

comment:19 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Are we sure that we actually want to do this? We have no control over those external packages. What if somebody has magma but with a version that is not fully compatible with Sage? With Sage's optional packages, that issue does not occur since we have full control over those packages.

A few comments on the code:

  1. Never use except:. Use except Exception: or some other specific exception class.
  2. Code structure: can we please move all this detect-optional-packages code to a separate function?
  3. Performance: how much time does it take to run all those tests? Note that #13540 has a system for "lazy" checking of optional dependencies: the check for a given package is only done if we are actually running a test involving that package.

comment:20 in reply to: ↑ 19 ; follow-ups: Changed 5 years ago by ncohen

Are we sure that we actually want to do this?

If you oppose the changes involving Maple/Matlab and others then I do not mind removing them for you. We can keep Gurobi and CPLEX for they are only used by Sage if the user explicitly linked them with it.

  1. Never use except:. Use except Exception: or some other specific exception class.

I thought that here it was pretty safe: I do not want this test to ever break, and if there is something wrong with the mathematica/maple/... interface, then other doctests will have to report it.

  1. Code structure: can we please move all this detect-optional-packages code to a separate function?

I will add a commit.

  1. Performance: how much time does it take to run all those tests?

Almost nothing, give it a try. That's clearly negligible against the time it takes to run the sage -b part of sage -btp.

Note that #13540 has a system for "lazy" checking of optional dependencies

I do not see a need for that given the time it takes to run those tests. Especially, if you want the maple/matlab/others to be removed from the list.

Nathann

comment:21 follow-up: Changed 5 years ago by ncohen

Also, do you object my removing the code that checks that the installed version of an optional package is indeed the latest? Now that they are automatically updated, that's rather useless.

comment:22 Changed 5 years ago by git

  • Commit changed from 9a011eb0a49a3041ae8574e9ce760a8556516417 to 698ebe958d528ac279601e0acbbf0c16f00350de

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

698ebe9trac #18904: Automatically test optional non-packages (CPLEX, Gurobi, Maple?, ..)

comment:23 Changed 5 years ago by dimpase

how about making this ticket depend on #18908 and include mathematica in the list?

comment:24 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

I rewrote this branch by moving everything to a new function, and added a 'Exception'. In this commit all Maple/Matlab/others are tested for I believe that we should test them.

If you object to that and are interested in other's opinion about it (on top of mine), perhaps you should write to sage-devel in the same thread or in a new one. If you oppose it, add a commit to remove the lines (I am tired of fighting endlessly).

I am waiting for your answer to 21 to simplify a couple of lines in the code.

how about making this ticket depend on #18908 and include mathematica in the list?

Given that Jeroen seems to doubts the wisdom of testing mathematica/maple/..., it seems a bit early to do that. Also, there is nothing inherently wrong in having #18908 add a couple of lines to control.py if this ticket is merged first.

Nathann

Last edited 5 years ago by ncohen (previous) (diff)

comment:25 follow-up: Changed 5 years ago by vbraun

Can we move the whole rummaging-in-external-interfaces part into a separate file? Right now its in the middle of a function thats already too long and has absolutely nothing to do with external interfaces. Though perhaps this ticket is about code golfing the most fugly solution, in that case go ahead ;-)

comment:26 in reply to: ↑ 25 Changed 5 years ago by ncohen

Can we move the whole rummaging-in-external-interfaces part into a separate file?

Right now the function takes 40 lines and I am still waiting for Jeroen's advice on whether we should cut 20 lines out of it. It is not that I do not enjoy moving this code around every time somebody asks, but I don't see the point of creating a new file for a 20 lines function (10 of which is documentation).

As reviewers, please settle on what exactly you want to see changed, and I will get to it.

Nathann

comment:27 follow-up: Changed 5 years ago by dimpase

Well, is there currently a way to mix automatic and manual management of --optional argument to the doctests? If one can do something like make ptest OPTIONAL+=... or 'sage -t --optional_extra=..., with optional_extra appending to --optional` list...

comment:28 in reply to: ↑ 27 Changed 5 years ago by ncohen

Well, is there currently a way to mix automatic and manual management of --optional argument to the doctests?

(tmp|…)~/sage/graphs$ sage -btp --optional=sage,optional,Hey .
...
Using --optional=bliss,cbc,ccache,cplex,...,hey

comment:29 in reply to: ↑ 21 Changed 5 years ago by jdemeyer

Replying to ncohen:

Also, do you object my removing the code that checks that the installed version of an optional package is indeed the latest? Now that they are automatically updated, that's rather useless.

For me, you can remove the code.

comment:30 in reply to: ↑ 20 ; follow-up: Changed 5 years ago by jdemeyer

Replying to ncohen:

We can keep Gurobi and CPLEX for they are only used by Sage if the user explicitly linked them with it.

Can you elaborate what you mean with this?

comment:31 in reply to: ↑ 20 ; follow-up: Changed 5 years ago by jdemeyer

Replying to ncohen:

Almost nothing, give it a try.

I don't have those non-open-source packages installed. Ideally it should be tested by people having the packages installed. And the fact that it takes time than ./sage -b is irrelevant. I often just do ./sage -tp without the ./sage -b when I'm writing doctests.

comment:32 in reply to: ↑ 30 ; follow-up: Changed 5 years ago by ncohen

Can you elaborate what you mean with this?

I mean that CPLEX and Gurobi are not auto-detected from $PATH or $LD_LIBRARY_PATH. The corresponding cython modules are only built if those instructions are followed: http://doc.sagemath.org/html/en/thematic_tutorials/linear_programming.html#using-cplex-or-gurobi-through-sage

Thus, I believe that it is safe to assume that if Sage can use CPLEX/Gurobi then it means that the user wants it to be linked with Sage. The situation is different when you install Sage on a machine that already has mathematica installed, for it is automatically detected even though the user may not want to use mathematica from Sage.

If I understood your objections to having automatic tests with mathematica/maple, they do not apply to CPLEX/Gurobi because of this installation procedure.

Nathann

comment:33 in reply to: ↑ 31 Changed 5 years ago by ncohen

Almost nothing, give it a try.

sage: %time _=DC._default_optional_tags()
CPU times: user 8 ms, sys: 28 ms, total: 36 ms
Wall time: 350 ms

That's how long you lose. Compare it with the time it takes to doctest control.py (which is not a 'hard' file by any standard)

(tmp|…)~/sage/doctest$ time sage -tp control.py
...
sage -tp control.py  2.94s user 0.45s system 74% cpu 4.530 total

And of course this become totally negligible if you test several files, a folder, or the whole of Sage.

I don't have those non-open-source packages installed. Ideally it should be tested by people having the packages installed.

If you are talking about CPLEX and Gurobi, then the time it takes to test whether they are present is *REALLY* negligible compared to the time it takes to test for maple/mathematica/others. It is more or less equivalent to load a module and interpret the 'nameerror' if the (cython) module does not exist. This is done twice, so the cost is very very small.

For the interface.* guys, I personally don't have them on my computer but I consider the time it takes to run the tests to be non-important. If you want to use this argument to claim on sage-devel that they should not be tested, however, you are of course allowed to.

Overall, I think that maple/matlab and others should be included in our tests, for otherwise they will break forever. Paying 0.5s in a ptestlong does not stop me. On the other hand I know that it is useless to argue with you when you do not want to hear something, so I am quite ready to sacrifice it if you but ask. Just decide something and explain the outcome to people on sage-devel who would want this to be tested. I did my part.

Nathann

Last edited 5 years ago by ncohen (previous) (diff)

comment:34 follow-up: Changed 5 years ago by dimpase

the more tests are actually being run, the better. If something breaks for someone who has an unblessed by Sage install of a MMA member, it's OK, a good warning...

comment:35 in reply to: ↑ 34 Changed 5 years ago by ncohen

the more tests are actually being run, the better. If something breaks for someone who has an unblessed by Sage install of a MMA member, it's OK, a good warning...

And that's the only way for us to learn in how many ways such installs can be broken, and act upon it by fixing our interfaces.

Besides, in the case where you really don't have those external softwares installed, checking whether they can be run should be as fast as detecting that the executable cannot be found. So we can also improve that, if we find it too slow.

Nathann

comment:36 in reply to: ↑ 32 Changed 5 years ago by jdemeyer

Replying to ncohen:

If I understood your objections to having automatic tests with mathematica/maple, they do not apply to CPLEX/Gurobi because of this installation procedure.

Agreed.

comment:37 follow-up: Changed 5 years ago by ncohen

I am waiting for your answer to 21 to simplify a couple of lines in the code.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 5 years ago by jdemeyer

Replying to ncohen:

I am waiting for your answer to 21 to simplify a couple of lines in the code.

See 29: yes you can remove that code.

comment:39 in reply to: ↑ 38 Changed 5 years ago by ncohen

See 29: yes you can remove that code.

I received two emails at once, and only noticed the last one. It would be easier to handle your comments if you were giving them all at once. Thanks for your answer though, I'll update it in a second.

Nathann

comment:40 Changed 5 years ago by git

  • Commit changed from 698ebe958d528ac279601e0acbbf0c16f00350de to 44dd24195f94413afd072ef20d03d5491b8b5f2d

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

44dd241trac #18904: Simplify the listing of installed optional new-style packages

comment:41 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

mathematica(1) is just the way I used to detect whether the software is installed. If we need something more reliable, we can have a is_mathematica_available() function somewhere that checks it properly.

Good idea!

I'm still thinking if that would be a sufficient solution, but I think you really should introduce functions like that. And I agree with Volker to move these detecting-optional-tags functions to a new file.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 5 years ago by ncohen

Good idea!

This branch does nothing with mathematica, however.

I'm still thinking if that would be a sufficient solution, but I think you really should introduce functions like that.

I will not do it, that's for sure. You can add a commit, though.

And I agree with Volker to move these detecting-optional-tags functions to a new file.

I would like to understand what you have in mind, first. The function that this branch adds is 40 lines long, and it does not seem justified to create a file for that. Or please explain what you think this file should contain and do. If you want is_X_installed functions, then I expect that those should be in the interface.* files, and not centralized in a doctest code.

Nathann

comment:43 in reply to: ↑ 42 ; follow-up: Changed 5 years ago by jdemeyer

Replying to ncohen:

If you want is_X_installed

It's not really is_X_installed, it's more like is_the_version_of_X_which_is_installed_sufficient_for_the_doctests? So, it is very much related to the doctests.

comment:44 in reply to: ↑ 43 ; follow-up: Changed 5 years ago by ncohen

It's not really is_X_installed, it's more like is_the_version_of_X_which_is_installed_sufficient_for_the_doctests? So, it is very much related to the doctests.

To me it would make more sense to have a version() function in sage/interfaces/mathematica.py that would return the version. This can then be analyzed all you want in the doctest code. By the way, do you have any idea of which version should be kept and which ones should be discarded? Or do you only want to make my job more complicated?

Nathann

comment:45 follow-up: Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

Until somebody can explain why this 40-lines code *must* be moved to a (yet unnamed) new module, or what exactly should be checked if it is not 'magma(1)', ...

comment:46 in reply to: ↑ 44 ; follow-up: Changed 5 years ago by dimpase

Replying to ncohen:

It's not really is_X_installed, it's more like is_the_version_of_X_which_is_installed_sufficient_for_the_doctests? So, it is very much related to the doctests.

To me it would make more sense to have a version() function in sage/interfaces/mathematica.py that would return the version.

I would let .version() return -1000 if there is no program available, instead of a separate function...

comment:47 in reply to: ↑ 46 Changed 5 years ago by ncohen

I would let .version() return -1000 if there is no program available, instead of a separate function...

+1.

comment:48 in reply to: ↑ 45 ; follow-up: Changed 5 years ago by vbraun

Replying to ncohen:

Until somebody can explain why this 40-lines code *must* be moved to a (yet unnamed) new module

https://en.wikipedia.org/wiki/Single_responsibility_principle

Is the doctest controller responsible for third-party interfaces?

E.g. page 34 of http://vitoex.googlecode.com/svn/trunk/Read/Clean%20Code.pdf recommends:

"The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. [...] Functions should not be 100 lines long. Functions should hardly ever be 20 lines long."

comment:49 in reply to: ↑ 48 ; follow-up: Changed 5 years ago by ncohen

Is the doctest controller responsible for third-party interfaces?

Where else do you expect to see the code which decides which third-party interfaces should be tested? Are third-party interfaces responsible for doctesting? Be serious.

E.g. page 34 of http://vitoex.googlecode.com/svn/trunk/Read/Clean%20Code.pdf recommends:

"The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. [...] Functions should not be 100 lines long. Functions should hardly ever be 20 lines long."

That a fortunate coincidence, for my function has around 26 lines of code (the rest is documentation). So I pass this test. Shoud I write myself to sage-devel to ask whether we should enforce a hard line limit of 20 lines per function?

The other good news for me is that I do not see anything about creating a new file for functions of 20< x < 30 lines.

So it's still up for review.

I think that you are officially splitting hairs.

Nathann

comment:50 in reply to: ↑ 49 ; follow-up: Changed 5 years ago by vbraun

Replying to ncohen:

Is the doctest controller responsible for third-party interfaces?

Where else do you expect to see the code which decides which third-party interfaces should be tested?

If you can't figure out an existing place that is responsible then your instinct should be to create a new one.

comment:51 in reply to: ↑ 50 Changed 5 years ago by ncohen

If you can't figure out an existing place that is responsible then your instinct should be to create a new one.

I have no problem figuring this out. There is a place in the doctest code where we define the default list of optional packages that we should test. It is done in the DoctestController class defined in doctest.py, and I don't see a better place to write the code that builds this list.

Nathann

comment:52 follow-up: Changed 5 years ago by vbraun

What is the DoctestController responsible for?

comment:53 in reply to: ↑ 52 ; follow-up: Changed 5 years ago by ncohen

What is the DoctestController responsible for?

Volker, I do not know what you are trying to achieve on this ticket. What I know is that my goal here is to improve Sage's tests, so that less code gets broken.

The way we proceed is through a code review (which IIRC never involved a lot of Zen questioning): if you see something wrong with the code, you say what you think needs to be changed and why, so that we can discuss it. The 20lines limit for functions, I'm ashamed to say, it not in this ticket's scope (discuss it on sage-devel).

I now have really important things that I must see to, like tonight's tomato sauce with heaps of basil.

Nathann

comment:54 Changed 5 years ago by ncohen

  • Description modified (diff)

comment:55 in reply to: ↑ 53 ; follow-up: Changed 5 years ago by vbraun

Replying to ncohen:

if you see something wrong with the code, you say what you think needs to be changed and why, so that we can discuss it.

I said it already, but let me repeat: Move the logic for detecting installed / compatible third-party packages to a separate module. This new module shall only be responsible for that one thing.

comment:56 in reply to: ↑ 55 Changed 5 years ago by ncohen

I said it already, but let me repeat: Move the logic for detecting installed / compatible third-party packages to a separate module. This new module shall only be responsible for that one thing.

It would be a module containing a 40 lines function only. I do not think that it makes much sense, unless you have more code to move there.

Nathann

comment:57 Changed 5 years ago by ncohen

Moreover, please note that the code which builds the list of optional packages was already there. Before I was asked to turn this into an independent function, all that this patch did is add 10 lines of code right after other lines that already did a very similar job. So it is a bit surprising to me that all of a sudden this needs to be changed.

comment:58 Changed 5 years ago by ncohen

I just had an idea: I'm done working on this patch. Whoever wants to see those packages tested can do whatever he likes, for as long as this ticket's original purpose (test Gurobi+CPLEX) works.

Have fun without me.

Nathann

comment:59 follow-up: Changed 5 years ago by dimpase

by now, Mathematica can be added to the list. How about instead of using --optional make another parameter, say --optionalplus. Then most of the issues raised here will be gone.

comment:60 Changed 5 years ago by git

  • Commit changed from 44dd24195f94413afd072ef20d03d5491b8b5f2d to b7213efaf806139f42c9f6b8d3b354c4dbf05047

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

b7213eftrac #18904: Merged with 6.9.beta6

comment:61 in reply to: ↑ 59 Changed 5 years ago by jj

Replying to dimpase:

by now, Mathematica can be added to the list. How about instead of using --optional make another parameter, say --optionalplus. Then most of the issues raised here will be gone.

+1 (--external? or --thirdparty?)

comment:62 Changed 5 years ago by kcrisman

optional=internet seems useful too, fwiw, see https://groups.google.com/forum/#!topic/sage-devel/qP_3jjVqb5U

comment:63 Changed 5 years ago by klee

In #20182, I started a rewrite of this ticket. Check and modify it!

comment:64 Changed 5 years ago by kcrisman

So is this now a wontfix or duplicate?

comment:65 Changed 5 years ago by jdemeyer

Let's wait until #20182 is merged.

comment:66 Changed 5 years ago by klee

  • Milestone changed from sage-6.8 to sage-duplicate/invalid/wontfix
  • Reviewers set to Kwankyu Lee
  • Status changed from needs_review to positive_review

comment:67 Changed 4 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.