#33823 closed enhancement (fixed)
sage -t --optional='sage,!FEATURE'
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.7 |
Component: | refactoring | Keywords: | |
Cc: | klee, mjo, fbissey, slabbe | Merged in: | |
Authors: | Matthias Koeppe | Reviewers: | Sebastian Oehms |
Report Upstream: | N/A | Work issues: | |
Branch: | ba86146 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
sage -t --optional='sage,!FEATURE'
will disable autodetection of FEATURE
.
Useful for checking sequences of # optional - FEATURE
annotations in particular when FEATURE
is standard in Sage, for example sage.misc.cython
(#33029):
./sage -t --optional='sage,!sage.misc.cython' src/sage/misc/inherit_comparison.pyx
Also, sage -t --optional='sage,optional,!FEATURE'
will remove it from the list of optional features supplied by the package list and disable auto-detection.
For example, when the optional SPKG bliss
is installed, --optional='sage,optional'
would expand to a list including bliss
. By using --optional='sage,optional,!bliss'
, it can be removed.
Change History (28)
comment:1 Changed 3 months ago by
- Summary changed from sage -t --optional=sage,!FEATURE to sage -t --optional='sage,!FEATURE'
comment:2 Changed 3 months ago by
- Description modified (diff)
comment:3 Changed 3 months ago by
- Branch set to u/mkoeppe/sage__t___optional__sage__feature_
comment:4 Changed 3 months ago by
- Cc klee mjo added
- Commit set to 800032050b15c7b743635b7c08892375f7f5c563
- Status changed from new to needs_review
comment:5 Changed 3 months ago by
- Description modified (diff)
comment:6 Changed 3 months ago by
- Commit changed from 800032050b15c7b743635b7c08892375f7f5c563 to d0a52ed409ed07ae060c305f7481ab41479d3403
Branch pushed to git repo; I updated commit sha1. New commits:
d0a52ed | src/bin/sage-runtests: Update documentation of --optional
|
comment:7 Changed 3 months ago by
- Cc fbissey added
comment:8 Changed 3 months ago by
How about respecting --disable-bliss
instead?
comment:9 follow-up: ↓ 11 Changed 3 months ago by
We don't have a mechanism for that, and it does not help for the main use case of this ticket - disabling standard features such as sage.misc.cython
when doctesting.
comment:10 follow-up: ↓ 14 Changed 3 months ago by
Yes, you are giving yourself some tools to be able to isolate what you are testing more. Which is interesting in and of itself. But if we completely migrate to pytest (let's be honest that will/would take years), will that feature survive the migration? Does pytest support such disabling?
comment:11 in reply to: ↑ 9 ; follow-ups: ↓ 12 ↓ 13 Changed 3 months ago by
Replying to mkoeppe:
We don't have a mechanism for that
It worked for years until you added runtime detection that overrides it. Now you want to add a third layer of complexity without fixing the previous two.
I posted one possible solution recently on the mailing list and another ticket. Another (inevitable, eventually) option is to add a ./configure
script to whatever modularized component of sage requires bliss, and to copy AC_ARG_ENABLE([bliss])
there. Then the "feature" that detects it from the other components becomes simply e.g. import sage.whatever.BLISS
. There is already precedent for having setup.py run sh ./configure
to keep that component pip-installable.
comment:12 in reply to: ↑ 11 Changed 3 months ago by
Replying to mjo:
Replying to mkoeppe:
We don't have a mechanism for that
It worked for years until you added runtime detection that overrides it.
Sage has never had a mechanism to disable the use of optional features by the Sage library.
The only thing that has changed is what the doctester tests. The present ticket gives you the tool that you need if you have problems with an optional package in your distribution packaging.
comment:13 in reply to: ↑ 11 Changed 3 months ago by
Let's keep the ticket focused. It is not meant as an invitation to continue your musings from sage-devel
comment:14 in reply to: ↑ 10 Changed 3 months ago by
Replying to fbissey:
Yes, you are giving yourself some tools to be able to isolate what you are testing more. Which is interesting in and of itself. But if we completely migrate to pytest (let's be honest that will/would take years), will that feature survive the migration? Does pytest support such disabling?
Conditionalizing tests on the presence of features is a key idea of the modularization, and we'll make it work with pytest too. Skipping tests based on predicates is a standard feature of all test frameworks.
#33546 implements the Sage doctest discovery in pytest. It continues to use code from sage.doctest
, so I see no issue in making # optional
annotations work, but I don't know the details.
comment:15 Changed 3 months ago by
- Description modified (diff)
comment:16 Changed 3 months ago by
- Cc slabbe added
comment:17 follow-up: ↓ 20 Changed 3 months ago by
I think it's a good idea to have a negation possibility here. Maybe the following needs a clarification in the help-string:
In a positive list of features you don't need to type '
and maybe a lot of people use it in this way. But if you want to exclude a feature using this new functionality you must type them or use \!
instead of !
.
BTW: If you have installed some optional feature xyz
on your system and forgot a # optional - xyz
or xyz.is_present()
somewhere in your change of code then this new doctest option will not bring this to your attention. That was what I hoped when I've first seen this ticket. Do you know, how this can be achieved without uninstalling the package?
Further: I really don't know the intention of the log-message Features detected for doctesting
. My observation is that this messages shows nothing even though optional tests were performed. At least this is irritating. Example:
Using --optional=!database_knotinfo,sage Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,dvipng,gfan,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,msolve,nauty,palp,pandoc,pdf2svg,pdftocairo,phitigra,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib Doctesting 1 file. sage -t --random-seed=71318033978106427613212696911118388467 src/sage/knots/knotinfo.py [323 tests, 5.51 s] ---------------------------------------------------------------------- All tests passed! ---------------------------------------------------------------------- Total time for all tests: 5.6 seconds cpu time: 5.5 seconds cumulative wall time: 5.5 seconds Features detected for doctesting:
Together with 30 optional doctests, the log-message shows the same result:
Using --optional=database_knotinfo,sage Features to be detected: 4ti2,benzene,bliss,buckygen,conway_polynomials,csdp,database_cremona_ellcurve,database_cremona_mini_ellcurve,database_jones_numfield,database_knotinfo,dvipng,gfan,graphviz,imagemagick,jupymake,kenzo,latte_int,lrslib,mcqd,meataxe,msolve,nauty,palp,pandoc,pdf2svg,pdftocairo,phitigra,plantri,polytopes_db,polytopes_db_4d,pynormaliz,python_igraph,rubiks,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.plot,sage.rings.number_field,sage.rings.padics,sage.rings.real_double,sage.symbolic,sage_numerical_backends_coin,sagemath_doc_html,sphinx,tdlib Doctesting 1 file. sage -t --random-seed=32074844050499063835156990883996507444 src/sage/knots/knotinfo.py [353 tests, 7.94 s] ---------------------------------------------------------------------- All tests passed! ---------------------------------------------------------------------- Total time for all tests: 8.1 seconds cpu time: 7.9 seconds cumulative wall time: 7.9 seconds Features detected for doctesting:
This seems to be due to:
sage: from sage.doctest.external import AvailableSoftware sage: S = AvailableSoftware() sage: S._indices['database_knotinfo'] 10 sage: S._seen[10] 0
But this can be changed by performing a __contains__
test:
sage: 'database_knotinfo' in S True sage: S._seen[10] 1
So it looks as if such a test is missing somewhere.
comment:18 Changed 2 months ago by
- Commit changed from d0a52ed409ed07ae060c305f7481ab41479d3403 to ba86146e5a5e3308026aac49d87d2716a80c6a93
comment:19 Changed 2 months ago by
The line Features detected for doctesting:
only reports the dynamically detected features, a subset of what is listed as Features to be detected:
.
The statically known ones (from --optional
) are not reported there.
This works as designed, but if you have a suggestion how it should be changed, we can change it.
comment:20 in reply to: ↑ 17 ; follow-up: ↓ 21 Changed 2 months ago by
Replying to soehms:
If you have installed some optional feature
xyz
on your system and forgot a# optional - xyz
orxyz.is_present()
somewhere in your change of code then this new doctest option will not bring this to your attention. That was what I hoped when I've first seen this ticket. Do you know, how this can be achieved without uninstalling the package?
The doctests marked # optional - xyz
*are* disabled using --optional="!xyz"
-- as your test above with --optional=!database_knotinfo,sage
verified.
There is no mechanism to disable the actual feature in the Sage runtime. What xyz.is_present()
does is not affected by the present ticket.
comment:21 in reply to: ↑ 20 ; follow-ups: ↓ 22 ↓ 24 Changed 2 months ago by
- Reviewers set to Sebastian Oehms
- Status changed from needs_review to positive_review
Replying to mkoeppe:
The line Features detected for doctesting: only reports the dynamically detected features, a subset of what is listed as Features to be detected:. The statically known ones (from --optional) are not reported there.
I'm wondering what dynamically means here? From the code I displayed in my previous comment it appears that a feature is dynamically detected if it is tested to be contained in available_software
. Where is that supposed to happen?
This works as designed, but if you have a suggestion how it should be changed, we can change it.
It's difficult to make a suggestion before I've completely understood the intention. According to the given phrase I assumed it would correspond to the Features to be detected. So, at least in order to resolve that irritation it would be enough to change the phrase into Dynamically detected features ...
(whatever this means).
Replying to soehms: There is no mechanism to disable the actual feature in the Sage runtime. What
xyz.is_present()
does is not affected by the present ticket.
Perhaps I have in mind a new option sage -t --hide=FEATURES
which would imply --optional='!FEATURE1,!FEATURE2,..'
. Is there something against having such an additional option? I think that could be realized via a boolean _hidden
in class Feature
, which default is False
and would be switched to True
in case of this option.
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 23 Changed 2 months ago by
Replying to soehms:
Replying to mkoeppe:
The line Features detected for doctesting: only reports the dynamically detected features, a subset of what is listed as Features to be detected:. The statically known ones (from --optional) are not reported there.
I'm wondering what dynamically means here? From the code I displayed in my previous comment it appears that a feature is dynamically detected if it is tested to be contained in
available_software
. Where is that supposed to happen?
Sorry for using undefined terminology.
When you use sage -t --optional=sage,FEATURE
, then FEATURE is "statically known".
When you use sage -t --optional=sage,optional
and SPKG is the name of a normal package that is installed, then SPKG is "statically known".
Other features, by default those enumerated by sage.features.all.all_features
except for those enumerated by sage.doctest.external.external_software
, are subject to detection the first time that their name appears as an # optional - FEATURE
annotation. (This is done by sage.doctest.parsing.SageDocTestParser
.)
comment:23 in reply to: ↑ 22 Changed 2 months ago by
Replying to mkoeppe:
Replying to soehms:
Replying to mkoeppe:
The line Features detected for doctesting: only reports the dynamically detected features, a subset of what is listed as Features to be detected:. The statically known ones (from --optional) are not reported there.
I'm wondering what dynamically means here? From the code I displayed in my previous comment it appears that a feature is dynamically detected if it is tested to be contained in
available_software
. Where is that supposed to happen?Sorry for using undefined terminology.
When you use
sage -t --optional=sage,FEATURE
, then FEATURE is "statically known".When you use
sage -t --optional=sage,optional
and SPKG is the name of a normal package that is installed, then SPKG is "statically known".Other features, by default those enumerated by
sage.features.all.all_features
except for those enumerated bysage.doctest.external.external_software
, are subject to detection the first time that their name appears as an# optional - FEATURE
annotation. (This is done bysage.doctest.parsing.SageDocTestParser
.)
Thanks for the explanation (I missed the fact that issuperset
does the job)!
comment:24 in reply to: ↑ 21 ; follow-up: ↓ 25 Changed 2 months ago by
Replying to soehms:
There is no mechanism to disable the actual feature in the Sage runtime. What
xyz.is_present()
does is not affected by the present ticket.Perhaps I have in mind a new option
sage -t --hide=FEATURES
which would imply--optional='!FEATURE1,!FEATURE2,..'
. Is there something against having such an additional option?
Yes, I think something like this could make sense if there's a strong enough use case for it. It would probably be best to provide this mechanism also for non-doctest situations. So this would be a separate ticket.
Thanks for reviewing!
comment:25 in reply to: ↑ 24 ; follow-up: ↓ 28 Changed 2 months ago by
Replying to mkoeppe:
Replying to soehms:
There is no mechanism to disable the actual feature in the Sage runtime. What
xyz.is_present()
does is not affected by the present ticket.Perhaps I have in mind a new option
sage -t --hide=FEATURES
which would imply--optional='!FEATURE1,!FEATURE2,..'
. Is there something against having such an additional option?Yes, I think something like this could make sense if there's a strong enough use case for it.
The use case I'm thinking of is the following: If you have installed an optional package xyz
and run a test on your changes, say
sage -t src/sage/....py
then it might happen that you get All tests passed!
here. But after
make xyz-clean
the same invocation of sage -t
could find failures. Probably we will get more situations like that as modularization goes ahead. Surely, the patchbots will detect these. But it will be more convenient if you can detect these failures easily on your own system. I don't know how difficult it would be to fake the absence of an optional package in general. But in all cases where the Features
functionality is used consequently, it should be doable.
It would probably be best to provide this mechanism also for non-doctest situations. So this would be a separate ticket.
Of course! After my vacation (that is in a couple of weeks) I will try to figure out how this could be done.
comment:26 Changed 2 months ago by
- Branch changed from u/mkoeppe/sage__t___optional__sage__feature_ to ba86146e5a5e3308026aac49d87d2716a80c6a93
- Resolution set to fixed
- Status changed from positive_review to closed
comment:27 Changed 8 weeks ago by
- Commit ba86146e5a5e3308026aac49d87d2716a80c6a93 deleted
Thanks for this ticket. It will be quite useful for example to test that sage -t
works even when let's say there is no latex on the machine (which is the case for the computer on which Volker makes tests before merging tickets).
comment:28 in reply to: ↑ 25 Changed 5 weeks ago by
Replying to soehms:
Replying to mkoeppe:
Replying to soehms:
There is no mechanism to disable the actual feature in the Sage runtime. What
xyz.is_present()
does is not affected by the present ticket.Perhaps I have in mind a new option
sage -t --hide=FEATURES
which would imply--optional='!FEATURE1,!FEATURE2,..'
. Is there something against having such an additional option?Yes, I think something like this could make sense if there's a strong enough use case for it.
The use case I'm thinking of is the following: If you have installed an optional package
xyz
and run a test on your changes, saysage -t src/sage/....pythen it might happen that you get
All tests passed!
here. But aftermake xyz-cleanthe same invocation of
sage -t
could find failures. Probably we will get more situations like that as modularization goes ahead. Surely, the patchbots will detect these. But it will be more convenient if you can detect these failures easily on your own system. I don't know how difficult it would be to fake the absence of an optional package in general. But in all cases where theFeatures
functionality is used consequently, it should be doable.It would probably be best to provide this mechanism also for non-doctest situations. So this would be a separate ticket.
Of course! After my vacation (that is in a couple of weeks) I will try to figure out how this could be done.
see #34185!
New commits:
sage -t: Handle --optional=!FEATURE