sage -fixdoctests: Handle directory names, call sage -t only once

[mkoeppe]

We change the handling of file name arguments in sage -fixdoctests: Instead of calling sage -t on files one by one, we just pass them to sage -t as a whole and then handle its output for all files. This is much faster because of the startup overhead of sage -t, and also adds the handling of directory names.

Cherry-picked from

• Modularization of sagelib: Distributions sagemath-{brial,combinat,eclib,flint,gap,giac,glpk,graphs,groups,homfly,lcalc,libbraiding,libecm,linbox,modules,mpmath,ntl,pari,plot,polyhedra,schemes,singular,standard-no-symbolics,symbolics} #35095
  Part of
• Meta-ticket: Modularize sagelib into separate distributions (pip-installable packages) sagemath-... for Sage 10.x #29705

📝 Checklist

• The title is concise, informative, and self-explanatory.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

[kwankyu]

I checked that handling directory works well againstsage -fixdoctests --long src/sage/schemes/curves. With this tool, we could fix all doctests all at once :-)

On the other hand, comparing the docs after fixing doctests with the docs before fixing, I have one concern. While we are here, let me explain.

Now that we can replace many individual doctest line tags with block-scoped tags, it seems that needs tags are rather informative than visually cluttering. Hence we don't need to make efforts to hide needs tags anymore. See for instance

So I suggest the following.

1. Adjust tabs so as to make more portion of an individual doctest line tag be really seen. (it seems that there's not much room to adjust tabs, without incurring visual clutter)
2. Use a block-scoped tag for three (or more) consecutive lines of doctest tags.

You may disagree with me. Certainly accepting my suggestions or not is not considered for review of this PR.

[mkoeppe]

Now that we can replace many individual doctest line tags with block-scoped tags, it seems that needs tags are rather informative than visually cluttering. Hence we don't need to make efforts to hide needs tags anymore.

I think making this change would be a premature step. I, of course, also consider the tags informative, but the burden for the few developers who already find the new tags informative -- having to scroll horizontally to get the full information -- is rather low.

[kwankyu]

OK. I agree.

[kwankyu]

I get

$ sage -fixdoctests src/sage/rings/function_field/constructor.py output
Running "sage -t -p src/sage/rings/function_field/constructor.py output"
ERROR: file or directory not found: output

sage-runtests: Skipping 'output' because it does not have one of the recognized file name extensions
too few successful tests, not using stored timings
Skipping 'output' because it does not have one of the recognized file name extensions
No fixes made in 'src/sage/rings/function_field/constructor.py'

Shouldn't I get the deprecation warning?

[mkoeppe]

Thanks for testing this case. I've made the necessary fixes.

[kwankyu]

Thanks. That works now.

I get

$ sage -fixdoctests --overwrite src/sage/schemes/curves/affine_curve.py 
Running "sage -t -p src/sage/schemes/curves/affine_curve.py"
too few successful tests, not using stored timings
No fixes made in 'src/sage/schemes/curves/affine_curve.py'

See the line too few successful tests, not using stored timings. This seems from doctester, and looks spurious. Could we remove that?

[kwankyu]

That comes from second_on_modern_computer() from src/sage/doctest/control.py.

We can avoid that if we give --warn-long option to sage doctester.

[mkoeppe]

I never use that option, what should we use?

[kwankyu]

--warn-long=60 means that the doctester spit an overtime message for a doctest that takes more time than 60 seconds.

[kwankyu]

The default is 60, but adjusted by an actual timing on the local computer based the collected data.

[kwankyu]

    def _init_warn_long(self):
        """
        Pick a suitable default for the ``--warn-long`` option if not specified.

        It is desirable to have all tests (even ``# long`` ones)
        finish in less than about 5 seconds. Longer tests typically
        don't add coverage, they just make testing slow.

        The default used here is 60 seconds on a modern computer. It
        should eventually be lowered to 5 seconds, but its best to
        boil the frog slowly.

        The stored timings are used to adjust this limit according to
        the machine running the tests.

        EXAMPLES::

            sage: from sage.doctest.control import DocTestDefaults, DocTestController
            sage: DC = DocTestController(DocTestDefaults(), [])
            sage: DC.options.warn_long = 5.0
            sage: DC._init_warn_long()
            sage: DC.options.warn_long    # existing command-line options are not changed
            5.00000000000000
        """
        # default is -1.0
        if self.options.warn_long >= 0:     # Specified on the command line
            return
        try:
            self.options.warn_long = 60.0 * self.second_on_modern_computer()
        except RuntimeError as err:
            if not sage.doctest.DOCTEST_MODE:
                print(err)   # No usable timing information

    def second_on_modern_computer(self):
        """
        Return the wall time equivalent of a second on a modern computer.

        OUTPUT:

        Float. The wall time on your computer that would be equivalent
        to one second on a modern computer. Unless you have kick-ass
        hardware this should always be >= 1.0. Raises a
        ``RuntimeError`` if there are no stored timings to use as
        benchmark.

        EXAMPLES::

            sage: from sage.doctest.control import DocTestDefaults, DocTestController
            sage: DC = DocTestController(DocTestDefaults(), [])
            sage: DC.second_on_modern_computer()   # not tested
        """
        if len(self.stats) == 0:
            raise RuntimeError('no stored timings available')
        success = []
        failed = []
        for mod in self.stats.values():
            if mod.get('failed', False):
                failed.append(mod['walltime'])
            else:
                success.append(mod['walltime'])
        if len(success) < 2500:
            raise RuntimeError('too few successful tests, not using stored timings')
        if len(failed) > 20:
            raise RuntimeError('too many failed tests, not using stored timings')
        expected = 12800.0       # Core i7 Quad-Core 2014
        return sum(success) / expected     

[kwankyu]

I don't understand the reasoning in second_on_modern_computer() though...

[mkoeppe]

Thanks. This simple fix seems to do the trick

[kwankyu]

I never looked into this before, but I just did. The data is stored in DOT_SAGE/timings_dt_test.json.

For my case, it happens to contain too few timings, so I get the too few ... warning.

If I had run say sage -t --all previously, I guess I don't get the warning.

[kwankyu]

I don't know what is a "right" fix. Your fix basically sets --warn-long=infinity. I think this is reasonable for --fixdoctests.

[kwankyu]

I get

$ sage -fixdoctests src/bin/sage-fixdoctests
Running "sage -t -p src/bin/sage-fixdoctests"
sage-runtests: Skipping 'src/bin/sage-fixdoctests' because it does not have one of the recognized file name extensions
Skipping 'src/bin/sage-fixdoctests' because it does not have one of the recognized file name extensions

The same message twice.

[mkoeppe]

The first one comes from sage -t, this is intended behavior

[kwankyu]

OK then. This is a minor annoyance. Otherwise LGTM.

[github-actions]

Documentation preview for this PR (built with commit 2dccf18; changes) is ready! 🎉

[mkoeppe]

Thank you!