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

[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)

[ncohen]

New commits:

​ef53635

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

[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).

[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

[dimpase]

say NO to blind copy/paste! Do

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

[ncohen]

say NO to blind copy/paste! Do

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

[ncohen]

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

[dimpase]

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

[ncohen]

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

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

[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?

[git]

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

​091ed73

trac #18904: More optional softwares

[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

[git]

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

​9a011eb

trac #18904: More optional softwares

[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)

[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

[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 :-)

[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

[jdemeyer]

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.

[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.

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

I will add a commit.

3. 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

[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.

[git]

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

​698ebe9

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

[dimpase]

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

[ncohen]

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

[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 ;-)

[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

[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...

[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

[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.

[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?

[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.

[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

[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

[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...

[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

[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.

[ncohen]

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

[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.

[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

[git]

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

​44dd241

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

[jdemeyer]

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.

[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

[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.

[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

[ncohen]

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)', ...

[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...

[ncohen]

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

+1.

[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."

[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

[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.

[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

[vbraun]

What is the DoctestController responsible for?

[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

[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.

[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

[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.

[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

[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.

[git]

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

​b7213ef

trac #18904: Merged with 6.9.beta6

[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?)

[kcrisman]

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

[klee]

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

[kcrisman]

So is this now a wontfix or duplicate?

[jdemeyer]

Let's wait until #20182 is merged.