Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sage -t --optional='sage,!FEATURE' #33823

Closed
mkoeppe opened this issue May 8, 2022 · 31 comments
Closed

sage -t --optional='sage,!FEATURE' #33823

mkoeppe opened this issue May 8, 2022 · 31 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2022

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.

CC: @kwankyu @orlitzky @kiwifb @seblabbe

Component: refactoring

Author: Matthias Koeppe

Branch: ba86146

Reviewer: Sebastian Oehms

Issue created by migration from https://trac.sagemath.org/ticket/33823

@mkoeppe mkoeppe added this to the sage-9.7 milestone May 8, 2022
@mkoeppe mkoeppe changed the title sage -t --optional=sage,!FEATURE sage -t --optional='sage,!FEATURE' May 9, 2022
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

New commits:

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

Commit: 8000320

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2022

Changed commit from 8000320 to d0a52ed

@orlitzky
Copy link
Contributor

orlitzky commented May 9, 2022

comment:8

How about respecting --disable-bliss instead?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

comment:9

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.

@kiwifb
Copy link
Member

kiwifb commented May 9, 2022

comment:10

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?

@orlitzky
Copy link
Contributor

orlitzky commented May 9, 2022

comment:11

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

comment:12

Replying to @orlitzky:

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

comment:13

Let's keep the ticket focused. It is not meant as an invitation to continue your musings from sage-devel

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 9, 2022

comment:14

Replying to @kiwifb:

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.

@mkoeppe

This comment has been minimized.

@soehms
Copy link
Member

soehms commented Jun 1, 2022

comment:17

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Changed commit from d0a52ed to ba86146

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 5, 2022

comment:19

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 5, 2022

comment:20

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.

@soehms
Copy link
Member

soehms commented Jun 8, 2022

Reviewer: Sebastian Oehms

@soehms
Copy link
Member

soehms commented Jun 8, 2022

comment:21

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 8, 2022

comment:22

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

@soehms
Copy link
Member

soehms commented Jun 8, 2022

comment:23

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 8, 2022

comment:24

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!

@soehms
Copy link
Member

soehms commented Jun 10, 2022

comment:25

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.

@vbraun
Copy link
Member

vbraun commented Jun 12, 2022

Changed branch from u/mkoeppe/sage__t___optional__sage__feature_ to ba86146

@seblabbe
Copy link
Contributor

comment:27

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

@seblabbe
Copy link
Contributor

Changed commit from ba86146 to none

@soehms
Copy link
Member

soehms commented Jul 15, 2022

comment:28

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants