Opened 3 months ago

Closed 2 months ago

Last modified 5 weeks ago

#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:

Status badges

Description (last modified by mkoeppe)

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 mkoeppe

  • Summary changed from sage -t --optional=sage,!FEATURE to sage -t --optional='sage,!FEATURE'

comment:2 Changed 3 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Description modified (diff)

comment:3 Changed 3 months ago by mkoeppe

  • Branch set to u/mkoeppe/sage__t___optional__sage__feature_

comment:4 Changed 3 months ago by mkoeppe

  • Cc klee mjo added
  • Commit set to 800032050b15c7b743635b7c08892375f7f5c563
  • Status changed from new to needs_review

New commits:

8000320sage -t: Handle --optional=!FEATURE

comment:5 Changed 3 months ago by mkoeppe

  • Description modified (diff)

comment:6 Changed 3 months ago by git

  • Commit changed from 800032050b15c7b743635b7c08892375f7f5c563 to d0a52ed409ed07ae060c305f7481ab41479d3403

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

d0a52edsrc/bin/sage-runtests: Update documentation of --optional

comment:7 Changed 3 months ago by mkoeppe

  • Cc fbissey added

comment:8 Changed 3 months ago by mjo

How about respecting --disable-bliss instead?

comment:9 follow-up: Changed 3 months ago by mkoeppe

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.

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

comment:10 follow-up: Changed 3 months ago by 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?

comment:11 in reply to: ↑ 9 ; follow-ups: Changed 3 months ago by mjo

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 mkoeppe

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 mkoeppe

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 mkoeppe

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 mkoeppe

  • Description modified (diff)

comment:16 Changed 3 months ago by mkoeppe

  • Cc slabbe added

comment:17 follow-up: Changed 3 months ago by soehms

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 git

  • Commit changed from d0a52ed409ed07ae060c305f7481ab41479d3403 to ba86146e5a5e3308026aac49d87d2716a80c6a93

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b1ce8d6sage -t: Handle --optional=!FEATURE
a670ae8src/bin/sage-runtests: Update documentation of --optional
ba86146src/bin/sage-runtests --help: Say that ! needs to be quoted

comment:19 Changed 2 months ago by 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.

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: Changed 2 months ago by mkoeppe

Replying to soehms:

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?

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: Changed 2 months ago by soehms

  • 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: Changed 2 months ago by 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 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 soehms

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 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.)

Thanks for the explanation (I missed the fact that issuperset does the job)!

comment:24 in reply to: ↑ 21 ; follow-up: Changed 2 months ago by 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. 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: Changed 2 months ago by 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, 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 vbraun

  • 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 slabbe

  • 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 soehms

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, 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.

see #34185!

Note: See TracTickets for help on using tickets.