#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: |
Description (last modified by )
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
- Branch set to u/chapoton/32332
- Commit set to 32d10d1ab718b78cbcfefaf8f976a8f72865bdc8
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 11 months ago by
- Commit changed from 32d10d1ab718b78cbcfefaf8f976a8f72865bdc8 to 8c92fc443c0f2c816564d804cb3ac893d5889344
Branch pushed to git repo; I updated commit sha1. New commits:
8c92fc4 | simplify GCAction in sage-runtests
|
comment:3 Changed 11 months ago by
- Cc tscrim slelievre gh-kliem added
comment:4 Changed 11 months ago by
- 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
- 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
- Commit changed from 8c92fc443c0f2c816564d804cb3ac893d5889344 to 1d3dfc38b6f75c0783763e6aa52d568f359678e7
comment:7 Changed 10 months ago by
- Commit changed from 1d3dfc38b6f75c0783763e6aa52d568f359678e7 to 4aa95ed2d10329290772e1b7e67354153e5c43e5
Branch pushed to git repo; I updated commit sha1. New commits:
4aa95ed | another fix for the logfile
|
comment:8 Changed 10 months ago by
- 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
- Commit changed from 4aa95ed2d10329290772e1b7e67354153e5c43e5 to 8a458aa5b9513400251dd58858b199cdbb83f484
Branch pushed to git repo; I updated commit sha1. New commits:
8a458aa | better handling for -p
|
comment:10 Changed 10 months ago by
- 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: ↓ 13 Changed 10 months ago by
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
- Commit changed from 8a458aa5b9513400251dd58858b199cdbb83f484 to b6f8162e7a78df3f7479c3305a243981587f7006
Branch pushed to git repo; I updated commit sha1. New commits:
b6f8162 | little tweak in argparse for runtests
|
comment:13 in reply to: ↑ 11 Changed 10 months ago by
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
- Commit changed from b6f8162e7a78df3f7479c3305a243981587f7006 to 0c556408ff92446a738f13b421b21f997e3e5f13
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0c55640 | use argparse in sage-runtests
|
comment:15 Changed 10 months ago by
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
- Commit changed from 0c556408ff92446a738f13b421b21f997e3e5f13 to 3a9358337afbf13e28ae769308cf0e992b571905
Branch pushed to git repo; I updated commit sha1. New commits:
3a93583 | better handling short and warn-long
|
comment:17 Changed 10 months ago by
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
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
- Commit changed from 3a9358337afbf13e28ae769308cf0e992b571905 to 7328505eff557af40d5a9f1fb32ac5d1a8c9d24e
Branch pushed to git repo; I updated commit sha1. New commits:
7328505 | adding custom treatment
|
comment:20 Changed 10 months ago by
- Commit changed from 7328505eff557af40d5a9f1fb32ac5d1a8c9d24e to c3dc88c9d9974339b58f9a983fbd8a53904d9188
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c3dc88c | use argparse in sage-runtests
|
comment:21 Changed 10 months ago by
- 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
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
- Commit changed from c3dc88c9d9974339b58f9a983fbd8a53904d9188 to dd18675b44c31d387d96caff5646046af9fdde4b
Branch pushed to git repo; I updated commit sha1. New commits:
dd18675 | fix
|
comment:24 follow-up: ↓ 25 Changed 10 months ago by
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
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
- Commit changed from dd18675b44c31d387d96caff5646046af9fdde4b to d057ee2d3263e4aca9f4a26a763364a616ed2a0d
Branch pushed to git repo; I updated commit sha1. New commits:
d057ee2 | another try for argparse of runtests
|
comment:27 Changed 10 months ago by
Here is a new proposal. I tried to keep things simple.
comment:28 Changed 10 months ago by
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
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
- Commit changed from d057ee2d3263e4aca9f4a26a763364a616ed2a0d to 04e40ffba2f944c8369cc48f3b5aeef6fc409bfc
comment:31 Changed 10 months ago by
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
- Commit changed from 04e40ffba2f944c8369cc48f3b5aeef6fc409bfc to 84471a3d11c198c23fd59ced9815069181bb5f58
Branch pushed to git repo; I updated commit sha1. New commits:
84471a3 | fix for --short
|
comment:33 Changed 9 months ago by
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
- 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
- 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
- Commit changed from 84471a3d11c198c23fd59ced9815069181bb5f58 to c87148b86fe0de11be68a0f01917e45b705e4c83
Branch pushed to git repo; I updated commit sha1. New commits:
c87148b | fixing logfile and warn-long handling
|
comment:37 Changed 9 months ago by
- Status changed from needs_work to needs_review
damn. I have fixed a few things again.
comment:38 Changed 9 months ago by
- Status changed from needs_review to positive_review
Green bot and passes locally.
comment:39 Changed 9 months ago by
- 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
- 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
- Cc arojas added
comment:42 Changed 9 months ago by
Nevermind, I see this is intentional (re comment 29)
New commits:
use argparse in sage-runtests