Opened 12 months ago

Closed 2 weeks ago

#30778 closed enhancement (fixed)

sage.doctest.control: Exclude doctests in files via file directives ''# sage.doctest: optional - xyz'

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: doctest framework Keywords: sd111
Cc: SimonKing, fbissey, roed, saraedum, nthiery, vdelecroix Merged in:
Authors: Matthias Koeppe, John Palmieri Reviewers: John Palmieri, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 5cc1288 (Commits, GitHub, GitLab) Commit: 5cc1288bec7a4c165723f8fd20d14843547faccc
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

When a file is marked # sage.doctest: optional - xyz, we omit it from doctesting unless --optional=xyz is given.

This will save us from having to add lots of # optional - ... tags to files in the course of modularization (#29705)

We do this by extending sage.doctest.control.skipfile, which already parses files for # nodoctest file directives.

Previous related proposals/discussions: #3260, #20427

Also related: #30746

Change History (50)

comment:1 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:2 Changed 11 months ago by mkoeppe

  • Keywords sd111 added

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

comment:3 Changed 8 months ago by mkoeppe

  • Description modified (diff)
  • Milestone changed from sage-9.3 to sage-9.4

comment:4 Changed 8 months ago by mkoeppe

  • Description modified (diff)

comment:5 Changed 8 months ago by mkoeppe

  • Cc fbissey added

comment:6 follow-up: Changed 8 months ago by mkoeppe

We could also parse a directive "# doctest: optional - ..." in the file header

comment:7 Changed 8 months ago by mkoeppe

  • Branch set to u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions

comment:8 in reply to: ↑ 6 ; follow-ups: Changed 8 months ago by jhpalmieri

  • Branch u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions deleted

Replying to mkoeppe:

We could also parse a directive "# doctest: optional - ..." in the file header

This has been discussed in the past and rejected, on the grounds that the directive is hidden when people look at the reference manual or just do x.method?, so they may expect those tests to work all the time. For any of these proposed directives, the corresponding page in the reference manual should have a clear warning. I don't know what to do about the help string for each relevant method/function/class. It's overkill to modify the help string based on such a directive, right?

comment:9 Changed 8 months ago by jhpalmieri

  • Branch set to u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions
  • Commit set to b92e036d7d6ea6a9e5a8ffd93cdd2804d0f0315a

New commits:

b92e036sage.doctest: Handle file directives '# sage.doctest: optional - xyz'

comment:10 Changed 8 months ago by mkoeppe

  • Branch u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions deleted
  • Commit b92e036d7d6ea6a9e5a8ffd93cdd2804d0f0315a deleted
  • Description modified (diff)
  • Summary changed from sage.doctest.control: Exclude doctests in files from non-installed distributions to sage.doctest.control: Exclude doctests in files via file directives ''# sage.doctest: optional - xyz'

comment:11 Changed 8 months ago by mkoeppe

  • Branch set to u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions

comment:12 in reply to: ↑ 8 Changed 8 months ago by mkoeppe

  • Commit set to b92e036d7d6ea6a9e5a8ffd93cdd2804d0f0315a

Replying to jhpalmieri:

Replying to mkoeppe:

We could also parse a directive "# doctest: optional - ..." in the file header

This has been discussed in the past and rejected

I was trying to find the ticket with this discussion but did not succeed


New commits:

b92e036sage.doctest: Handle file directives '# sage.doctest: optional - xyz'

comment:13 in reply to: ↑ 8 Changed 8 months ago by mkoeppe

There is certainly an open question here. I don't think it's only a matter of marking up the doctests with "optional" tags. With modularization going forward, we need to find a way to inform the user that some Python module is provided by a particular distribution package. I think this information should be attached to the module name instead of cluttering the doctests.

comment:14 Changed 8 months ago by mkoeppe

Until we have a more systematic solution for this, I would just suggest that we add an admonition to the module documentation that says something like: "The classes in this module require the optional package rpy2 to be installed"

comment:15 Changed 8 months ago by mkoeppe

OK, found some previous discussion: #3260, #20427

comment:16 Changed 8 months ago by mkoeppe

  • Cc roed saraedum nthiery added
  • Dependencies changed from #30746 to #30746, #3260, #20427
  • Description modified (diff)

Looks like this topic is showing up every 4-6 years.

What has changed now is that modularization (#29705) would introduce LOTS of new optional tags if we don't find a systematic solution.

comment:17 Changed 8 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Dependencies #30746, #3260, #20427 deleted
  • Description modified (diff)
  • Status changed from new to needs_review

comment:18 Changed 8 months ago by mkoeppe

  • Cc vdelecroix added

comment:19 Changed 8 months ago by jhpalmieri

How about a change like this?

  • src/sage/doctest/control.py

    diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
    index 6e75131fb3..0b10e19552 100644
    a b def skipfile(filename, tested_optional_tags=False): 
    216216    - ``tested_optional_tags`` - a list or tuple or set of optional tags to test,
    217217      or ``False`` (no optional test) or ``True`` (all optional tests)
    218218
     219    If ``filename`` contains a line of the form ``"# sage.doctest:
     220    optional - xyz")``, then this will return ``False`` if "xyz" is in
     221    ``tested_optional_tags``. Otherwise, it returns the matching line.
     222
    219223    EXAMPLES::
    220224
    221225        sage: from sage.doctest.control import skipfile
    def skipfile(filename, tested_optional_tags=False): 
    231235        sage: with open(filename, "w") as f:
    232236        ....:     _ = f.write("# sage.doctest: optional - xyz")
    233237        sage: skipfile(filename, False)
     238        '# sage.doctest: optional - xyz'
     239        sage: bool(skipfile(filename, False))
    234240        True
    235241        sage: skipfile(filename, ['abc'])
    236         True
     242        '# sage.doctest: optional - xyz'
    237243        sage: skipfile(filename, ['abc', 'xyz'])
    238244        False
    239245        sage: skipfile(filename, True)
    def skipfile(filename, tested_optional_tags=False): 
    252258                m = optionalfiledirective_regex.match(line)
    253259                if m:
    254260                    if tested_optional_tags is False:
    255                         return True
     261                        return m.group(0)
    256262                    optional_tags = parse_optional_tags('#' + m.group(2))
    257263                    extra = optional_tags - set(tested_optional_tags)
    258264                    if extra:
    259                         return True
     265                        return m.group(0)
    260266            line_count += 1
    261267            if line_count >= 10:
    262268                break
  • src/sage/misc/sageinspect.py

    diff --git a/src/sage/misc/sageinspect.py b/src/sage/misc/sageinspect.py
    index a37edbbffc..670a4aa4e6 100644
    a b def sage_getdoc(obj, obj_name='', embedded_override=False): 
    20312031        return ''
    20322032    r = sage_getdoc_original(obj)
    20332033    s = sage.misc.sagedoc.format(r, embedded=(embedded_override or EMBEDDED_MODE))
     2034    f = sage_getfile(obj)
     2035    if f:
     2036        from sage.doctest.control import skipfile
     2037        skip = skipfile(f)
     2038        if skip:
     2039            warn = """WARNING: the enclosing module is marked '{}',
     2040so doctests may not pass.""".format(skip)
     2041            s = warn + "\n\n" + s
     2042        pass
    20342043
    20352044    # Fix object naming
    20362045    if obj_name != '':

Then the help message printed by obj? will include a warning if the file is tagged to be skipped. Is it okay if skipfile returns a string instead of just True?

comment:20 Changed 8 months ago by mkoeppe

I like this idea. How about using m.group(2) instead? I don't think the reader needs to know about the syntax of the # sage.doctest: directive

comment:21 Changed 8 months ago by jhpalmieri

  • Branch changed from u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions to u/jhpalmieri/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions

comment:22 Changed 8 months ago by jhpalmieri

  • Commit changed from b92e036d7d6ea6a9e5a8ffd93cdd2804d0f0315a to 07818e626a8a69bee77668969840c98d61f9a983

How about this? I also added a bit to the developer's guide.


New commits:

07818e6trac 30778: when a module is labeled "optional - xyz",

comment:23 follow-ups: Changed 8 months ago by roed

Are there any files in the Sage library now that we want to use this feature on? I think having some examples (in this ticket or a followup) would be good.

There are also a couple test failures reported by the patchbot.

comment:24 in reply to: ↑ 23 Changed 8 months ago by SimonKing

Replying to roed:

Are there any files in the Sage library now that we want to use this feature on? I think having some examples (in this ticket or a followup) would be good.

It makes sense for optional extension modules, such as sage.matrix.matrix_gfpn_dense (but I don't know if there are others). Indeed the doctests are one reason why my optional package p_group_cohomology is not part of the sage library, although is is mostly written in Python and Cython and would thus fit well into the Sage library (but should still be an optional package).

comment:25 Changed 8 months ago by mkoeppe

Yes, so let's do #30787...

comment:26 in reply to: ↑ 23 Changed 8 months ago by mkoeppe

  • Dependencies set to #31377, #30912, #31377, #30912

Replying to roed:

Are there any files in the Sage library now that we want to use this feature on? I think having some examples (in this ticket or a followup) would be good.

Indeed it would be best to test this on files that are in tree but could be made "optional" via the same mechanism that we already use for extensions modules depending on optional packages. Candidates are:

  • modules depending on giac
  • modules depending on brial
  • modules depending on ntl
  • more - see #30666

Since this will require modification of setup.py, it's best to do this on top of #31377, #30912

Last edited 8 months ago by mkoeppe (previous) (diff)

comment:27 Changed 8 months ago by mkoeppe

  • Dependencies changed from #31377, #30912, #31377, #30912 to #31377, #30912

comment:28 Changed 8 months ago by mkoeppe

  • Authors changed from Matthias Koeppe to Matthias Koeppe, John Palmieri

comment:29 Changed 8 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:30 Changed 8 months ago by mkoeppe

  • Dependencies changed from #31377, #30912 to #31377, #30912, #30383

... and on top of #30383 because we need to make it possible to use configure --disable-... for at least one standard package.

comment:31 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:32 Changed 3 weeks ago by git

  • Commit changed from 07818e626a8a69bee77668969840c98d61f9a983 to 94727df4dbfa1e03c09e89cb87c2433232318962

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

f35123cMerge branch 'develop' into t/30778/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions
94727dftrac 30778: patch sageinspect to not print warning when skipfile returns True.

comment:33 Changed 3 weeks ago by jhpalmieri

This could still use some examples, as mentioned in comment:23 and comment:26.

comment:34 Changed 3 weeks ago by mkoeppe

  • Dependencies #31377, #30912, #30383 deleted

comment:35 Changed 3 weeks ago by mkoeppe

As an example, we could apply it to src/sage/matrix/matrix_gfpn_dense.pyx and remove the line-by-line annotations "# optional: meataxe".

However, as git grep 'sage:' src/sage/matrix/matrix_gfpn_dense.pyx shows, there are some doctest lines that are not marked optional. Not sure if they have any value by themselves.

comment:36 follow-up: Changed 3 weeks ago by jhpalmieri

@SimonKing?: would you object if we skipped all tests in src/sage/matrix/matrix_gfpn_dense.pyx unless meataxe is installed? I've skimmed through and don't see anything that (I hope) isn't tested elsewhere for other matrix implementations, but you know this file pretty well.

comment:37 in reply to: ↑ 36 Changed 3 weeks ago by SimonKing

Replying to jhpalmieri:

@SimonKing?: would you object if we skipped all tests in src/sage/matrix/matrix_gfpn_dense.pyx unless meataxe is installed? I've skimmed through and don't see anything that (I hope) isn't tested elsewhere for other matrix implementations, but you know this file pretty well.

I did not intend to create new tests for other matrix implementations. So, I wouldn't object to make all the module's tests optional. In fact, this is what I asked for since I first created the module.

comment:38 Changed 3 weeks ago by mkoeppe

  • Branch changed from u/jhpalmieri/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions to u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions

comment:39 Changed 3 weeks ago by mkoeppe

  • Commit changed from 94727df4dbfa1e03c09e89cb87c2433232318962 to 1276609634a35c5d4b0131661791f4d47f1914cf
  • Status changed from needs_work to needs_review

New commits:

1276609src/sage/matrix/matrix_gfpn_dense.pyx: Use # sage.doctest: optional - meataxe

comment:40 Changed 3 weeks ago by mkoeppe

(untested)

comment:41 Changed 3 weeks ago by jhpalmieri

This is working for me the way that I think it should: commands like make ptestlong or ./sage -tp src/sage/matrix/ ignore matrix_gfpn_dense.pyx, but if you specify the file explicitly (or let the shell specify it with ./sage -tp src/sage/matrix/matrix_g*), then it gets tested, and of course many tests fail. I think that's the right behavior.

Last edited 3 weeks ago by jhpalmieri (previous) (diff)

comment:42 follow-up: Changed 3 weeks ago by mkoeppe

Remains to put something in the documentation of matrix_gfpn_dense.pyx, as discussed in comment:8, comment:13, comment:14

comment:43 in reply to: ↑ 42 Changed 3 weeks ago by jhpalmieri

Replying to mkoeppe:

Remains to put something in the documentation of matrix_gfpn_dense.pyx, as discussed in comment:8, comment:13, comment:14

This is easy enough to do manually. It would be nice to autodetect the line # sage.doctest: optional - meataxe, but do we want to do that here or a follow-up ticket?

comment:44 Changed 3 weeks ago by mkoeppe

Let's do this manually for this ticket first

comment:45 Changed 3 weeks ago by mkoeppe

In #32614 I now define some new optional tags, in particular a tag sage.matrix.matrix_gfpn_dense. So if we depend on that ticket, we could now write # sage.doctest: optional - sage.matrix.matrix_gfpn_dense; and in the module documentation we could refer to the class providing the feature (sage__matrix__matrix_gfpn_dense)

comment:46 Changed 3 weeks ago by git

  • Commit changed from 1276609634a35c5d4b0131661791f4d47f1914cf to 5cc1288bec7a4c165723f8fd20d14843547faccc

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

5cc1288src/sage/matrix/matrix_gfpn_dense.pyx: Add reference to meataxe spkg

comment:47 Changed 3 weeks ago by mkoeppe

I've kept it simple and just added a note in the docstring.

But do we actually build HTML documentation for optional compiled modules at all?

comment:48 Changed 2 weeks ago by jhpalmieri

  • Reviewers set to John Palmieri, ...

I'm happy with your changes on the ticket: positive review for those parts.

comment:49 Changed 2 weeks ago by mkoeppe

  • Reviewers changed from John Palmieri, ... to John Palmieri, Matthias Koeppe
  • Status changed from needs_review to positive_review

Thanks!

comment:50 Changed 2 weeks ago by vbraun

  • Branch changed from u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions to 5cc1288bec7a4c165723f8fd20d14843547faccc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.