Opened 22 months ago
Closed 10 months 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: |
Description (last modified by )
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 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:2 Changed 20 months ago by
- Keywords sd111 added
comment:3 Changed 18 months ago by
- Description modified (diff)
- Milestone changed from sage-9.3 to sage-9.4
comment:4 Changed 18 months ago by
- Description modified (diff)
comment:5 Changed 18 months ago by
- Cc fbissey added
comment:6 follow-up: ↓ 8 Changed 18 months ago by
We could also parse a directive "# doctest: optional - ..." in the file header
comment:7 Changed 18 months ago by
- Branch set to u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions
comment:8 in reply to: ↑ 6 ; follow-ups: ↓ 12 ↓ 13 Changed 18 months ago by
- 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 18 months ago by
- Branch set to u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions
- Commit set to b92e036d7d6ea6a9e5a8ffd93cdd2804d0f0315a
New commits:
b92e036 | sage.doctest: Handle file directives '# sage.doctest: optional - xyz'
|
comment:10 Changed 18 months ago by
- 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 18 months ago by
- Branch set to u/mkoeppe/sage_doctest_control__exclude_doctests_in_files_from_non_installed_distributions
comment:12 in reply to: ↑ 8 Changed 18 months ago by
- 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:
b92e036 | sage.doctest: Handle file directives '# sage.doctest: optional - xyz'
|
comment:13 in reply to: ↑ 8 Changed 18 months ago by
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 18 months ago by
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 18 months ago by
comment:16 Changed 18 months ago by
- 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 18 months ago by
- Dependencies #30746, #3260, #20427 deleted
- Description modified (diff)
- Status changed from new to needs_review
comment:18 Changed 18 months ago by
- Cc vdelecroix added
comment:19 Changed 18 months ago by
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): 216 216 - ``tested_optional_tags`` - a list or tuple or set of optional tags to test, 217 217 or ``False`` (no optional test) or ``True`` (all optional tests) 218 218 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 219 223 EXAMPLES:: 220 224 221 225 sage: from sage.doctest.control import skipfile … … def skipfile(filename, tested_optional_tags=False): 231 235 sage: with open(filename, "w") as f: 232 236 ....: _ = f.write("# sage.doctest: optional - xyz") 233 237 sage: skipfile(filename, False) 238 '# sage.doctest: optional - xyz' 239 sage: bool(skipfile(filename, False)) 234 240 True 235 241 sage: skipfile(filename, ['abc']) 236 True242 '# sage.doctest: optional - xyz' 237 243 sage: skipfile(filename, ['abc', 'xyz']) 238 244 False 239 245 sage: skipfile(filename, True) … … def skipfile(filename, tested_optional_tags=False): 252 258 m = optionalfiledirective_regex.match(line) 253 259 if m: 254 260 if tested_optional_tags is False: 255 return True261 return m.group(0) 256 262 optional_tags = parse_optional_tags('#' + m.group(2)) 257 263 extra = optional_tags - set(tested_optional_tags) 258 264 if extra: 259 return True265 return m.group(0) 260 266 line_count += 1 261 267 if line_count >= 10: 262 268 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): 2031 2031 return '' 2032 2032 r = sage_getdoc_original(obj) 2033 2033 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 '{}', 2040 so doctests may not pass.""".format(skip) 2041 s = warn + "\n\n" + s 2042 pass 2034 2043 2035 2044 # Fix object naming 2036 2045 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 18 months ago by
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 18 months ago by
- 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 18 months ago by
- Commit changed from b92e036d7d6ea6a9e5a8ffd93cdd2804d0f0315a to 07818e626a8a69bee77668969840c98d61f9a983
How about this? I also added a bit to the developer's guide.
New commits:
07818e6 | trac 30778: when a module is labeled "optional - xyz",
|
comment:23 follow-ups: ↓ 24 ↓ 26 Changed 17 months ago by
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 17 months ago by
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 17 months ago by
Yes, so let's do #30787...
comment:26 in reply to: ↑ 23 Changed 17 months ago by
- 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
comment:27 Changed 17 months ago by
- Dependencies changed from #31377, #30912, #31377, #30912 to #31377, #30912
comment:28 Changed 17 months ago by
comment:29 Changed 17 months ago by
- Status changed from needs_review to needs_work
comment:30 Changed 17 months ago by
- 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 13 months ago by
- 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 10 months ago by
- Commit changed from 07818e626a8a69bee77668969840c98d61f9a983 to 94727df4dbfa1e03c09e89cb87c2433232318962
comment:33 Changed 10 months ago by
This could still use some examples, as mentioned in comment:23 and comment:26.
comment:34 Changed 10 months ago by
- Dependencies #31377, #30912, #30383 deleted
comment:35 Changed 10 months ago by
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: ↓ 37 Changed 10 months ago by
@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 10 months ago by
Replying to jhpalmieri:
@SimonKing?: would you object if we skipped all tests in
src/sage/matrix/matrix_gfpn_dense.pyx
unlessmeataxe
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 10 months ago by
- 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 10 months ago by
- Commit changed from 94727df4dbfa1e03c09e89cb87c2433232318962 to 1276609634a35c5d4b0131661791f4d47f1914cf
- Status changed from needs_work to needs_review
New commits:
1276609 | src/sage/matrix/matrix_gfpn_dense.pyx: Use # sage.doctest: optional - meataxe
|
comment:40 Changed 10 months ago by
(untested)
comment:41 Changed 10 months ago by
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.
comment:42 follow-up: ↓ 43 Changed 10 months ago by
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 10 months ago by
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 10 months ago by
Let's do this manually for this ticket first
comment:45 Changed 10 months ago by
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 10 months ago by
- Commit changed from 1276609634a35c5d4b0131661791f4d47f1914cf to 5cc1288bec7a4c165723f8fd20d14843547faccc
Branch pushed to git repo; I updated commit sha1. New commits:
5cc1288 | src/sage/matrix/matrix_gfpn_dense.pyx: Add reference to meataxe spkg
|
comment:47 Changed 10 months ago by
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 10 months ago by
- Reviewers set to John Palmieri, ...
I'm happy with your changes on the ticket: positive review for those parts.
comment:49 Changed 10 months ago by
- Reviewers changed from John Palmieri, ... to John Palmieri, Matthias Koeppe
- Status changed from needs_review to positive_review
Thanks!
comment:50 Changed 10 months ago by
- 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
Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111