Skip to content

Commit 821d8da

Browse files
author
Release Manager
committed
gh-36741: Doctest hide option: Better detection of hidden packages <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes #1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> This PR implements the suggestion made in #36696 (comment). This means that it introduces a counter in the feature class to detect the number of events a feature has been asked to be present while it was hidden. This allows to remove the call of the `is_present` method in `doctest/control.py`. In order to test it I ran `sage -tp --all --hide=all`. Ideally this should give *All tests passed*. But I got two failing files. One of those was `src/sage/features/databases.py` because `database_cremona_mini_ellcurve` was detected to be optional even thought it is a standard package. This is a leftover of #35820 which I fix here. Similarily I got 37 failures in `src/sage/groups/abelian_gps/abelian_group.py` since `gap_package_polycyclic` was classified optional even though it is a Gap standard package since Gap 4.10 (as well as `gap_package_atlasrep`). But in this case a corresponding fix, i.e.: ``` def all_features(): - return [GapPackage("atlasrep", spkg="gap_packages"), + return [GapPackage("atlasrep", spkg="gap_packages", type='standard'), GapPackage("design", spkg="gap_packages"), GapPackage("grape", spkg="gap_packages"), GapPackage("guava", spkg="gap_packages"), GapPackage("hap", spkg="gap_packages"), - GapPackage("polycyclic", spkg="gap_packages"), + GapPackage("polycyclic", spkg="gap_packages", type='standard'), GapPackage("qpa", spkg="gap_packages"), GapPackage("quagroup", spkg="gap_packages")] ``` is not the correct way (it gives `UserWarning: Feature gap_package_polycyclic is declared standard, but it is provided by gap_packages, which is declared optional in SAGE_ROOT/build/pkgs`). I would say, the correct fix would be to remove both Gap packages from the `all_features` list and erase the corresponding *optional tags* in `permgroup_named.py`, `distance_regular.pyx`, `abelian_aut.py`, `abelian_group.py` and `abelian_group_gap.py`. If you agree I will open another PR for this. BTW: there seem to be more packages with ambiguous type (detected with current Docker image): ``` Digest: sha256:790197bb223bd7e20b0a2e055aa7b4c5846dc86d94b2425cd233cb6160a5b944 Status: Downloaded newer image for sagemath/sagemath:develop ┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 10.2.rc4, Release Date: 2023-11-17 │ │ Using Python 3.11.1. Type "help()" for help. │ └────────────────────────────────────────────────────────────────────┘ ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ ┃ Warning: this is a prerelease version, and it may be unstable. ┃ ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ sage: from sage.features.all import all_features sage: [(f, f._spkg_type()) for f in all_features() if f.is_present() and f._spkg_type() != 'standard'] [(Feature('sage.misc.cython'), 'optional'), (Feature('database_cremona_mini_ellcurve': Cremona's database of elliptic curves), 'optional'), (Feature('gap_package_atlasrep'), 'optional'), (Feature('gap_package_polycyclic'), 'optional'), (Feature('sage.libs.ecl'), 'optional'), (Feature('sage.libs.gap'), 'optional'), (Feature('sage.libs.singular'), 'optional'), (Feature('sage.numerical.mip'), 'optional')] sage: sage: [(f, f._spkg_type()) for f in all_features() if not f.is_present() and f._spkg_type() == 'standard'] [(Feature('sagemath_doc_html'), 'standard'), (Feature('sage.sat'), 'standard')] ``` ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #36741 Reported by: Sebastian Oehms Reviewer(s): Matthias Köppe, Sebastian Oehms
2 parents ad7ee31 + 03670bd commit 821d8da

File tree

5 files changed

+83
-60
lines changed

5 files changed

+83
-60
lines changed

src/sage/doctest/control.py

Lines changed: 37 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -416,7 +416,7 @@ def __init__(self, options, args):
416416
if options.gc:
417417
options.timeout *= 2
418418
if options.nthreads == 0:
419-
options.nthreads = int(os.getenv('SAGE_NUM_THREADS_PARALLEL',1))
419+
options.nthreads = int(os.getenv('SAGE_NUM_THREADS_PARALLEL', 1))
420420
if options.failed and not (args or options.new):
421421
# If the user doesn't specify any files then we rerun all failed files.
422422
options.all = True
@@ -450,15 +450,11 @@ def __init__(self, options, args):
450450
options.hide.discard('all')
451451
from sage.features.all import all_features
452452
feature_names = {f.name for f in all_features() if not f.is_standard()}
453-
from sage.doctest.external import external_software
454-
feature_names.difference_update(external_software)
455453
options.hide = options.hide.union(feature_names)
456454
if 'optional' in options.hide:
457455
options.hide.discard('optional')
458456
from sage.features.all import all_features
459457
feature_names = {f.name for f in all_features() if f.is_optional()}
460-
from sage.doctest.external import external_software
461-
feature_names.difference_update(external_software)
462458
options.hide = options.hide.union(feature_names)
463459

464460
options.disabled_optional = set()
@@ -1008,7 +1004,7 @@ def expand():
10081004
if os.path.isdir(path):
10091005
for root, dirs, files in os.walk(path):
10101006
for dir in list(dirs):
1011-
if dir[0] == "." or skipdir(os.path.join(root,dir)):
1007+
if dir[0] == "." or skipdir(os.path.join(root, dir)):
10121008
dirs.remove(dir)
10131009
for file in files:
10141010
if not skipfile(os.path.join(root, file),
@@ -1345,9 +1341,9 @@ def run_val_gdb(self, testing=False):
13451341
flags = os.getenv("SAGE_MEMCHECK_FLAGS")
13461342
if flags is None:
13471343
flags = "--leak-resolution=high --leak-check=full --num-callers=25 "
1348-
flags += '''--suppressions="%s" ''' % (os.path.join(SAGE_EXTCODE,"valgrind", "pyalloc.supp"))
1349-
flags += '''--suppressions="%s" ''' % (os.path.join(SAGE_EXTCODE,"valgrind", "sage.supp"))
1350-
flags += '''--suppressions="%s" ''' % (os.path.join(SAGE_EXTCODE,"valgrind", "sage-additional.supp"))
1344+
flags += '''--suppressions="%s" ''' % (os.path.join(SAGE_EXTCODE, "valgrind", "pyalloc.supp"))
1345+
flags += '''--suppressions="%s" ''' % (os.path.join(SAGE_EXTCODE, "valgrind", "sage.supp"))
1346+
flags += '''--suppressions="%s" ''' % (os.path.join(SAGE_EXTCODE, "valgrind", "sage-additional.supp"))
13511347
elif opt.massif:
13521348
toolname = "massif"
13531349
flags = os.getenv("SAGE_MASSIF_FLAGS", "--depth=6 ")
@@ -1362,7 +1358,7 @@ def run_val_gdb(self, testing=False):
13621358
if opt.omega:
13631359
toolname = "omega"
13641360
if "%s" in flags:
1365-
flags %= toolname + ".%p" # replace %s with toolname
1361+
flags %= toolname + ".%p" # replace %s with toolname
13661362
cmd += flags + sage_cmd
13671363

13681364
sys.stdout.flush()
@@ -1489,6 +1485,25 @@ def run(self):
14891485
cumulative wall time: ... seconds
14901486
Features detected...
14911487
0
1488+
1489+
Test *Features that have been hidden* message::
1490+
1491+
sage: DC.run() # optional - meataxe
1492+
Running doctests with ID ...
1493+
Using --optional=sage
1494+
Features to be detected: ...
1495+
Doctesting 1 file.
1496+
sage -t ....py
1497+
[4 tests, ... s]
1498+
----------------------------------------------------------------------
1499+
All tests passed!
1500+
----------------------------------------------------------------------
1501+
Total time for all tests: ... seconds
1502+
cpu time: ... seconds
1503+
cumulative wall time: ... seconds
1504+
Features detected...
1505+
Features that have been hidden: ...meataxe...
1506+
0
14921507
"""
14931508
opt = self.options
14941509
L = (opt.gdb, opt.lldb, opt.valgrind, opt.massif, opt.cachegrind, opt.omega)
@@ -1516,10 +1531,10 @@ def run(self):
15161531
pass
15171532
try:
15181533
ref = subprocess.check_output(["git",
1519-
"--git-dir=" + SAGE_ROOT_GIT,
1520-
"describe",
1521-
"--always",
1522-
"--dirty"])
1534+
"--git-dir=" + SAGE_ROOT_GIT,
1535+
"describe",
1536+
"--always",
1537+
"--dirty"])
15231538
ref = ref.decode('utf-8')
15241539
self.log("Git ref: " + ref, end="")
15251540
except subprocess.CalledProcessError:
@@ -1537,12 +1552,11 @@ def run(self):
15371552
pass
15381553
else:
15391554
f = available_software._features[i]
1540-
if f.is_present():
1541-
f.hide()
1542-
self.options.hidden_features.add(f)
1543-
for g in f.joined_features():
1544-
if g.name in self.options.optional:
1545-
self.options.optional.discard(g.name)
1555+
f.hide()
1556+
self.options.hidden_features.add(f)
1557+
for g in f.joined_features():
1558+
if g.name in self.options.optional:
1559+
self.options.optional.discard(g.name)
15461560

15471561
for o in self.options.disabled_optional:
15481562
try:
@@ -1553,8 +1567,6 @@ def run(self):
15531567
available_software._seen[i] = -1
15541568

15551569
self.log("Features to be detected: " + ','.join(available_software.detectable()))
1556-
if self.options.hidden_features:
1557-
self.log("Hidden features: " + ','.join([f.name for f in self.options.hidden_features]))
15581570
if self.options.probe:
15591571
self.log("Features to be probed: " + ('all' if self.options.probe is True
15601572
else ','.join(self.options.probe)))
@@ -1564,11 +1576,11 @@ def run(self):
15641576
self.sort_sources()
15651577
self.run_doctests()
15661578

1567-
for f in self.options.hidden_features:
1568-
f.unhide()
1569-
15701579
self.log("Features detected for doctesting: "
15711580
+ ','.join(available_software.seen()))
1581+
if self.options.hidden_features:
1582+
features_hidden = [f.name for f in self.options.hidden_features if f.unhide()]
1583+
self.log("Features that have been hidden: " + ','.join(features_hidden))
15721584
self.cleanup()
15731585
return self.reporter.error_status
15741586

src/sage/features/__init__.py

Lines changed: 34 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
Here we test whether the grape GAP package is available::
2929
3030
sage: from sage.features.gap import GapPackage
31-
sage: GapPackage("grape", spkg="gap_packages").is_present() # optional - gap_packages
31+
sage: GapPackage("grape", spkg="gap_packages").is_present() # optional - gap_package_grape
3232
FeatureTestResult('gap_package_grape', True)
3333
3434
Note that a :class:`FeatureTestResult` acts like a bool in most contexts::
@@ -158,6 +158,12 @@ def __init__(self, name, spkg=None, url=None, description=None, type='optional')
158158
self._hidden = False
159159
self._type = type
160160

161+
# For multiprocessing of doctests, the data self._num_hidings should be
162+
# shared among subprocesses. Thus we use the Value class from the
163+
# multiprocessing module (cf. self._seen of class AvailableSoftware)
164+
from multiprocessing import Value
165+
self._num_hidings = Value('i', 0)
166+
161167
try:
162168
from sage.misc.package import spkg_type
163169
except ImportError: # may have been surgically removed in a downstream distribution
@@ -182,7 +188,7 @@ def is_present(self):
182188
EXAMPLES::
183189
184190
sage: from sage.features.gap import GapPackage
185-
sage: GapPackage("grape", spkg="gap_packages").is_present() # optional - gap_packages
191+
sage: GapPackage("grape", spkg="gap_packages").is_present() # optional - gap_package_grape
186192
FeatureTestResult('gap_package_grape', True)
187193
sage: GapPackage("NOT_A_PACKAGE", spkg="gap_packages").is_present()
188194
FeatureTestResult('gap_package_NOT_A_PACKAGE', False)
@@ -205,15 +211,21 @@ def is_present(self):
205211
sage: TestFeature("other").is_present()
206212
FeatureTestResult('other', True)
207213
"""
208-
if self._hidden:
209-
return FeatureTestResult(self, False, reason="Feature `{name}` is hidden.".format(name=self.name))
210214
# We do not use @cached_method here because we wish to use
211215
# Feature early in the build system of sagelib.
212216
if self._cache_is_present is None:
213217
res = self._is_present()
214218
if not isinstance(res, FeatureTestResult):
215219
res = FeatureTestResult(self, res)
216220
self._cache_is_present = res
221+
222+
if self._hidden:
223+
if self._num_hidings.value > 0:
224+
self._num_hidings.value += 1
225+
elif self._cache_is_present:
226+
self._num_hidings.value = 1
227+
return FeatureTestResult(self, False, reason="Feature `{name}` is hidden.".format(name=self.name))
228+
217229
return self._cache_is_present
218230

219231
def _is_present(self):
@@ -381,40 +393,34 @@ def hide(self):
381393
Feature `benzene` is hidden.
382394
Use method `unhide` to make it available again.
383395
384-
sage: Benzene().unhide()
396+
sage: Benzene().unhide() # optional - benzene, needs sage.graphs
397+
1
385398
sage: len(list(graphs.fusenes(2))) # optional - benzene, needs sage.graphs
386399
1
387400
"""
388401
self._hidden = True
389402

390403
def unhide(self):
391404
r"""
392-
Revert what :meth:`hide` does.
393-
394-
EXAMPLES:
405+
Revert what :meth:`hide` did.
395406
396-
PolyCyclic is an optional GAP package. The following test
397-
fails if it is hidden, regardless of whether it is installed
398-
or not::
407+
OUTPUT: The number of events a present feature has been hidden.
399408
400-
sage: from sage.features.gap import GapPackage
401-
sage: Polycyclic = GapPackage("polycyclic", spkg="gap_packages")
402-
sage: Polycyclic.hide()
403-
sage: libgap(AbelianGroup(3, [0,3,4], names="abc")) # needs sage.libs.gap # optional - gap_packages_polycyclic
404-
Traceback (most recent call last):
405-
...
406-
FeatureNotPresentError: gap_package_polycyclic is not available.
407-
Feature `gap_package_polycyclic` is hidden.
408-
Use method `unhide` to make it available again.
409-
410-
After unhiding the feature, the test should pass again if PolyCyclic
411-
is installed and loaded::
409+
EXAMPLES:
412410
413-
sage: Polycyclic.unhide()
414-
sage: libgap(AbelianGroup(3, [0,3,4], names="abc")) # needs sage.libs.gap # optional - gap_packages_polycyclic
415-
Pcp-group with orders [ 0, 3, 4 ]
411+
sage: from sage.features.sagemath import sage__plot
412+
sage: sage__plot().hide()
413+
sage: sage__plot().is_present()
414+
FeatureTestResult('sage.plot', False)
415+
sage: sage__plot().unhide() # needs sage.plot
416+
1
417+
sage: sage__plot().is_present() # needs sage.plot
418+
FeatureTestResult('sage.plot', True)
416419
"""
420+
num_hidings = self._num_hidings.value
421+
self._num_hidings.value = 0
417422
self._hidden = False
423+
return int(num_hidings)
418424

419425

420426
class FeatureNotPresentError(RuntimeError):
@@ -802,7 +808,7 @@ class StaticFile(FileFeature):
802808
To install no_such_file...you can try to run...sage -i some_spkg...
803809
Further installation instructions might be available at http://rand.om.
804810
"""
805-
def __init__(self, name, filename, *, search_path=None, **kwds):
811+
def __init__(self, name, filename, *, search_path=None, type='optional', **kwds):
806812
r"""
807813
TESTS::
808814
@@ -817,7 +823,7 @@ def __init__(self, name, filename, *, search_path=None, **kwds):
817823
'/bin/sh'
818824
819825
"""
820-
Feature.__init__(self, name, **kwds)
826+
Feature.__init__(self, name, type=type, **kwds)
821827
self.filename = filename
822828
if search_path is None:
823829
self.search_path = [SAGE_SHARE]

src/sage/features/databases.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,12 @@ class DatabaseCremona(StaticFile):
5252
EXAMPLES::
5353
5454
sage: from sage.features.databases import DatabaseCremona
55-
sage: DatabaseCremona('cremona_mini').is_present()
55+
sage: DatabaseCremona('cremona_mini', type='standard').is_present()
5656
FeatureTestResult('database_cremona_mini_ellcurve', True)
5757
sage: DatabaseCremona().is_present() # optional - database_cremona_ellcurve
5858
FeatureTestResult('database_cremona_ellcurve', True)
5959
"""
60-
def __init__(self, name="cremona"):
60+
def __init__(self, name="cremona", spkg="database_cremona_ellcurve", type='optional'):
6161
r"""
6262
TESTS::
6363
@@ -290,7 +290,7 @@ def __init__(self, name='polytopes_db'):
290290
def all_features():
291291
return [PythonModule('conway_polynomials', spkg='conway_polynomials', type='standard'),
292292
DatabaseCremona(),
293-
DatabaseCremona('cremona_mini'),
293+
DatabaseCremona('cremona_mini', type='standard'),
294294
DatabaseEllcurves(),
295295
DatabaseGraphs(),
296296
DatabaseJones(),

src/sage/features/gap.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def _is_present(self):
5353
EXAMPLES::
5454
5555
sage: from sage.features.gap import GapPackage
56-
sage: GapPackage("grape", spkg="gap_packages")._is_present() # optional - gap_packages
56+
sage: GapPackage("grape", spkg="gap_packages")._is_present() # optional - gap_package_grape
5757
FeatureTestResult('gap_package_grape', True)
5858
"""
5959
try:

src/sage/features/join_feature.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,9 @@ def hide(self):
152152

153153
def unhide(self):
154154
r"""
155-
Revert what :meth:`hide` does.
155+
Revert what :meth:`hide` did.
156+
157+
OUTPUT: The number of events a present feature has been hidden.
156158
157159
EXAMPLES::
158160
@@ -165,11 +167,14 @@ def unhide(self):
165167
FeatureTestResult('sage.groups.perm_gps.permgroup', False)
166168
167169
sage: f.unhide()
170+
4
168171
sage: f.is_present() # optional sage.groups
169172
FeatureTestResult('sage.groups', True)
170173
sage: f._features[0].is_present() # optional sage.groups
171174
FeatureTestResult('sage.groups.perm_gps.permgroup', True)
172175
"""
176+
num_hidings = 0
173177
for f in self._features:
174-
f.unhide()
175-
super().unhide()
178+
num_hidings += f.unhide()
179+
num_hidings += super().unhide()
180+
return num_hidings

0 commit comments

Comments
 (0)