using argparse in sage-runtests script

[chapoton]

instead of the deprecated optparse

This was rather simple, but needs testing.

also full flake8 cleanup for the modified file

[chapoton]

New commits:

​32d10d1

use argparse in sage-runtests

[git]

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

​8c92fc4

simplify GCAction in sage-runtests

[tscrim]

LGTM as far as I can tell. A good ticket for the initial 9.5.beta0 release.

[vbraun]

make ptestlong
[...]
make[1]: Leaving directory '/home/release/Sage'
./sage -t -p --all --long --logfile=logs/ptestlong.log
usage: sage -t [options] filenames
sage-runtests: error: argument -p/--nthreads: expected one argument
make: *** [Makefile:201: ptestlong] Error 2

[git]

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

​0d148cb	Merge branch 'u/chapoton/32332' in 9.5.b0
​1d3dfc3	fix for argparse when using -p

[git]

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

​4aa95ed

another fix for the logfile

[chapoton]

ok, should be better now. I fixed the behaviour of -p and --logfile.

[git]

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

​8a458aa

better handling for -p

[tscrim]

There are still issues:

$ ./sage -btp src/sage/combinat/partition.py
...
sage-runtests: error: argument -p/--nthreads: invalid int value: 'src/sage/combinat/partition.py'
$ ./sage -tp src/sage/combinat/partition.py 
usage: sage -t [options] filenames
sage-runtests: error: argument -p/--nthreads: invalid int value: 'src/sage/combinat/partition.py'

I at least use these shortcuts a lot, but I guess that is not a strict requirement to have them fixed. It would just mean a lot of relearning for me.

These both continue to not work (so they don't need to be fixed)

$ ./sage -btp8 src/sage/combinat/partition.py 
Error: unknown option: -btp8
$ ./sage -tp8 src/sage/combinat/partition.py 
Error: unknown option: -tp8

but this now works (before it didn't):

$ ./sage -t -p8 src/sage/combinat/partition.py

[chapoton]

This may be difficult to fix. What's the point of tp alone, given that the default value for nthreads is 1 ? This amounts to say, parallel, but not parallel.

Note that ./sage -tp 8 src/sage/combinat/partition.py works fine, and same for btp.

There is an inherent ambiguity in the options

-p filename.py

that argparse cannot really handle, if I understand correctly.

[git]

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

​b6f8162

little tweak in argparse for runtests

[tscrim]

Replying to chapoton:

This may be difficult to fix. What's the point of tp alone, given that the default value for nthreads is 1 ? This amounts to say, parallel, but not parallel.

From what I can tell, it does run it in parallel.

[git]

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

​0c55640

use argparse in sage-runtests

[chapoton]

Now the behaviour is

no option -p   =>   nthreads = 1
-p             =>   nthreads = 0
-p k           =>   nthreads = k

But I still do not see how to fix your concern.

[git]

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

​3a93583

better handling short and warn-long

[chapoton]

I would see a solution to avoid breaking sage -tp file.py

but at the prize of breaking

sage -tp 4 [...]

The idea is to always add the argument 0 to -p when using -tp or -btp.

So what would you prefer ?

People would still be able to use

sage -t -p 4 [...]

[chapoton]

We have the same issue with other options

~/sage$ sage -t --short src/sage/combinat/partition.py 
either use --new or --all or some filenames
~/sage$ sage -t --warn-long src/sage/combinat/partition.py 
either use --new or --all or some filenames

A workaround seems to be

~/sage$ sage -t --warn-long -- src/sage/combinat/partition.py 

[git]

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

​7328505

adding custom treatment

[git]

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

​c3dc88c

use argparse in sage-runtests

[chapoton]

I think I have found a solution. Maybe not perfect.

[tscrim]

If we have to make a choice, I personally would prefer -tp and -btp to work, but that is because it is what I most frequently use. Does your solution keep both input formats?

What about .pyx (and probably .pxd and .pxi) files? I think those also need to be added to the list.

[git]

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

​dd18675

fix

[chapoton]

Thanks for the suggestion about pyx. I have added that.

I think everything works smoothly now.

What would still fail would be something like

sage -tp src/sage/combinat/

which can easily be obtained as

sage -tp 0 src/sage/combinat

[mkoeppe]

Replying to chapoton:

What would still fail would be something like

sage -tp src/sage/combinat/

which can easily be obtained as

sage -tp 0 src/sage/combinat

-1 on making such incompatible changes, it will be annoying to developers

[git]

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

​d057ee2

another try for argparse of runtests

[chapoton]

Here is a new proposal. I tried to keep things simple.

[tscrim]

Thank you. We are getting close.

$ ./sage -tp --warn-long src/sage/rings/function_field/function_field.py

works, but this does not (although it used to)

$ ./sage -tp src/sage/rings/function_field/function_field.py --warn-long
too few successful tests, not using stored timings
Running doctests with ID 2021-09-13-14-49-55-02c83a90.
Git branch: u/chapoton/32332
Using --optional=bliss,build,debian,dochtml,dot2tex,e_antic,libnauty,ninja_build,normaliz,pip,sage,sage_spkg
Traceback (most recent call last):
  File "/home/uqtscrim/sage-build/src/bin/sage-runtests", line 149, in <module>
    err = DC.run()
  File "/home/uqtscrim/sage-build/local/lib/python3.9/site-packages/sage/doctest/control.py", line 1204, in run
    self.expand_files_into_sources()
  File "/home/uqtscrim/sage-build/local/lib/python3.9/site-packages/sage/doctest/control.py", line 788, in expand_files_into_sources
    self.sources = [FileDocTestSource(path, self.options) for path in expand()]
  File "/home/uqtscrim/sage-build/local/lib/python3.9/site-packages/sage/doctest/control.py", line 788, in <listcomp>
    self.sources = [FileDocTestSource(path, self.options) for path in expand()]
  File "/home/uqtscrim/sage-build/local/lib/python3.9/site-packages/sage/doctest/sources.py", line 527, in __init__
    raise ValueError("unknown file extension %r"%ext)
ValueError: unknown file extension ''

Although I could very easily argue that this is not supported behavior, so it might be more luck than anything it worked before. This is a very minor point and not something I would withhold a positive review for.

[chapoton]

I do not think that we should allow options after the filenames.

The usage is "options then filenames", and that's all.

[git]

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

​66083fd	Merge branch 'u/chapoton/32332' in 9.5.b1
​04e40ff	fine tuning short and warn-long options

[tscrim]

Okay, then let's not (unofficially) support that input format anymore.

Shouldn't --short take floating points? Granted, I find it unlikely that someone will want to do something in 321.0123 seconds, but perhaps for one file they don't want whole seconds? The output for --short has also become a lot more verbose. (Not that that is a bad thing, it is just a change in behavior.)

Anyways, this should hopefully be the last of it. The sooner we can get this merged and out for people to test it more in the wild, the better we can pick up other changes in behavior (that people care about).

[git]

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

​84471a3

fix for --short

[chapoton]

ok, short was indeed broken by the change of default value. Repaired now.

Short was taking integers and will still take integers. I would prefer not to change that.

[tscrim]

Okay, thank you. Let's get this tested by the rest of the developers.

[vbraun]

sage -t --long --warn-long 44.9 --random-seed=0 src/sage/doctest/test.py  # 21 doctests failed
sage -t --long --warn-long 44.9 --random-seed=0 src/sage/doctest/forker.py  # 11 doctests failed
sage -t --long --warn-long 44.9 --random-seed=0 src/sage/doctest/control.py  # Bad exit: 1 after testing finished

[git]

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

​c87148b

fixing logfile and warn-long handling

[chapoton]

damn. I have fixed a few things again.

[tscrim]

Green bot and passes locally.

[arojas]

This breaks running sage -t file --optional=foo for me, regardless of the optional tags I specify

> sage -t /usr/lib/python3.9/site-packages/sage/sets/integer_range.py --optional=sage
too many failed tests, not using stored timings
Running doctests with ID 2021-10-12-12-41-27-6d41dc7c.
Using --optional=dochtml,pip,sage
Traceback (most recent call last):
  File "/usr/bin/sage-runtests", line 151, in <module>
    err = DC.run()
  File "/usr/lib/python3.9/site-packages/sage/doctest/control.py", line 1210, in run
    self.expand_files_into_sources()
  File "/usr/lib/python3.9/site-packages/sage/doctest/control.py", line 794, in expand_files_into_sources
    self.sources = [FileDocTestSource(path, self.options) for path in expand()]
  File "/usr/lib/python3.9/site-packages/sage/doctest/control.py", line 794, in <listcomp>
    self.sources = [FileDocTestSource(path, self.options) for path in expand()]
  File "/usr/lib/python3.9/site-packages/sage/doctest/sources.py", line 527, in __init__
    raise ValueError("unknown file extension %r"%ext)
ValueError: unknown file extension ''

[arojas]

Nevermind, I see this is intentional (re comment 29)