Opened 9 years ago

Last modified 7 years ago

#12415 closed enhancement

Update doctesting framework — at Version 28

Reported by: robertwb Owned by: mvngu
Priority: major Milestone: sage-5.9
Component: doctest framework Keywords:
Cc: kini, ohanar, jhpalmieri Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13147,#13146, #13145, #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384 Stopgaps:

Description (last modified by roed)

There are several improvements that would be good to make, including but not limited to:

  1. Forking rather than starting up a new Sage each time.
  2. Customizable comparison rather than mutating doctests into new doctests (e.g. followup to #10952).
  3. Eliminate need to write temporary files for every doctested file.
  4. Constant headaches caused by the difference in doctesting library vs. non-library files.
  5. The ability to drop into a debugger if a doctest raises an exception.

Robert's code is at

http://code.google.com/p/sagemath-timer/

and does most of the above (plus more which has been moved to followup tickets #12720 and #12722)

Apply attachment:12415_stderr_vs_51b5.patch, attachment:12415_doctest_fixes.patch, attachment:12415_framework.patch

Apply attachment:12415_script.patch to the scripts repo

Apply attachment:12415_sagenb_fixes.patch to the sagenb repo (fixes doctest errors)

Apply attachment:12415_spkg_bin_sage.patch to the root repo

Change History (28)

comment:1 Changed 9 years ago by robertwb

  • Description modified (diff)

comment:2 Changed 9 years ago by robertwb

  • Description modified (diff)

comment:3 Changed 9 years ago by roed

  • Description modified (diff)

I'm working on this. E-mail me if you want to collaborate.

comment:4 Changed 9 years ago by roed

  • Description modified (diff)

comment:5 Changed 9 years ago by roed

  • Description modified (diff)

comment:6 Changed 9 years ago by roed

  • Dependencies set to #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384

comment:7 Changed 9 years ago by roed

  • Dependencies changed from #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384 to #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384

comment:8 Changed 8 years ago by kini

  • Cc kini added

comment:9 Changed 8 years ago by robertwb

12415_2.patch seems to contain a lot of tempfile changes, could you split out the doctest changes into a separate patch (or are these affecting doctests?)

comment:10 Changed 8 years ago by ohanar

  • Cc ohanar added

comment:11 Changed 8 years ago by ohanar

apparently even I forget my trac username is not what it should be

comment:12 Changed 8 years ago by roed

  • Dependencies changed from #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384 to #13145, #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384

comment:13 Changed 8 years ago by roed

  • Dependencies changed from #13145, #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384 to #13146, #13145, #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384

comment:14 Changed 8 years ago by roed

  • Dependencies changed from #13146, #13145, #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384 to #13147,#13146, #13145, #12723, #12392, #12393, #12395, #12396, #12397, #12381, #12382, #12383, #12384

comment:15 Changed 8 years ago by roed

I've moved the patches dealing with temporary files to #13147.

comment:16 Changed 8 years ago by jhpalmieri

  • Cc jhpalmieri added

comment:17 Changed 8 years ago by roed

  • Description modified (diff)

comment:18 Changed 8 years ago by roed

  • Description modified (diff)

comment:19 Changed 8 years ago by roed

  • Status changed from new to needs_review

Ready for review!

There are still a few known issues:

  • valgrinding under sage -t doesn't work. I haven't used valgrind much and don't know how to resolve this.
  • The fix at #13147 probably needs work: I'm getting a failure in sage.rings.polynomial.multi_polynomial_libsingular to do with garbage collection. I'm working on it.
  • I'm in the process of updating the developer guide.

But I wanted to get some other people involved in reviewing.

comment:20 Changed 8 years ago by jhpalmieri

This may be a silly question, but how do I use the new framework? Don't we need changes to the scripts repo, too? (If I just run sage -t FILES, I don't see the various messages, like Doctesting ..., that should be printed by run_doctests are not printed, so sage -t is not yet using the new framework.)

One of the patches also didn't apply cleanly to 5.1.beta5:

hg qimport -P ~/Downloads/12415_stderr.patch 
adding 12415_stderr.patch to series file
applying 12415_stderr.patch
patching file sage/lfunctions/lcalc.py
Hunk #1 FAILED at 226
1 out of 2 hunks FAILED -- saving rejects to file sage/lfunctions/lcalc.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 12415_stderr.patch

comment:21 Changed 8 years ago by roed

Did you apply the new dependencies (#13145, #13146, #13147)? There are some more patches to other repositories: I'll update the ticket description.

comment:22 Changed 8 years ago by roed

  • Description modified (diff)

comment:23 Changed 8 years ago by roed

The description is updated: let me know if you have any more problems. I think you need to apply 12415_stderr_vs_51b5.patch instead of 12415_stderr.patch.

comment:24 Changed 8 years ago by jhpalmieri

Regarding 12415_spkg_bin_sage.patch, the file in question should be part of the root repo.

comment:25 Changed 8 years ago by jhpalmieri

One regression: with the old set-up, running sage -tp FILES (with no argument for tp) would choose the number of threads automatically. With the new set-up, this command gives an error, because FILES is not an integer. Therefore I think that, for example, make ptestlong will fail. (The file sage-runtests says that the default value for nthreads is 1, but I don't think optparse is dealing with the missing argument properly here. Also, passing the argument 0 causes doctests to never happen: maybe it's using 0 threads instead of minimum(8, cpu_count()).

comment:26 Changed 8 years ago by jhpalmieri

I would suggest the following two changes: for the scripts repo:

  • sage-runtests

    diff --git a/sage-runtests b/sage-runtests
    a b import optparse, os, sys 
    55if __name__ == "__main__":
    66    parser = optparse.OptionParser()
    77
    8     parser.add_option("-p", "--nthreads", type=int, default=1, metavar="N", help="tests in parallel using N threads with 0 interpreted as minimum(8, cpu_count())")
     8    parser.add_option("-p", "--nthreads", type="string", default=1, metavar="N", help="tests in parallel using N threads with 0 interpreted as minimum(8, cpu_count())")
    99    parser.add_option("--serial", action="store_true", default=False, help="run tests in a single process in series")
    1010    parser.add_option("--timeout", type=int, default=-1, help="timeout (in seconds) for doctesting one file")
    1111    parser.add_option("-a", "--all", action="store_true", default=False, help="test all files in the Sage library")
    if __name__ == "__main__": 
    4949    parser.add_option("--stats_path", "--stats-path", default=os.path.join(os.path.expanduser("~/.sage/timings2.json")), \
    5050                          help="path to a json dictionary for the latest run storing a timing for each file")
    5151
     52    options, args = parser.parse_args()
     53    try:
     54        options.nthreads = int(options.nthreads)
     55    except ValueError:
     56        args.insert(0, options.nthreads.strip())
     57        options.nthreads = 0
     58
     59    if options.nthreads == 0:
     60        try:
     61            options.nthreads = int(os.environ['SAGE_NUM_THREADS_PARALLEL'])
     62        except KeyError:
     63            options.nthreads = 1
     64
    5265    from sage.doctest.control import DocTestController
    53     DC = DocTestController(*parser.parse_args())
     66    DC = DocTestController(options, args)
    5467    err = DC.run()
    5568    sys.exit(err)

and for the Sage library:

  • sage/doctest/control.py

    diff --git a/sage/doctest/control.py b/sage/doctest/control.py
    a b class DocTestController(SageObject): 
    433433            else:
    434434                nother += 1
    435435        if nfiles + ndbsources + nother:
    436             self.log("Doctesting %s."%(", ".join((["%s file%s"%(nfiles, "s" if nfiles > 1 else "")] if nfiles else []) +
    437                                                  (["%s other sources"%nother] if nother else []))))
     436            self.log("Doctesting %s"%(", ".join((["%s file%s"%(nfiles, "s" if nfiles > 1 else "")] if nfiles else []) +
     437                                                 (["%s other sources"%nother] if nother else [])))
     438                + " using %s threads."%self.options.nthreads if self.options.nthreads > 1 else ".")
    438439            self.reporter = DocTestReporter(self)
    439440            self.dispatcher = DocTestDispatcher(self)
    440441            try:

Or something like that. I guess you should also print the number of threads when you print "Doctesting entire Sage library.", etc.

comment:27 Changed 8 years ago by roed

I've updated the option parsing to allow -p to take no options (though I used a callback so that the -p doesn't have to be last). I changed 12415_spkg_bin_sage.patch to be an actual patch against the root repository. I also made a small change to the debug functionality (printing the doctest that failed before dropping into the debugger).

Since I'm modifying four repositories, I'll just update the patches rather than creating lots of referee patches (unless the referee objects!).

comment:28 Changed 8 years ago by roed

  • Description modified (diff)
Note: See TracTickets for help on using tickets.