Test all installed optional packages by default

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

With this branch, running "sage -t" will automatically involve testing all installed up-to-date new-style optional Sage packages.

Depends on #18456 Depends on #18124 Depends on #18559 Depends on #18563 Depends on #18579 Depends on #18581

CC: @vbraun @jdemeyer @sagetrac-tmonteil @seblabbe

Component: doctest framework

Author: Nathann Cohen, Jeroen Demeyer

Branch/Commit: 82f3eec

Reviewer: Jeroen Demeyer, Karl-Dieter Crisman, John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/18558

jdemeyer commented 9 years ago

Changed dependencies from #18456, #18124, #18559 to #18456, #18124, #18559, #18563

jdemeyer commented 9 years ago

comment:47

Replying to @nathanncohen:

What is it exactly that you want to do?

See #18563.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

5954ba9 trac #18558: 'optional' keyword

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 1ffc3c1 to 5954ba9

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:49

Done.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:50

See #18563.

Just change their type? Okay, no problem. When do you think it will be ready for review?

jdemeyer commented 9 years ago

comment:51

Replying to @nathanncohen:

See #18563.

Just change their type? Okay, no problem. When do you think it will be ready for review?

Well, I need to actually install all new-style optional packages and test with #18558.

jdemeyer commented 9 years ago

comment:52

Should

if 'installed' in options.optional:

actually be

if 'optional' in options.optional:

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:53

Well, I need to actually install all new-style optional packages and test with #18558.

Okay, shouldn't take long.

You can get the list of those packages with sage -optional --local --no-version

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

374a17f trac #18558: 'optional' keyword

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 5954ba9 to 374a17f

jdemeyer commented 9 years ago

comment:57

Replying to @nathanncohen:

Well, I need to actually install all new-style optional packages and test with #18558.

Okay, shouldn't take long.

I guess you never tried to install all optional packages...

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:58

I guess you never tried to install all optional packages...

Ahahahah. Well, surely none is worse than atlas? :-P

I was thinking 'one day at most'. But I expect that it can take time indeed ^^;

Nathann

jdemeyer commented 9 years ago

comment:59

For information, this is a list of all optional tags with count (assuming #18124 and #18559):

    302 
     59 4ti2
    622 arb
    114 axiom
      7 benzene
     31 bliss
     10 buckygen
    209 cbc
     25 chevie
     33 CHomP
    383 coxeter3
    185 CPLEX
    185 cryptominisat
      1 CryptoMiniSat
      4 cunningham
     11 CVXOPT
     24 database_cremona_ellcurve
    112 database_gap
     14 database_jones_numfield
      2 database_kohel
      3 database_odlyzko_zeta
      8 database_pari
     28 database_stein_watkins
     13 database_symbolic_data
      1 debug
      3 dot2tex
     31 dot2tex graphviz
     43 FES
      8 ffmpeg
     65 fricas
     51 frobby
    115 gambit
     93 gap3
      2 gap3chevie
     67 gap_packages
      5 gdb
    142 giac
      2 ginv
      1 gnuplot
     19 gperftools
    201 Gurobi
      6 GUROBI
      1 Hmisc R package
     72 ImageMagick
    272 internet
      2 java
      2 java internet
    109 kash
      8 latex
     29 latte_int
      1 libjpeg
     96 lie
      9 long time
     30 lrs
      2 M2
     15 macaulay2
    660 magma
    151 maple
      2 Maple
    119 mathematica
     59 matlab
      4 mcqd
     10 modular_decomposition
     79 mupad
      2 MuPAD
     20 nauty
    157 Nonexistent_LP_solver
     61 octave
     69 phc
     35 plantri
     15 polymake
      5 polytopes_db_4d
    180 qepcad
      1 random
      2 requires maple
     11 rgraphics
      2 runsnake
     98 scilab
      5 sloane_database
     16 souvigner
      3 surf
      4 tides
      8 time
     40 TOPCOM
      3 webbrowser

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:60

My dear old Nonexistent_LP_solver.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:61

It seems that you missed mupad-Combinat

jdemeyer commented 9 years ago

comment:62

Replying to @nathanncohen:

It seems that you missed mupad-Combinat

No, I did not because # optional - mupad-Combinat is really parsed as # optional mupad # Combinat.

jdemeyer commented 9 years ago

Changed branch from u/ncohen/18558 to u/jdemeyer/18558

jdemeyer commented 9 years ago

Changed commit from 374a17f to 732f2f8

jdemeyer commented 9 years ago

New commits:

732f2f8 Show optional tags in the doctest log

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 732f2f8 to 59af698

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

59af698 Fix passing optional tags to gdb/valgrind doctesting

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:66

Jeroen, please move this commit to another ticket. And the previous one as well, if you think that the two are tied together.

jdemeyer commented 9 years ago

comment:67

Replying to @nathanncohen:

Jeroen, please move this commit to another ticket.

Why? If I do that, there will surely be merge conflicts.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:68

Why? If I do that, there will surely be merge conflicts.

Why? Just make your other ticket depend on this one.

jdemeyer commented 9 years ago

comment:69

Replying to @nathanncohen:

Why? Just make your other ticket depend on this one.

That's the wrong way. This ticket would need the bugfix of the "other ticket".

Please, these two tickets belong together, let's not make things more complicated.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:70

That's the wrong way. This ticket would need the bugfix of the "other ticket".

Please, these two tickets belong together, let's not make things more complicated.

If you cared to explain what you do and why, perhaps we could talk about it. You have been pushing commits here without a single word.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:71

And I really feel that you behave a bit boldly here. As the ticket's author I have to answer any request made by a reviewer if I want to get my ticket reviewed. At the same time, you just push things without asking for my opinion, and as a result I am forced to agree with whatever you do here if I want my code to pass.

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:72

Let me also mention that you consider anything related to this ticket as a dependency of it, which you thus add as a dependency of this ticket. Forcing me to review them. This, without reviewing this ticket's main dependency: #18456.

jdemeyer commented 9 years ago

comment:73

Replying to @nathanncohen:

If you cared to explain what you do and why, perhaps we could talk about it. You have been pushing commits here without a single word.

Well, you can start by reading the commit message or looking at the commits. I felt that the message Fix passing optional tags to gdb/valgrind doctesting would make it clear that your patch broke doctesting under gdb or valgrind. I can explain later if you want, I'll answer your other complaints first.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:74

Well, you can start by reading the commit message or looking at the commits. I felt that the message Fix passing optional tags to gdb/valgrind doctesting would make it clear that your patch broke doctesting under gdb or valgrind.

I fail to see how, so please illustrate it.

Nathann

kcrisman commented 9 years ago

comment:75

Well, you can start by reading the commit message or looking at the commits. I felt that the message Fix passing optional tags to gdb/valgrind doctesting would make it clear that your patch broke doctesting under gdb or valgrind.

I fail to see how, so please illustrate it.

Valgrind can have relatively system-dependent output, or so I've been led to believe.

Sorry for butting in, I was just wondering if this all had a way to check whether someone had the most recent version of an optional package. Especially for some that are quite independent of Sage it could be possible someone didn't notice that the optional package had been updated and then got an error that's really because of their version, not an actual failure. If this is answered somewhere above I apologize - there are a lot of comments but I didn't see it, nor when searching the page for relevant terms like 'version'.

jdemeyer commented 9 years ago

comment:76

Replying to @nathanncohen:

At the same time, you just push things without asking for my opinion

That's true, but you can still give your opinion after seeing the commit. I am currently testing this ticket. It's easier for me to fix bugs right away instead of first explaining the problem on the ticket and then waiting for you to fix it before I can continue testing.

and as a result I am forced to agree with whatever you do here if I want my code to pass.

You're not forced to agree with anything. I you don't agree, you tell me or you change it. No problem.

you consider anything related to this ticket as a dependency of it

About #18124 and #18563: those are needed because otherwise all doctests would not pass.
About #18559: I added that as a dependency only after it got positive_review (and you're the author so I assume you agree with that ticket), so it won't slow down this ticket.

This, without reviewing this ticket's main dependency: #18456.

It seems that there are already other reviewers on that ticket. In any case, it's not because I help you with this ticket, that I should be forced to review its dependencies.

jdemeyer commented 9 years ago

Changed author from Nathann Cohen to Nathann Cohen, Jeroen Demeyer

jdemeyer commented 9 years ago

comment:77

Replying to @kcrisman:

Especially for some that are quite independent of Sage it could be possible someone didn't notice that the optional package had been updated and then got an error that's really because of their version, not an actual failure.

I say it would be an actual failure. If upgrading the optional package would make the failure go away, then the user should upgrade to the newest version.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:78

Sorry for butting in, I was just wondering if this all had a way to check whether someone had the most recent version of an optional package.

You should ask about this in #18456 (needs review) which is more related. If you apply and type sage -optional you will see on your screen the installed version of each package, and the latest 'online' version. There is no tool to tell you which ones should be upgraded, but that can be added rather easily.

There is something to be aware of, hwoever: you may be surprised to find that the 'online' version of a package is older than the one installed on your computer. This is because the distant list of packages can contain the same package as an upstream (new-style) package and as an old spkg. When the mirrors will be updated, that will be reliable again.

Nathann

jdemeyer commented 9 years ago

comment:79

Replying to @jdemeyer:
732f2f8	`Show optional tags in the doctest log`

This is just useful information for logging. When running the tests, it shows the optional tags in the log:

Running doctests with ID 2015-06-01-16-36-29-dcf185f8.
Git branch: HEAD
Optional tags: arb,autotools,benzene,bliss,buckygen,cbc,compilerwrapper,cryptominisat,d3js,database_cremona_ellcurve,database_gap,database_odlyzko_zeta,database_pari,database_stein_watkins,database_stein_watkins_mini,database_symbolic_data,dot2tex,gambit,gcc,gdb,git_trac,libogg,libtheora,lidia,mcqd,modular_decomposition,mpir,nauty,normaliz,nose,openssl,plantri,python2,sage,sage_mode,termcap,threejs,tides
Doctesting 2 files.

jdemeyer commented 9 years ago

comment:80

Replying to @sagetrac-git:
59af698	`Fix passing optional tags to gdb/valgrind doctesting`

This is needed because ./sage -t --gdb --optional=sage,otherpackage has always been broken. I know you didn't introduce this bug, but you made it visible because now there are more optional tags enabled by default.

I also added some cosmetic changes, like making ./sage -t --optional=all,something,else equivalent to ./sage -t --optional=all and deprecating ./sage -t --optional=true.

jdemeyer commented 9 years ago

comment:81

I finished testing this (and its dependencies) and my Linux laptop with all new-style optional packages installed and all tests pass. I am also doing the same on OS X 10.10, but that will take some more time.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:82

That's true, but you can still give your opinion after seeing the commit. I am currently testing this ticket. It's easier for me to fix bugs right away instead of first explaining the problem on the ticket and then waiting for you to fix it before I can continue testing.

Of course. It would be more pleasant (for me at last) if you could say exactly what you are doing when you push commits (especially when the branch's name is not public/...).

You're not forced to agree with anything. I you don't agree, you tell me or you change it. No problem.

I would like to know what exactly you fix. I would like to understand why you consider it as a dependency of this ticket.

It seems that there are already other reviewers on that ticket.

I have many tickets on which many people come to make me change a lot of things, but nobody ends up taking the time to review it completely.

In any case, it's not because I help you with this ticket, that I should be forced to review its dependencies.

You are under no obligation to, it is true. I am also free to find that behaviour unpleasant.

Nathann

jdemeyer commented 9 years ago

comment:83

Replying to @nathanncohen:

Of course. It would be more pleasant (for me at last) if you could say exactly what you are doing when you push commits (especially when the branch's name is not public/...).

Maybe you're just a bit too impatient. My first priority was to have a functioning branch without doctest failures. Now that this is the case on at least one system, we can discuss.

jdemeyer commented 9 years ago

comment:84

Replying to @nathanncohen:

I would like to know what exactly you fix. I would like to understand why you consider it as a dependency of this ticket.

Is that now cleared up by the comments above?

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:85

Is that now cleared up by the comments above?

Granted. Do you notice, however, that you ended up forcing this ticket to only be merged if all tests pass when all optional packages are installed? I did not agree with this 'dependency' at first, and neither did others.

No doubt that the result is better now. But I felt forced, at every step, to follow with what you were doing if I wanted my ticket reviewed.

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:86

Okay, I reviewed this latest commit of yours.

kcrisman commented 9 years ago

comment:87

Especially for some that are quite independent of Sage it could be possible someone didn't notice that the optional package had been updated and then got an error that's really because of their version, not an actual failure.

I say it would be an actual failure. If upgrading the optional package would make the failure go away, then the user should upgrade to the newest version.

Right, except would the user actually know that the solution to the failure was upgrading? In principle we don't want a sage-release or other email to come in unless there is an actual failure. If this gives a clear message to that effect (upgrade package so-and-so) then it's no problem.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:88

Right, except would the user actually know that the solution to the failure was upgrading?

This is an interesting discussion, but this is not the place. There is still work going on here. I will answer by email.

Nathann

kcrisman commented 9 years ago

comment:89

Right, except would the user actually know that the solution to the failure was upgrading?

This is an interesting discussion, but this is not the place.

I disagree, because this means that the (old) default behavior of not getting doctest failures will be replaced (possibly) by lots of perhaps-very-confusing doctest errors, with this branch.

But I don't have any specific ideas on how to solve the problem, just wondering how it is/will be dealt with.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:90

I disagree, because this means that the (old) default behavior of not getting doctest failures will be replaced (possibly) by lots of perhaps-very-confusing doctest errors, with this branch.

Okay. So let us have this conversation here. For a start, do we agree that we are talking about users running doctests with optional packages installed? If so, we cannot call them beginners.

Since you insist on having this conversation here, I also wonder why you think that it is doctest-related. If you are thinking of users running an old version of a package, which would thus break Sage code, then clearly it is a problem of a much wider scope than doctest testing, isn't it ?

In particular, I do not believe that there is anything related to that which can be dealt with at the level of doctest. What you request is a way to detect that the installed package is too old to be used for this or that feature, and such a feature cannot be implemented without knowing each package individually.

If such a thing should be implemented, then we would need the is_package_installed function to be able to deal with version numbers, and thus be able to check whether the current version is sufficiently new.

But I don't have any specific ideas on how to solve the problem, just wondering how it is/will be dealt with.

It is a question for sage-devel, unless you have strong reasons to believe that it should be addressed by this ticket.

Nathann

kcrisman commented 9 years ago

comment:91

I disagree, because this means that the (old) default behavior of not getting doctest failures will be replaced (possibly) by lots of perhaps-very-confusing doctest errors, with this branch.

Okay. So let us have this conversation here. For a start, do we agree that we are talking about users running doctests with optional packages installed? If so, we cannot call them beginners.

Disagree already. There are lots of places it warns people to run doctests to check that Sage works, and likewise there are a number of optional packages that people "just need". In R certainly optional packages are pretty much the whole point of using the system - not so much in Sage, but for instance I can very easily imagine someone using #18536 installing gambit brainlessly for the sole purpose of solving non-cooperative games.

Since you insist on having this conversation here, I also wonder why you think that it is doctest-related. If you are thinking of users running an old version of a package, which would thus break Sage code, then clearly it is a problem of a much wider scope than doctest testing, isn't it ?

?? Not at all; using an old version of a package might not at all break any existing Sage code. It might only change something like (again using the game example) the example decimal representation of a specific answer, for instance, and one that would only be noted if one were using that optional package.

In particular, I do not believe that there is anything related to that which can be dealt with at the level of doctest. What you request is a way to detect that the installed package is too old to be used for this or that feature, and such a feature cannot be implemented without knowing each package individually.

Maybe... I think instead I am imagining that Sage could detect whether the latest version (for a given version of Sage) was installed. R does this all the time. But maybe that is too hard now because we don't really have a concept of "version of Sage required for this version of optional package X" or vice versa.

If such a thing should be implemented, then we would need the is_package_installed function to be able to deal with version numbers, and thus be able to check whether the current version is sufficiently new.

Yes, or something analogous to that. After all, aren't the current package numbers already in build/pkgs/pkg_name/version or something? It could just check that the packages which are installed match. (Not necessarily for old-style, but that is presumably yet another snag.)

It is a question for sage-devel, unless you have strong reasons to believe that it should be addressed by this ticket.

The only reason I think it belongs on this ticket is because this ticket would expose the problem. I would say it would do so rather dramatically except I don't have any evidence for it. But again, here is what I anticipate:

User occasionally runs doctests because is told it's a good idea to make sure their Sage works.
User updates Sage but not some package they use, because that wasn't announced somewhere they saw it (git logs are not announcements)
User runs doctests and gets mysterious failures that seem to be related to updating Sage, but are actually related to not updating package.
User sends email to sage-devel or sage-release or sage-support or ask.sagemath or whatever.
Someone whose time is better spent on real bugs has to figure out what the heck happened.

Without this ticket, that is much less likely to happen, since the person would have to be running optional doctests - and I would agree that such a person is not so likely to be a beginner. But that's not what would happen here.

Anyway, if two people (e.g. you and Jeroen) feel like it's not worth dealing with here (and it probably would be a big change) I won't complain further, except to say "I told you so" if it happens and to be very happy if it turns out it doesn't. I did want to make sure it got raised, though.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 9 years ago

comment:92

Maybe... I think instead I am imagining that Sage could detect whether the latest version (for a given version of Sage) was installed. R does this all the time.

Detecting whether "the local version of a package" is the latest available can be done easily from the output of sage-list-package (see #18456).

If what you want is to automatically detect, in doctests or when running Sage, that some errors are caused by an old version of a package, then it is a more complicated problem and you could help make this conversation more constructive by describing a specific behaviour that you would like. It is my belief that it is also, in this case, unrelated to this ticket.

Yes, or something analogous to that. After all, aren't the current package numbers already in build/pkgs/pkg_name/version or something? It could just check that the packages which are installed match. (Not necessarily for old-style, but that is presumably yet another snag.)

We can know the version of any installed packaged, be it new or old. See output of sage -installed in #18456. Detecting this is not a problem.

The only reason I think it belongs on this ticket is because this ticket would expose the problem.

This ticket cannot fix all the pre-existing bugs that it exposes. If everybody comes and say "this bug existed before, you must fix it or your code will not pass" I will give up very quickly.

User occasionally runs doctests because is told it's a good idea to make sure their Sage works.

Jeroen made sure that all tests will pass. So that's not a problem.

User updates Sage but not some package they use, because that wasn't announced somewhere they saw it (git logs are not announcements)

For new-style packages this is already done automatically.

User runs doctests and gets mysterious failures that seem to be related to updating Sage, but are actually related to not updating package.

Again, only happens for old-style packages. Convert them to new-style to avoid this problem.

Without this ticket, that is much less likely to happen, since the person would have to be running optional doctests - and I would agree that such a person is not so likely to be a beginner. But that's not what would happen here.

Anyway, if two people (e.g. you and Jeroen) feel like it's not worth dealing with here (and it probably would be a big change) I won't complain further, except to say "I told you so" if it happens and to be very happy if it turns out it doesn't. I did want to make sure it got raised, though.

Thank you for your contribution. As you will see from looking at the logs, Jeroen installed all optional doctests to make sure that all tests will pass.

Nathann

sagemath / sage

Test all installed optional packages by default #18558