Opened 9 years ago

Closed 9 years ago

#12486 closed enhancement (fixed)

Make the Sage patchbot an optional spkg

Reported by: robertwb Owned by: mvngu
Priority: major Milestone: sage-5.3
Component: packages: optional Keywords:
Cc: kini, dkrenn Merged in: sage-5.3.rc1
Authors: Robert Bradshaw, Dan Drake Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by robertwb)

Anyone can run sage --patchbot to contribute.

Instructions:

Attachments (7)

patchbot.patch (81.5 KB) - added by robertwb 9 years ago.
patchbot-root-5.0.patch (564 bytes) - added by ddrake 9 years ago.
for 5.0 series, apply to SAGE_ROOT repo
patchbot-scripts-5.0.patch (81.1 KB) - added by ddrake 9 years ago.
for 5 .0 series, apply to scripts repo
12486-patchbot-scripts-5.0-updated.patch (82.7 KB) - added by kini 9 years ago.
apply to $SAGE_LOCAL/bin
12486-fix-sdist.patch (635 bytes) - added by robertwb 9 years ago.
Apply to $SAGE_LOCAL/bin.
trac_12486_fix_fd_leak.patch (650 bytes) - added by vbraun 9 years ago.
Initial patch
patchbot-root-5.0.quoted.patch (566 bytes) - added by robertwb 9 years ago.

Download all attachments as: .zip

Change History (92)

comment:1 Changed 9 years ago by robertwb

  • Status changed from new to needs_review

Changed 9 years ago by robertwb

comment:2 Changed 9 years ago by ddrake

note that this doesn't apply to 5.0.beta3, since the sage-sage script has been deprecated. It looks like the relevant bit needs to be applied to SAGE_ROOT/spkg/bin/sage for 5.0.

comment:3 Changed 9 years ago by kini

  • Cc kini added

comment:4 Changed 9 years ago by ddrake

  • Description modified (diff)

I split the original patch, since in 5.0, the sage script is tracked by the root repo.

comment:5 Changed 9 years ago by ddrake

We should document the patchbot. Something better than "read the code" and "ask Robert on sage-devel". This is now #12505.

Changed 9 years ago by ddrake

for 5.0 series, apply to SAGE_ROOT repo

comment:6 Changed 9 years ago by ddrake

New patches (1) remove trailing whitespace, (2) include ticket number in commit message, and (3) have proper node and parent IDs, so patchbot knows where to apply them. These changes were prompted by looking at the patchbot's logs for this ticket. :)

comment:7 Changed 9 years ago by dkrenn

  • Cc dkrenn added

comment:8 Changed 9 years ago by davidloeffler

I tried this out (you'll see there are some results from debian/squeeze/sid/x86_64/2.6.32-37-server/fermat on the master patchbot server) and I got some strange behaviour. If you run the bot and give it a ticket number, with the --ticket command-line option, then it tests that ticket, but on finishing the test it gets stuck in an infinite loop of idling roundabout line 178 of patchbot.py (in particular, it never sends the test results back to the server). This is kind of annoying.

comment:9 follow-up: Changed 9 years ago by davidloeffler

Sorry, the issue I reported the last time is a non-issue (you just have to wait a bit longer for the Tee background process to notice that it's finished).

But there is an amusing bug which I spotted when running a patchbot under 5.0.beta10. The function that compares version numbers just splits on "." and sorts alphabetically, and hence it thinks beta1 < beta10 < beta2! This caused a bunch of spurious "Apply Failed" warnings when it tried to apply patches from tickets that had already been merged (listed as dependencies of open tickets). See for example the first of the two runs for ticket #10527.

The fix is to add the lines

a = a.replace("beta", "beta.")
b = b.replace("beta", "beta.")

to util.compare_versions.

Last edited 9 years ago by davidloeffler (previous) (diff)

Changed 9 years ago by ddrake

for 5 .0 series, apply to scripts repo

comment:10 in reply to: ↑ 9 Changed 9 years ago by ddrake

Replying to davidloeffler:

The fix is to add the lines

a = a.replace("beta", "beta.")
b = b.replace("beta", "beta.")

to util.compare_versions.

The updated patch does that. (It also is a bit more strict about catching a ValueError when doing int(s), which probably isn't a big deal, but it seems like a good idea.)

comment:11 Changed 9 years ago by robertwb

Thanks. I noticed that too.

comment:12 Changed 9 years ago by jdemeyer

  • Authors set to Robert Bradshaw, Dan Drake
  • Description modified (diff)

comment:13 Changed 9 years ago by jdemeyer

Since a while now, there is no requirement for the ticket number to be in the commit message, it's added automatically when merging in the standard format

Trac #12486: message from the patch file

comment:14 Changed 9 years ago by kini

I noticed just now that on the buildbot arando, where I am running a patchbot on Sage 5.1.beta1, there were orphaned patchbot testing processes (i.e. instances of the python interpreter) left lying around, running things in ~/.sage/tmp/arando-*/, of which many seemed to be named maxima_abstract_*.py, though there were several other filenames as well. These testing processes were not sleeping or waiting but running full steam, taking up most of arando's CPU time, so I killed them. I've noticed this happen once or twice before, as well.

comment:15 Changed 9 years ago by jdemeyer

kini: I'd love to know which ticket causes the maxima_abstract failures.

comment:16 Changed 9 years ago by jdemeyer

By the way: the "Python keeps running after a doctest timeout" bug is #11338.

comment:17 Changed 9 years ago by kini

[2] patchbot@arando ~/.sage/tmp $ find | grep maxima_abstract | xargs grep "^#.*devel/sage-"
./arando-13446/maxima_abstract_22717.py:# '/opt/patchbot-5.1.beta0/devel/sage-12214/sage/interfaces/maxima_abstract.py'.
./arando-698/maxima_abstract_10001.py:# '/opt/patchbot-5.1.beta0/devel/sage-12120/sage/interfaces/maxima_abstract.py'.
./arando-21967/maxima_abstract_10092.py:# '/opt/patchbot-5.1.beta0/devel/sage-12989/sage/interfaces/maxima_abstract.py'.
./arando-25899/maxima_abstract_2742.py:# '/opt/patchbot-5.1.beta0/devel/sage-12298/sage/interfaces/maxima_abstract.py'.
./arando-25577/maxima_abstract_13608.py:# '/opt/patchbot-5.1.beta0/devel/sage-6812/sage/interfaces/maxima_abstract.py'.
./arando-7379/maxima_abstract_27734.py:# '/opt/patchbot-5.1.beta0/devel/sage-6812/sage/interfaces/maxima_abstract.py'.
./arando-16948/maxima_abstract_3744.py:# '/opt/patchbot-5.1.beta1/devel/sage-12043/sage/interfaces/maxima_abstract.py'.
./arando-5871/maxima_abstract_15144.py:# '/opt/patchbot-5.1.beta0/devel/sage-11726/sage/interfaces/maxima_abstract.py'.
./arando-10252/maxima_abstract_30489.py:# '/opt/patchbot-5.1.beta0/devel/sage-12518/sage/interfaces/maxima_abstract.py'.
./arando-25814/maxima_abstract_13608.py:# '/opt/patchbot-5.1.beta0/devel/sage-6812/sage/interfaces/maxima_abstract.py'.
./arando-21669/maxima_abstract_30915.py:# '/opt/patchbot-5.0/devel/sage-2215/sage/interfaces/maxima_abstract.py'.
./arando-3379/maxima_abstract_22614.py:# '/opt/patchbot-5.1.beta1/devel/sage-12251/sage/interfaces/maxima_abstract.py'.
./arando-23007/maxima_abstract_11131.py:# '/opt/patchbot-5.1.beta0/devel/sage-12989/sage/interfaces/maxima_abstract.py'.
./arando-7740/maxima_abstract_11922.py:# '/opt/patchbot-5.1.beta1/devel/sage-11930/sage/interfaces/maxima_abstract.py'.
./arando-13364/maxima_abstract_22646.py:# '/opt/patchbot-5.1.beta0/devel/sage-11143/sage/interfaces/maxima_abstract.py'.
./arando-12724/maxima_abstract_834.py:# '/opt/patchbot-5.1.beta0/devel/sage-12892/sage/interfaces/maxima_abstract.py'.
./arando-20437/maxima_abstract_29714.py:# '/opt/patchbot-5.1.beta0/devel/sage-11895/sage/interfaces/maxima_abstract.py'.
./arando-5929/maxima_abstract_15208.py:# '/opt/patchbot-5.1.beta0/devel/sage-12352/sage/interfaces/maxima_abstract.py'.
./arando-4951/maxima_abstract_14224.py:# '/opt/patchbot-5.1.beta0/devel/sage-2215/sage/interfaces/maxima_abstract.py'.

I don't know how many of these are files that these stalled processes were running and how many are just other temp files that happened not to have been cleaned up - next time this happens I'll grep ps instead. (The ticket numbers are in the path names of the form *devel/sage-*, not of the form ./arando-*.)

comment:18 Changed 9 years ago by jdemeyer

There is no logic behind this, too many independent tickets have this issue.

comment:19 Changed 9 years ago by jdemeyer

Is there a way how "state" could be preserved across patchbot runs? Could it happen that the patchbot tests ticket X and somehow ticket X has a bug causing failures in following tickets the patchbot tests? Because it looks like this is happening here.

I did 600 test runs of maxima_abstract.py on vanilla sage-5.1.beta2 and found no error. Nobody has reported this bug on the mailing list and I haven't seen it on the buildbot with sage-5.1.beta2 or earlier. So I conclude that the bug is introduced by a not-yet-merged ticket.

Since it is extremely unlikely that all the tickets in kini's list above cause the same bug, the most logical explanation is that somehow one ticket corrupted further patchbot runs.

comment:20 Changed 9 years ago by jdemeyer

For the record, here is the verbose output of the failure:

sage -t --long --verbose "devel/sage/sage/interfaces/maxima_abstract.py"
[...]
Trying:
    ~f###line 2304:_sage_    >>> ~f
Expecting:
    1/sin(x)
**********************************************************************
File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/devel/sage/sage/interfaces/maxima_abstract.py", line ?, in __main__.example_81
Failed example:
    ~f###line 2304:_sage_    >>> ~f
Exception raised:
    Traceback (most recent call last):
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_81[3]>", line 1, in <module>
        ~f###line 2304:_sage_    >>> ~f
      File "element.pyx", line 1833, in sage.structure.element.RingElement.__invert__ (sage/structure/element.c:13904)
      File "element.pyx", line 1799, in sage.structure.element.RingElement.__div__ (sage/structure/element.c:13362)
      File "coerce.pyx", line 744, in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:6704)
      File "coerce.pyx", line 858, in sage.structure.coerce.CoercionModel_cache_maps.canonical_coercion (sage/structure/coerce.c:7892)
      File "map.pyx", line 1282, in sage.categories.map.FormalCompositeMap._call_ (sage/categories/map.c:6190)
      File "morphism.pyx", line 159, in sage.categories.morphism.CallMorphism._call_ (sage/categories/morphism.c:3080)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/interface.py", line 200, in __cal
l__
        return self._coerce_from_special_method(x)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/interface.py", line 226, in _coer
ce_from_special_method
        return (x.__getattribute__(s))(self)
      File "sage_object.pyx", line 527, in sage.structure.sage_object.SageObject._maxima_ (sage/structure/sage_object.c:5444)
        This function may call the magma interpreter when it runs.
      File "sage_object.pyx", line 439, in sage.structure.sage_object.SageObject._interface_ (sage/structure/sage_object.c:3684)
        X = I(s)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/interface.py", line 198, in __call__
        return cls(self, x, name=name)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/maxima.py", line 1155, in __init__
        ExpectElement.__init__(self, parent, value, is_name=False, name=None)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/expect.py", line 1331, in __init__
        self._name = parent._create(value, name=name)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/interface.py", line 388, in _create
        self.set(name, value)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/maxima.py", line 1000, in set
        self._eval_line(cmd)
      File "/padic/scratch/jdemeyer/merger/sage-5.1.beta3/local/lib/python/site-packages/sage/interfaces/maxima.py", line 756, in _eval_line
        assert line_echo.strip() == line.strip()
    AssertionError
[...]
4 items had no tests:
    __main__
    __main__.change_warning_output
    __main__.check_with_tolerance
    __main__.warning_function
85 items passed all tests:
   3 tests in __main__.example_0
   6 tests in __main__.example_1
[...]
   4 tests in __main__.example_43
   4 tests in __main__.example_44
   4 tests in __main__.example_45
   4 tests in __main__.example_46
   4 tesException RuntimeError: RuntimeError('maximum recursion depth exceeded while calling a Python object',) in  ignored
Exception RuntimeError: RuntimeError('maximum recursion depth exceeded while calling a Python object',) in  ignored
Exception RuntimeError: RuntimeError('maximum recursion depth exceeded while calling a Python object',) in  ignored
Exception RuntimeError: RuntimeError('maximum recursion depth exceeded while calling a Python object',) in  ignored
Exception RuntimeError: RuntimeError('maximum recursion depth exceeded while calling a Python object',) in  ignored
Exception RuntimeError: RuntimeError('maximum recursion depth exceeded while calling a Python object',) in  ignored
[...goes on forever until killed...]

comment:21 Changed 9 years ago by jdemeyer

This bug is now prominently in my top 3 of "weirdest Sage bugs I encountered as release manager", together with #12221 and #10609.

Last edited 9 years ago by jdemeyer (previous) (diff)

comment:22 Changed 9 years ago by vbraun

My desktop does about 10 tickets/hour, each of which adds a $SAGE_ROOT/devel/sage-<ticket> directory of about 330MB. Is there a reason why the patchbot does not delete these? It kind of adds up, we are talking about >50 GB per day. Or should I get another harddisk to run the patchbot on?

About the maxima_abstract.py error, I never observed it here. It might be a 32-bit issue (I'm running Fedora 17 x86_64). Although perhaps far-fetched, there is another bug where the sage-maxima interface goes into an infinite recursion (#13097).

Even with these issues I'd be in favor of merging this ticket. Its useful functionality and it works for most. We can polish it in a follow-up ticket.

comment:23 Changed 9 years ago by robertwb

The patchbot deletes tickets directories when they are closed. It might be worth having an option to delete them after every run, but then a full re-clone and re-build would be required to test any additional patches (expensive).

As for space, all the build and .c files are hard linked from sage-main (unless they're changed), so the aggregate use of the directories is about 100MB each ticket (not each run). Even if we got up to, say, 500 open tickets you are looking at a stead state of 50GB total, not per day.

It will only apply patches to the sage-main tree, which it re-clones for every ticket, but the doctests and code itself could do anything, so one can't rule out any global state (though if there it's both strange and a bug).

I'm all in favor of getting this in and iterating on it there. I made some minor changes and will try to refresh these patches shortly.

comment:24 Changed 9 years ago by vbraun

My patchbot processed 236 tickets so far and I'm at 80 gigabytes:

[patchbot@volker-desktop sage-5.1.beta3]$ du -sh .
79G	.
[patchbot@volker-desktop sage-5.1.beta3]$ mount | grep /home
/dev/sdc1 on /home type ext4 (rw,relatime,seclabel,data=ordered)

Thats already accounting for hardlinks, du is smart enough to only count the linked file once. Turning this off (and overcounting) yields

[patchbot@volker-desktop sage-5.1.beta3]$ du -shl .
336G	.

So it should probably stop just below 100G, still acceptable I think.

Last edited 9 years ago by vbraun (previous) (diff)

comment:25 Changed 9 years ago by robertwb

  • Description modified (diff)

comment:26 Changed 9 years ago by robertwb

Hmm... that is quite a bit more than I'd expect. Certainly keeping only the most recently used N tickets would be an improvement, but between moving to git and cycache and ccache it will become irrelevant (one would simply specify the desired cache size) and 100G isn't overkill these days.

comment:27 Changed 9 years ago by davidloeffler

My experience with running patchbot has been that many tickets (notably those that add new modules to the reference manual) do seem to pollute the global state, particularly for documentation building purposes, where the docbuild log for running patchbot on ticket X will start complaining that it can't find a file that is added in some other ticket Y that it previously tested. (I think this is because the sage-clone script hard-links the Sphinx environment pickle.)

Moreover, even without "cross-branch contamination", adding source files and then removing them tends to leave junk in the Sage build tree. Thus the patchbot isn't really doing its job properly, of simulating the effect of applying the latest set of patches to a clean copy of Sage. So I found it preferable to fiddle with the patchbot test_a_ticket code such that it always made a new branch, then deleted it afterwards.

comment:28 Changed 9 years ago by robertwb

Well, this is a defect in sage -clone (which should hopefully go away with git). I'm not quite sure how

sage -clone ticket
[test ticket]
sage -b main
sage -clone newticket

would have different behavior from

sage -clone ticket
[test ticket]
sage -b main
rm -rf /path/to/deve/sage-ticket
sage -clone newticket

though I have seen issues with the build directory getting messed up when files are added and then removed.

comment:29 follow-up: Changed 9 years ago by vbraun

With the updated patch I get:

[patchbot@volker-desktop sage-5.1.beta3]$ ./sage -patchbot
/home/patchbot/Sage/sage-5.1.beta3/spkg/bin/sage: line 877: /home/patchbot/Sage/sage-5.1.beta3/local/bin/patchbot/patchbot.py: Permission denied

Manually changing the permission works.

Changed 9 years ago by kini

apply to $SAGE_LOCAL/bin

comment:30 in reply to: ↑ 29 Changed 9 years ago by kini

Replying to vbraun:

With the updated patch I get:

[patchbot@volker-desktop sage-5.1.beta3]$ ./sage -patchbot
/home/patchbot/Sage/sage-5.1.beta3/spkg/bin/sage: line 877: /home/patchbot/Sage/sage-5.1.beta3/local/bin/patchbot/patchbot.py: Permission denied

Manually changing the permission works.

I updated the patch.

comment:31 Changed 9 years ago by kini

Hah - patchbot gives its own code a green circle despite there being no patches for the Sage library :)

comment:32 Changed 9 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

Looks good to me!

The perfect is the enemy of the good.

comment:33 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:34 Changed 9 years ago by kini

On arando I am consistently getting a doctest failure in devel/sage-0/sage/rings/finite_rings/integer_mod.pyx when the patchbot first starts up and tests a vanilla Sage. When I doctest that particular file manually, there are no failures. Even when I run make ptestlong, there is no failure. But if I run sage --patchbot, it fails on that file. This has been happening since 5.1.beta5, as far as I can tell.

comment:35 Changed 9 years ago by kini

I tried it again while redirecting stdout to a text file which I later searched. The actual failure is apparently this:

sage -t  -force_lib devel/sage-0/sage/rings/finite_rings/integer_mod.pyx
**********************************************************************
File "/opt/patchbot-5.1.beta6/devel/sage-0/sage/rings/finite_rings/integer_mod.pyx", line 2855:
    sage: mod(5,13^5) == mod(5,13)                                                       
Expected:
    True
Got:
    False
**********************************************************************
1 items had failures:
   1 of   8 in __main__.example_88
***Test Failed*** 1 failures.

comment:36 Changed 9 years ago by vbraun

Keshav: Try make ptest or sage -tp <n> $SAGE_ROOT/devel/sage to test multiple files and see if you can reproduce it. The patchbot doesn't do anything different.

comment:37 Changed 9 years ago by kini

I've tried this three or four times with the patchbot and three or four times with make ptestlong, and of these trials all the patchbot runs end with this failure, and all the make ptestlongs pass all doctests. I'm trying again now with a plain make ptest (without the long doctests).

comment:38 Changed 9 years ago by kini

Yup, no errors with make ptest. Now trying sage --patchbot again for about the fifth time :)

comment:39 follow-up: Changed 9 years ago by jdemeyer

  • Summary changed from Add patchbot to Sage itself. to Add patchbot to Sage itself

May I remind you about my feature request for "patchbot log grepping": given a doctest error, I would like to find out whether the patchbot has seen that failure and which ticket caused it.

comment:40 follow-up: Changed 9 years ago by kini

sage --patchbot fails again on the same file. I think it's pretty clear that this is only happening when the patchbot runs the tests, though I can't imagine why. Is it worth setting the ticket to needs_work? Can anyone else reproduce this? (Try a 32-bit machine, I suppose...)

comment:41 Changed 9 years ago by vbraun

The patchbot just runs sage -t, its probably something in the doctesting framework that needs fixing. There is also the new doctesting framework at http://trac.sagemath.org/12415, maybe that'll fix it? In any case that discussion belongs to a different ticket.

Same for feature requests, Jeroen ;-)

comment:42 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to sage-sdist

It seems that sage-make_devel_packages (I guess) needs to be updated, the patchbot files in local/bin are not picked up by sage-sdist.

comment:43 in reply to: ↑ 39 Changed 9 years ago by robertwb

Replying to jdemeyer:

May I remind you about my feature request for "patchbot log grepping": given a doctest error, I would like to find out whether the patchbot has seen that failure and which ticket caused it.

Yes, that would be nice. At the moment my energies are focused on (finally) trying to just get this in.

comment:44 in reply to: ↑ 40 Changed 9 years ago by robertwb

Replying to kini:

sage --patchbot fails again on the same file. I think it's pretty clear that this is only happening when the patchbot runs the tests, though I can't imagine why. Is it worth setting the ticket to needs_work? Can anyone else reproduce this? (Try a 32-bit machine, I suppose...)

I have no idea, but unless this is reproducible by others I don't think this should block it getting in.

comment:45 Changed 9 years ago by kini

I agree. Volker actually told me on IRC that he couldn't reproduce it on a 32-bit VM, so it must be something peculiar to arando.

comment:46 Changed 9 years ago by robertwb

Indeed, it's very strange, as it's just running the same sage -t in a subshell.

Changed 9 years ago by robertwb

Apply to $SAGE_LOCAL/bin.

comment:47 Changed 9 years ago by robertwb

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Wow, such ugliness here. I can't wait until we're set up with a unified repository...

comment:48 Changed 9 years ago by vbraun

For the record, this is the 32-bit test VM passing the test: http://patchbot.sagemath.org/ticket/?machine=Fedora/16/i686/3.4.2-1.fc16.i686.PAE/sagevm&status=open In any case, this should not block the patchbot ticket going forward.

I noticed that $DOT_SAGE/cache contains a file $SAGE_ROOT-$PID-lazy_import_cache.pickle that seems to be not cleaned up by the sage cleaner. So over time the patchbot will fill it with 32k files, one per possible pid. Not terrible, but could be nicer. Should probably be fixed in the sage-cleaner, though.

Running the patchbot for a long time seems to leak file descriptors for a pseudoterminal, after a few days I have

[root@volker-desktop 2040]# ps -p 2040 u
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
patchbot  2040  0.0  0.3 301676 99940 pts/2    S+   Jun30   0:57 python /mnt/storage2TB/patchbot/Sage/sage-5.1.rc0/local/bin/patchbot/patchbot.py
[root@volker-desktop 2040]# ls -alv /proc/2040/fd/*
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/0 -> /dev/pts/2
l-wx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/1 -> pipe:[3598117]
l-wx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/2 -> pipe:[3598117]
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/3 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/4 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/5 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/6 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/7 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/8 -> /dev/pts/2
[...]
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/319 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/320 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 13:14 /proc/2040/fd/321 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/322 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/323 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 12:54 /proc/2040/fd/324 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 13:16 /proc/2040/fd/325 -> /dev/pts/2
lrwx------. 1 patchbot patchbot 64 Jul  1 13:16 /proc/2040/fd/326 -> /dev/pts/2
l-wx------. 1 patchbot patchbot 64 Jul  1 13:16 /proc/2040/fd/328 -> pipe:[3598117]

comment:49 Changed 9 years ago by vbraun

PS: there are no spurious processes around as far as I can tell

[root@volker-desktop fd]# pstree -p 2040
python(2040)─┬─bash(19705)───bash(19707)───python(19710)─┬─python(19711)───bash(22440)───python(22441)───python(22442)
             │                                           ├─python(19712)───bash(22317)───python(22318)───python(22319)─┬─gp(22346)
             │                                           │                                                             ├─gp(22364)
             │                                           │                                                             ├─mwrank(22334)
             │                                           │                                                             ├─mwrank(22337)
             │                                           │                                                             ├─mwrank(22340)
             │                                           │                                                             └─mwrank(22443)
             │                                           ├─python(19713)───bash(22450)───python(22451)───python(22452)
             │                                           ├─{python}(19714)
             │                                           ├─{python}(19715)
             │                                           └─{python}(19716)
             ├─python(19692)
             └─tee(19283)
[root@volker-desktop fd]# ps -p 19705 u
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
patchbot 19705  0.0  0.0 112096  1316 pts/2    S+   13:17   0:00 bash /mnt/storage2TB/patchbot/Sage/sage-5.1.rc0/sage -tp 3 -sagenb /mnt/storage2TB/patchbot/S
[root@volker-desktop fd]# ps -p 19692 u
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
patchbot 19692  0.0  0.0      0     0 pts/2    Z+   13:17   0:00 [python] <defunct>
[root@volker-desktop fd]# ps -p 19283 u
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
patchbot 19283  0.0  0.0 106848   624 pts/2    S+   13:16   0:00 tee /mnt/storage2TB/patchbot/Sage/sage-5.1.rc0/logs/10764-log.txt

comment:50 Changed 9 years ago by vbraun

Also, for the life of me I can't figure out how to make the patchbot ignore the unused patches in #12544. Am I missing anything? Perhaps we need to support "ignore file.patch" in the comment text to explicitly tell the bot to ignore patches?

Changed 9 years ago by vbraun

Initial patch

comment:51 Changed 9 years ago by vbraun

  • Description modified (diff)

The attached patch fixes the fd leak (2 per ticket) for me.

comment:52 Changed 9 years ago by vbraun

  • Work issues sage-sdist deleted

I'm fine with the ticket going into Sage. Positive review for everything except my patch.

comment:53 Changed 9 years ago by kini

For what it's worth, I'm no longer getting the failure on arando after I rebooted it and installed this ticket into 5.1.rc0 (I just got into Singapore today so I was able to physically access the machine again), so you can forget about that, I guess...

comment:54 Changed 9 years ago by robertwb

  • Status changed from needs_review to positive_review

Ah, thanks for the file descriptor leak! Positive review on that patch. I'll look into the issue at #12544, but that seems to be an exceptional case.

comment:55 Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_work

sage-sdist isn't fixed at all, the following files still disappear:

! patchbot/README.txt
! patchbot/db.py
! patchbot/http_post_file.py
! patchbot/images/blue-blob.png
! patchbot/images/green-blob.png
! patchbot/images/purple-blob.png
! patchbot/images/red-blob.png
! patchbot/images/white-blob.png
! patchbot/images/yellow-blob.png
! patchbot/patchbot.py
! patchbot/plugins.py
! patchbot/run_server.py
! patchbot/serve.py
! patchbot/templates/base.html
! patchbot/templates/ticket.html
! patchbot/templates/ticket_list.html
! patchbot/trac.py
! patchbot/util.py

comment:56 Changed 9 years ago by robertwb

Argh! OK, I'm going to rebase this on an entirely clean copy of Sage and git it fixed tonight.

comment:57 Changed 9 years ago by robertwb

I'm not sure I see what the problem is; is the issue that the files aren't extracted in the spkg itself? That shouldn't matter at all, as they're in the history which is what's used to install the spkg.

comment:58 Changed 9 years ago by jdemeyer

The issue is that the files are not copied into the spkg by sage-sdist.

comment:59 follow-up: Changed 9 years ago by robertwb

My question is more why do they need to be?

comment:60 in reply to: ↑ 59 Changed 9 years ago by jdemeyer

Replying to robertwb:

My question is more why do they need to be?

Because we have always done things that way, and I don't see any reason why we should make an exception for the patchbot files.

Surely our installation/packaging procedure can be improved, but that's the topic of #13015, not of this ticket.

Last edited 9 years ago by jdemeyer (previous) (diff)

comment:61 Changed 9 years ago by jdemeyer

If you really want to change the installation of the scripts spkg, make a separate ticket and add that as dependency here.

comment:62 Changed 9 years ago by robertwb

Sure, I'll do that. (I just did that here as it seemed cleaner than explicitly adding a bunch of globs to copy, making sure I got everything we wanted and nothing we didn't.)

comment:63 Changed 9 years ago by jdemeyer

* ping *

comment:64 Changed 9 years ago by jdemeyer

Does the patchbot already feature the automatic deleting of old devel/sage-NNNNN directories?

comment:65 Changed 9 years ago by kini

It doesn't do so by default at the moment, at least.

comment:66 follow-ups: Changed 9 years ago by robertwb

Sorry I haven't gotten back to this. I'll try to get to it within the week. Thanks for the ping.

Regarding deleting the sage-NNNNN directories, It should delete them as the tickets get closed.

comment:67 in reply to: ↑ 66 Changed 9 years ago by jdemeyer

Just an idea concerning the distribution of the patchbot: maybe you could make a separate spkg for the patchbot instead of distributing it with sage_scripts?

comment:68 in reply to: ↑ 66 Changed 9 years ago by jdemeyer

Replying to robertwb:

Regarding deleting the sage-NNNNN directories, It should delete them as the tickets get closed.

I would propose something like: if there is no activitity on a ticket (meaning no patches are changed) for a certain time (one week?), then also delete the directory.

comment:69 Changed 9 years ago by vbraun

I've been running a patchbot for a while now and disk usage isn't really that much of a deal. Especially since there isn't any reason to keep old versions around as development goes on. What is much more annoying is that this ticket is still not merged so with each beta I have to install a handful of patches on a machine on the other side of the atlantic...

comment:70 Changed 9 years ago by kini

I have this setup on arando:

[1] patchbot@arando /opt $ ls -hal
total 4.4G
drwxrwxr-x  3 root     opt      4.0K Aug 15 21:42 .
drwxr-xr-x 22 root     root     4.0K Aug 13 18:32 ..
lrwxrwxrwx  1 patchbot patchbot   18 Aug 13 20:12 patchbot -> patchbot-5.3.beta1
drwxr-xr-x 10 patchbot patchbot 4.0K Aug 13 23:49 patchbot-5.3.beta1
-rw-rw-r--  1 patchbot patchbot 295M Jul  3 03:15 sage-5.0.1.tar
-r--r--r--  1 kini     kini     294M May 15 17:54 sage-5.0.tar
-rw-r--r--  1 kini     kini     294M May 18 02:09 sage-5.1.beta0.tar
-rw-rw-r--  1 patchbot patchbot 294M May 29 05:40 sage-5.1.beta1.tar
-rw-rw-r--  1 patchbot patchbot 290M Jun  4 00:58 sage-5.1.beta2.tar
-rw-rw-r--  1 patchbot patchbot 290M Jun  7 17:37 sage-5.1.beta3.tar
-rw-rw-r--  1 patchbot patchbot 291M Jun 19 19:31 sage-5.1.beta5.tar
-rw-rw-r--  1 patchbot patchbot 291M Jun 25 19:30 sage-5.1.beta6.tar
-rw-rw-r--  1 patchbot patchbot 291M Jul  3 03:16 sage-5.1.rc0.tar
-rw-rw-r--  1 patchbot patchbot 291M Jul  6 07:32 sage-5.1.rc1.tar
-rw-rw-r--  1 patchbot patchbot 313M Jul 10 02:18 sage-5.2.beta0.tar
-rw-rw-r--  1 patchbot patchbot 314M Jul 25 01:59 sage-5.2.rc1.tar
-rw-rw-r--  1 patchbot patchbot 314M Jul 26 02:00 sage-5.2.tar
-rw-rw-r--  1 patchbot patchbot 314M Aug  1 23:55 sage-5.3.beta0.tar
-rw-rw-r--  1 patchbot patchbot 314M Aug 13 06:15 sage-5.3.beta1.tar
-rwxrwxr-x  1 patchbot patchbot 1.3K Aug 15 21:42 setup-patchbot.sh
[1] patchbot@arando /opt $ cat setup-patchbot.sh 
#!/bin/bash

# This script upgrades the currently running patchbot in this directory.
# Supply a Sage version number as an argument, and the script will
# download, set up, and run the patchbot.

VER=$1
[ -n $VER ] || exit 1

# Get new patchbot and extract it
rm -f sage-$VER.tar
wget http://boxen.math.washington.edu/home/release/sage-$VER/sage-$VER.tar
rm -rf $(readlink patchbot)

# Delete old patchbot dir (keep tarball) and move symlink to new
# patchbot dir
rm patchbot
tar xf sage-$VER.tar
mv sage-$VER patchbot-$VER
ln -s patchbot-$VER patchbot

# Build new patchbot's Sage from source
cd patchbot
make

# Install the patchbot into the newly built Sage version (TODO: update
# this when the patches on ticket #12486 change, or when #12486 is
# merged)
hg qimport -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/12486/patchbot-root-5.0.patch
hg -R local/bin qimport -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/12486/12486-patchbot-scripts-5.0-updated.patch
hg -R local/bin qimport -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/12486/12486-fix-sdist.patch
hg -R local/bin qimport -P http://trac.sagemath.org/sage_trac/raw-attachment/ticket/12486/trac_12486_fix_fd_leak.patch
./sage -b

# Start the patchbot
exec ./sage --patchbot
[1] patchbot@arando /opt $ 

Makes it pretty easy...

comment:71 Changed 9 years ago by robertwb

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I've created an spkg: http://sage.math.washington.edu/home/robertwb/patches/patchbot-1.0.spkg

The contents of this spkg should be identical to the (flattened) patches.

comment:72 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Adding a spkg requires changes to spkg/standard/deps and spkg/install.

Leaving $SAGE_LOCAL unquoted is not so good:

$SAGE_LOCAL/bin/patchbot/patchbot.py

Better is

"$SAGE_LOCAL/bin/patchbot/patchbot.py"

Changed 9 years ago by robertwb

comment:73 Changed 9 years ago by robertwb

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:74 Changed 9 years ago by robertwb

  • Description modified (diff)

comment:75 Changed 9 years ago by kini

As Simon King pointed out on sage-devel a few days ago, the patchbot shouldn't report "ApplyFailed?" when what failed was the downloading of the patches rather than the application of the patches.

Version 0, edited 9 years ago by kini (next)

comment:76 Changed 9 years ago by vbraun

  • Summary changed from Add patchbot to Sage itself to Make the Sage patchbot an optional spkg

Good idea to package the bot. I think we should start by making the patchbot an optional spkg. We don't need to modify spkg/standard/deps or add id to the sage -advanced help then. Once it is an optional spkg we can vote on sage-devel to make it standard (+1 from me).

For consistency the mercurial repo inside the spkg should have the bot committed and not as a patch queue. Apart from that its ready to go.

comment:77 Changed 9 years ago by kini

The Mercurial repo inside the SPKG should not have the bot committed. The bot should be in the src/ directory and the src/ directory should be ignored in .hgignore. This is standard among SPKGs, and is also what is present in the current patch queue in the SPKG.

comment:78 Changed 9 years ago by vbraun

Thats what I meant. Sorry for not being clear. My point is that the SPGK.txt, ... files are in a mercurial queue right now.

comment:79 Changed 9 years ago by vbraun

patchbot.py is not executable, so sage --patchbot fails to execute.

comment:80 Changed 9 years ago by robertwb

OK, I've pushed a new commit to make patchbot.py executable and make the hg commit a real commit. It's nit-picky stuff like this that in part made me want to avoid making an spkg. I'd much rather spend time improving or cleaning up the patchbot than worrying about packaging details as I've done over the last 7 (!) months since filing this ticket (whether as a patch or an spkg) but it's a real pain to not have this in. Basically, we need to move to a better system soon... OK, done ranting.

I also made it to qimport failing doesn't cause the ticket to fail.

comment:81 Changed 9 years ago by robertwb

And, to be clear, no offense to any of you intended, in fact I'm really grateful you're looking at this. It's the system that's to onerous.

comment:82 Changed 9 years ago by jdemeyer

  • Component changed from doctest to optional packages

comment:83 Changed 9 years ago by vbraun

  • Status changed from needs_review to positive_review

Looks good to me!

comment:84 Changed 9 years ago by schilly

the optional spkg is on the server+mirrors now.

comment:85 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.3.rc1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.