Opened 11 months ago

Closed 9 months ago

Last modified 9 months ago

#32332 closed enhancement (fixed)

using argparse in sage-runtests script

Reported by: chapoton Owned by:
Priority: major Milestone: sage-9.5
Component: scripts Keywords:
Cc: tscrim, slelievre, gh-kliem, arojas Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: c87148b (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by chapoton)

instead of the deprecated optparse

This was rather simple, but needs testing.

also full flake8 cleanup for the modified file

Change History (42)

comment:1 Changed 11 months ago by chapoton

  • Branch set to u/chapoton/32332
  • Commit set to 32d10d1ab718b78cbcfefaf8f976a8f72865bdc8
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

32d10d1use argparse in sage-runtests

comment:2 Changed 11 months ago by git

  • Commit changed from 32d10d1ab718b78cbcfefaf8f976a8f72865bdc8 to 8c92fc443c0f2c816564d804cb3ac893d5889344

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

8c92fc4simplify GCAction in sage-runtests

comment:3 Changed 11 months ago by chapoton

  • Cc tscrim slelievre gh-kliem added

comment:4 Changed 11 months ago by tscrim

  • Milestone changed from sage-9.4 to sage-9.5
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

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

comment:5 Changed 10 months ago by vbraun

  • Status changed from positive_review to needs_work
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

comment:6 Changed 10 months ago by git

  • Commit changed from 8c92fc443c0f2c816564d804cb3ac893d5889344 to 1d3dfc38b6f75c0783763e6aa52d568f359678e7

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

0d148cbMerge branch 'u/chapoton/32332' in 9.5.b0
1d3dfc3fix for argparse when using -p

comment:7 Changed 10 months ago by git

  • Commit changed from 1d3dfc38b6f75c0783763e6aa52d568f359678e7 to 4aa95ed2d10329290772e1b7e67354153e5c43e5

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

4aa95edanother fix for the logfile

comment:8 Changed 10 months ago by chapoton

  • Status changed from needs_work to needs_review

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

comment:9 Changed 10 months ago by git

  • Commit changed from 4aa95ed2d10329290772e1b7e67354153e5c43e5 to 8a458aa5b9513400251dd58858b199cdbb83f484

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

8a458aabetter handling for -p

comment:10 Changed 10 months ago by tscrim

  • Status changed from needs_review to needs_work

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

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

comment:12 Changed 10 months ago by git

  • Commit changed from 8a458aa5b9513400251dd58858b199cdbb83f484 to b6f8162e7a78df3f7479c3305a243981587f7006

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

b6f8162little tweak in argparse for runtests

comment:13 in reply to: ↑ 11 Changed 10 months ago by 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.

comment:14 Changed 10 months ago by git

  • Commit changed from b6f8162e7a78df3f7479c3305a243981587f7006 to 0c556408ff92446a738f13b421b21f997e3e5f13

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

0c55640use argparse in sage-runtests

comment:15 Changed 10 months ago by 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.

comment:16 Changed 10 months ago by git

  • Commit changed from 0c556408ff92446a738f13b421b21f997e3e5f13 to 3a9358337afbf13e28ae769308cf0e992b571905

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

3a93583better handling short and warn-long

comment:17 Changed 10 months ago by 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 [...]

comment:18 Changed 10 months ago by 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 

comment:19 Changed 10 months ago by git

  • Commit changed from 3a9358337afbf13e28ae769308cf0e992b571905 to 7328505eff557af40d5a9f1fb32ac5d1a8c9d24e

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

7328505adding custom treatment

comment:20 Changed 10 months ago by git

  • Commit changed from 7328505eff557af40d5a9f1fb32ac5d1a8c9d24e to c3dc88c9d9974339b58f9a983fbd8a53904d9188

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

c3dc88cuse argparse in sage-runtests

comment:21 Changed 10 months ago by chapoton

  • Status changed from needs_work to needs_review

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

comment:22 Changed 10 months ago by 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.

comment:23 Changed 10 months ago by git

  • Commit changed from c3dc88c9d9974339b58f9a983fbd8a53904d9188 to dd18675b44c31d387d96caff5646046af9fdde4b

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

dd18675fix

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

comment:25 in reply to: ↑ 24 Changed 10 months ago by 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

comment:26 Changed 10 months ago by git

  • Commit changed from dd18675b44c31d387d96caff5646046af9fdde4b to d057ee2d3263e4aca9f4a26a763364a616ed2a0d

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

d057ee2another try for argparse of runtests

comment:27 Changed 10 months ago by chapoton

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

comment:28 Changed 10 months ago by 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.

comment:29 Changed 10 months ago by chapoton

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

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

comment:30 Changed 10 months ago by git

  • Commit changed from d057ee2d3263e4aca9f4a26a763364a616ed2a0d to 04e40ffba2f944c8369cc48f3b5aeef6fc409bfc

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

66083fdMerge branch 'u/chapoton/32332' in 9.5.b1
04e40fffine tuning short and warn-long options

comment:31 Changed 10 months ago by 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).

comment:32 Changed 9 months ago by git

  • Commit changed from 04e40ffba2f944c8369cc48f3b5aeef6fc409bfc to 84471a3d11c198c23fd59ced9815069181bb5f58

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

84471a3fix for --short

comment:33 Changed 9 months ago by 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.

comment:34 Changed 9 months ago by tscrim

  • Status changed from needs_review to positive_review

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

comment:35 Changed 9 months ago by vbraun

  • Status changed from positive_review to needs_work
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

comment:36 Changed 9 months ago by git

  • Commit changed from 84471a3d11c198c23fd59ced9815069181bb5f58 to c87148b86fe0de11be68a0f01917e45b705e4c83

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

c87148bfixing logfile and warn-long handling

comment:37 Changed 9 months ago by chapoton

  • Status changed from needs_work to needs_review

damn. I have fixed a few things again.

comment:38 Changed 9 months ago by tscrim

  • Status changed from needs_review to positive_review

Green bot and passes locally.

comment:39 Changed 9 months ago by vbraun

  • Branch changed from u/chapoton/32332 to c87148b86fe0de11be68a0f01917e45b705e4c83
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:40 Changed 9 months ago by arojas

  • Commit c87148b86fe0de11be68a0f01917e45b705e4c83 deleted

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 ''

comment:41 Changed 9 months ago by arojas

  • Cc arojas added

comment:42 Changed 9 months ago by arojas

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

Note: See TracTickets for help on using tickets.