Opened 6 years ago
Closed 5 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, GitHub, GitLab) | Commit: | b7213efaf806139f42c9f6b8d3b354c4dbf05047 |
Dependencies: | Stopgaps: |
Description (last modified by )
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 6 years ago by
- Branch set to public/18904
- Commit set to ef53635ae85dbc5afdfd2128c7238bd404321d97
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 6 years ago by
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 6 years ago by
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: ↓ 5 Changed 6 years ago by
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 6 years ago by
say NO to blind copy/paste! Do
Proposing a commit is the trendy way to say *I care*.
comment:6 follow-up: ↓ 7 Changed 6 years ago by
Er... I could have done better: "Nothing says 'I care' like 'here is a commit'".
comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 6 years ago by
But I'd need write access to your brain then...
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
- Commit changed from ef53635ae85dbc5afdfd2128c7238bd404321d97 to 091ed73139ddc886260854e59499eb0e5eca9e91
Branch pushed to git repo; I updated commit sha1. New commits:
091ed73 | trac #18904: More optional softwares
|
comment:11 follow-up: ↓ 13 Changed 6 years ago by
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 6 years ago by
- Commit changed from 091ed73139ddc886260854e59499eb0e5eca9e91 to 9a011eb0a49a3041ae8574e9ce760a8556516417
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9a011eb | trac #18904: More optional softwares
|
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 6 years ago by
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: ↓ 15 Changed 6 years ago by
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: ↓ 16 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
- Description modified (diff)
comment:18 Changed 6 years ago by
- Description modified (diff)
comment:19 follow-up: ↓ 20 Changed 6 years ago by
- 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:
- Never use
except:
. Useexcept Exception:
or some other specific exception class. - Code structure: can we please move all this detect-optional-packages code to a separate function?
- 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: ↓ 30 ↓ 31 Changed 6 years ago by
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.
- Never use
except:
. Useexcept 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.
- Code structure: can we please move all this detect-optional-packages code to a separate function?
I will add a commit.
- 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: ↓ 29 Changed 6 years ago by
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 6 years ago by
- Commit changed from 9a011eb0a49a3041ae8574e9ce760a8556516417 to 698ebe958d528ac279601e0acbbf0c16f00350de
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?, ..)
|
comment:23 Changed 6 years ago by
how about making this ticket depend on #18908 and include mathematica in the list?
comment:24 Changed 6 years ago by
- 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
comment:25 follow-up: ↓ 26 Changed 6 years ago by
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 6 years ago by
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: ↓ 28 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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: ↓ 32 Changed 6 years ago by
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: ↓ 33 Changed 6 years ago by
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: ↓ 36 Changed 6 years ago by
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 6 years ago by
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
comment:34 follow-up: ↓ 35 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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: ↓ 38 Changed 6 years ago by
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: ↓ 39 Changed 6 years ago by
comment:39 in reply to: ↑ 38 Changed 6 years ago by
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 6 years ago by
- Commit changed from 698ebe958d528ac279601e0acbbf0c16f00350de to 44dd24195f94413afd072ef20d03d5491b8b5f2d
Branch pushed to git repo; I updated commit sha1. New commits:
44dd241 | trac #18904: Simplify the listing of installed optional new-style packages
|
comment:41 follow-up: ↓ 42 Changed 6 years ago by
- 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 ais_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: ↓ 43 Changed 6 years ago by
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: ↓ 44 Changed 6 years ago by
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: ↓ 46 Changed 6 years ago by
It's not really
is_X_installed
, it's more likeis_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: ↓ 48 Changed 6 years ago by
- 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: ↓ 47 Changed 6 years ago by
Replying to ncohen:
It's not really
is_X_installed
, it's more likeis_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 6 years ago by
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: ↓ 49 Changed 6 years ago by
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: ↓ 50 Changed 6 years ago by
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: ↓ 51 Changed 6 years ago by
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 6 years ago by
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: ↓ 53 Changed 6 years ago by
What is the DoctestController
responsible for?
comment:53 in reply to: ↑ 52 ; follow-up: ↓ 55 Changed 6 years ago by
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 6 years ago by
- Description modified (diff)
comment:55 in reply to: ↑ 53 ; follow-up: ↓ 56 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
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: ↓ 61 Changed 6 years ago by
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
- Commit changed from 44dd24195f94413afd072ef20d03d5491b8b5f2d to b7213efaf806139f42c9f6b8d3b354c4dbf05047
Branch pushed to git repo; I updated commit sha1. New commits:
b7213ef | trac #18904: Merged with 6.9.beta6
|
comment:61 in reply to: ↑ 59 Changed 5 years ago by
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
optional=internet
seems useful too, fwiw, see https://groups.google.com/forum/#!topic/sage-devel/qP_3jjVqb5U
comment:63 Changed 5 years ago by
In #20182, I started a rewrite of this ticket. Check and modify it!
comment:64 Changed 5 years ago by
So is this now a wontfix or duplicate?
comment:65 Changed 5 years ago by
Let's wait until #20182 is merged.
comment:66 Changed 5 years ago by
- 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 5 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
trac #18904: Automatically test optional non-packages (CPLEX, Gurobi, Maple?, ..)