Opened 4 years ago

Closed 3 years ago

#26740 closed enhancement (fixed)

create a python3 test make target

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: jdemeyer, tscrim, fbissey, embray, vbraun Merged in:
Authors: Frédéric Chapoton Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: f9fbbd0 (Commits, GitHub, GitLab) Commit: f9fbbd05fff30a1936a11c3bc743d13a4dbd0ef0
Dependencies: Stopgaps:

Status badges

Description

to prevent regression by testing submodules already known to be ok.

Attachments (1)

small-log.txt (26.8 KB) - added by chapoton 4 years ago.

Download all attachments as: .zip

Change History (53)

comment:1 Changed 4 years ago by chapoton

  • Branch set to public/ticket/26740
  • Commit set to 9268f9df6b04612cece9531505087c91d32f3bae
  • Status changed from new to needs_review

New commits:

9268f9dpy3: create a python3 test make target

comment:2 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

errors when not using this option..

comment:3 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

Oh, well, this seems to work correctly. Feedback would be appreciated.

comment:4 Changed 4 years ago by fbissey

I think that's a lofty goal but I have reservations until I understand how it impact and work with distro (like me). What happens when you hit this section with python2?

@@ -690,6 +690,13 @@ class DocTestController(SageObject):
             Doctesting the Sage notebook.
             sage: DC.files[0][-6:]  # py2
             'sagenb'
+
+        ::
+
+            sage: DD = DocTestDefaults(python3 = True)
+            sage: DC = DocTestController(DD, [])
+            sage: DC.add_files()
+            Doctesting modules compatible with Python 3
         """
         opj = os.path.join
         from sage.env import SAGE_SRC, SAGE_DOC_SRC, SAGE_ROOT

As a distro with sage installed for python2 and python3 do I have to run something like S{sage-python3} -t --python3 --long, but not --all, to run the appropriate doctests?

comment:5 Changed 4 years ago by chapoton

There is no reason for you to worry at all. It has no effect on the distro side, as far as I can tell.

This introduces a new option "--python3" when running the doctests. Maybe a better name would be "--python3only" or "--python3-valid-modules-only".

The only effect of using this option is to select a pre-defined list of modules whose doctests will be launched.

If you use the option with sage2, it will just test those files.

If you use this option with sage3, exactly the same.

The crop in your comment just doctests that this option exists and works. It does not launch the associated behaviour.

The goal of this ticket is to allow our release manager to have a builtbot that checks that no regression happens, namely that once a module is ok for python3, it will be checked to remain so.

Last edited 4 years ago by chapoton (previous) (diff)

comment:6 Changed 4 years ago by fbissey

Thank you for taking the time to answer. Just to be sure, I could run the test as I suggested on a distro to check for regression or problem with my distro regarding python3?

comment:7 Changed 4 years ago by chapoton

Right, if you provide sage3 on your distro, you could indeed use this option to check that all these tests do pass as they should.

comment:8 Changed 4 years ago by git

  • Commit changed from 9268f9df6b04612cece9531505087c91d32f3bae to 6b743fd3c413af9e9c69168fbe837c9360d2e9c8

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

6b743fdtrac 26740 fix

comment:9 Changed 4 years ago by embray

I don't really see the point. Just let them fail. If regressions happen they will usually be small, and will be reduced over time. Before long hopefully there will, or should, be none, in which case this will be a useless feature.

What I think would be more useful, which is something I have wanted for other purposes as well, is a way to provide ./sage -t -a a custom list of "known failures" which could be provided on a machine by machine basis (e.g. known failures for a specific platform being tested on). This could be passed, for example, to any Python 3 builders if you wanted. It would also be helpful to know when tests that are expected to fail start passing :)

Last edited 4 years ago by embray (previous) (diff)

comment:10 Changed 4 years ago by chapoton

The point is to move the responsability of fixing the new failing doctests from me (or should I say us?) to the ticket owners..

comment:11 Changed 4 years ago by embray

I'm not opposed to that of course, but I think a more generic way to list known-failures or exclusions would be better. This also obviates the need for a special make target since this can just be passed in the TEST_OPTS variable.

comment:12 Changed 4 years ago by vbraun

  • Status changed from needs_review to positive_review

Being able to pass in a list of known failures might be useful, feel free to open a ticket for that. Until then: The perfect is the enemy of the good.

comment:13 Changed 4 years ago by vbraun

  • Reviewers set to Volker Braun

comment:14 Changed 4 years ago by embray

  • Status changed from positive_review to needs_work

I disagree--this can very easily be improved. Just give me a day or two.

comment:15 Changed 4 years ago by embray

All that's required is to remove the "python 3" specific stuff, and change that option to be able to read a list of filenames/directories (possibly optionally from a file as well). We could even add a file listing known Python 3 failures to the repository which I wouldn't mind, but it's silly to hard-code this when a few small changes can upgrade it to a more generally useful feature that won't just have to be ripped out later when it becomes obsolete.

comment:16 Changed 4 years ago by vbraun

OK, you have 2 days. And make sure that the known python3 failures are in the repo so I don't have to manually maintain them on a dozen buildbots.

comment:17 Changed 4 years ago by embray

I mean, I don't know what you mean by "a dozen buildbots" as that's a setting that can just go in the builder settings for Python 3 builds, which is one place. The builder settings have to be updated either way. But I'm happy to maintain a list in the repo for now since they can be scratched off alongside changes that fix them.

comment:18 Changed 4 years ago by vbraun

Thanks, looking forward to having a py3 buildbot...

comment:19 Changed 4 years ago by embray

It occurs to me I initially misread the patch on this ticket--I thought the hard-coded python3_files was for some reason a list of known failures to exclude, but in fact it's a list of sub-packages that are known passing. In which case this could have been done without adding anything to the doctest runner. Rather, just put this list in a file or something and (as a compromise, for simplicity's sake) keep the separate makefile target and do the rough equivalent of ./sage -t --long $(cat list-of-known-passing.txt).

However, I still think having the converse--an ability to list known failures--would be very useful in general.

comment:20 Changed 4 years ago by embray

Frédéric, does your list of "known passing" include with flags like --long? Or are we not worrying about that for now? I'm just curious because I want to see about generating the converse list, of test modules with known failures.

Changed 4 years ago by chapoton

comment:21 Changed 4 years ago by chapoton

I have attached the list of failing files in 8.5.b5. This is the result of a patchbot run, so it was done with "--long" but not using any other optional option.

EDIT: There is a script attached to #26212 that takes a patchbot log and extract useful information, including the list of failing modules.

Last edited 4 years ago by chapoton (previous) (diff)

comment:22 Changed 4 years ago by embray

Thanks. Interestingly, I just ran the tests against current develop branch without --long and got 309 failing files: more than the log you attached. Perhaps YMMV depending on your system, or the order in which the test were run. Still, pretty good! A lot of progress (~150 more passing) since I last tried.

comment:23 Changed 4 years ago by embray

Please see #26785 and its companion #26784.

One minor concern I have is that because this list of "known failures" is more fine-grained than the list of "expected passes" in this ticket, it could be a little more difficult to manage especially w.r.t. merge conflicts. I would like to give it a try though. Merge conflicts shouldn't be too much trouble since we want to mostly be removing lines from the file and never adding them.

comment:24 Changed 4 years ago by chapoton

I am not very happy (for the present task) with the idea that Erik proposed to store a list of failing files. And I dislike even more a list of failing files than a list of failing modules. My only aim here (yes it's scaffolding, but that is useful and can be removed once used) is to prevent regression by using this make target as a lock.

Storing and testing a list of successful python-3 modules has two advantages : it needs much less maintenance work, as it will change less often. And we do not want to run for nothing the doctests that will be failing.

Maybe other opinions would help to arbitrate (@jdemeyer, @fbissey). If somebody else review #26785 and #26784, I will certainly not object in any manner.

comment:25 Changed 4 years ago by embray

I don't really understand your point--this accomplishes the same purpose. It's still effectively storing a list of "successful" modules in the form of the list of all tests, minus those that are known failures.

And we do not want to run for nothing the doctests that will be failing.

It's not for nothing: In fact it can inform you when some files' tests start passing e.g. due to something that was fixed.

comment:26 Changed 4 years ago by embray

I thought I made this point elsewhere, but should add your patch could be implemented as a file like python3-known-passing.txt:

src/sage/foo
src/sage/bar
src/sage/whatever
...
$ cat python3-known-passing.txt | xargs ./sage -t --long -p

No need for special "hacks".

comment:27 Changed 4 years ago by vbraun

So how about turning your one-liner into a makefile target? Both positive and negative lists can be useful for different use cases...

comment:28 Changed 4 years ago by chapoton

  • Milestone changed from sage-8.5 to sage-8.7

comment:29 Changed 3 years ago by chapoton

  • Branch changed from public/ticket/26740 to u/chapoton/26740
  • Commit changed from 6b743fd3c413af9e9c69168fbe837c9360d2e9c8 to 1cdda8f79604b1518c1b9aa20ea74157316c22d3

another tentative, as suggested by Erik


New commits:

1cdda8fpy3: create a python3 test make target

comment:30 Changed 3 years ago by git

  • Commit changed from 1cdda8f79604b1518c1b9aa20ea74157316c22d3 to 5fe82a376061f26ada95d206826e6e8ebc993b83

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

5fe82a3py3: create a python3 test make target

comment:31 Changed 3 years ago by chapoton

  • Status changed from needs_work to needs_review

ok, ready for review

comment:32 Changed 3 years ago by jdemeyer

Why is python3-known-passing.txt one long line? Isn't the whole point of xargs to support a file with one filename per line?

comment:33 Changed 3 years ago by chapoton

Because this is the (only) way I managed to get it work. If you feel able to do better, please help!

comment:34 Changed 3 years ago by chapoton

it turns out that one can split the lines, coming soon

comment:35 Changed 3 years ago by git

  • Commit changed from 5fe82a376061f26ada95d206826e6e8ebc993b83 to 2ea46789ab0b31fda19a3fb9cd29f396bcb1dd61

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

2ea4678py3: create a python3 test make target

comment:36 Changed 3 years ago by chapoton

done. Better like that ?

comment:37 Changed 3 years ago by embray

Just to avoid top-level file proliferation, can this .txt file go in misc/ or something?

comment:38 Changed 3 years ago by chapoton

Just tell me where exactly I should put that and I will do so..

comment:39 Changed 3 years ago by jhpalmieri

Maybe create a file in src/ext/doctest/?

comment:40 Changed 3 years ago by git

  • Commit changed from 2ea46789ab0b31fda19a3fb9cd29f396bcb1dd61 to f9fbbd05fff30a1936a11c3bc743d13a4dbd0ef0

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

f9fbbd0move the python3-known-passing file

comment:41 Changed 3 years ago by chapoton

done, thanks

comment:42 Changed 3 years ago by chapoton

This works well on my usual python3-test machine, where the selected passing doctests are known to pass since many months. Before putting it on a buildbot, could other people please launch the new make target on python3-sage installations on various machines ? We need to see if some failures happen in these modules in other architectures and os.

comment:43 Changed 3 years ago by jhpalmieri

It worked for me on OS X. On some future ticket, we should add a ptestlong option.

comment:44 Changed 3 years ago by chapoton

Thanks John. Anybody else ? Maybe we can consider that this is ready for positive review ? Volker, what do you think ?

comment:45 Changed 3 years ago by embray

I literally just meant adding a top-level directory called misc/, but either way.

comment:46 Changed 3 years ago by chapoton

would you please try it on Cygwin ?

comment:47 Changed 3 years ago by embray

I would but my Windows machine is currently occupied building the Sage 8.6 release. I'm not sure it's really needed though: There's nothing very special about Python 2 vs Python 3 on Cygwin that I'm aware of as it pertains to Sage.

comment:48 Changed 3 years ago by jdemeyer

I can test on ppc64le

comment:49 Changed 3 years ago by jdemeyer

On Linux sardonis 4.4.0-141-generic #167-Ubuntu SMP Wed Dec 5 10:33:00 UTC 2018 ppc64le ppc64le ppc64le GNU/Linux:

----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 367.5 seconds
    cpu time: 4433.2 seconds
    cumulative wall time: 4408.9 seconds

comment:50 Changed 3 years ago by chapoton

Thanks. Would somebody be brave enough to set a positive review ?

comment:51 Changed 3 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:52 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/26740 to f9fbbd05fff30a1936a11c3bc743d13a4dbd0ef0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.