Review Day next Tuesday (22 Sep)

1 view
Skip to first unread message

Jason Grout

unread,
Sep 15, 2009, 4:46:50 PM9/15/09
to sage-...@googlegroups.com
We have been extremely blessed lately with a load of great
contributions. Currently, there are:

* 96 tickets that have patches/spkgs and are waiting for review:
http://trac.sagemath.org/sage_trac/report/10. These tickets have been
waiting for review for up to 10 months! [1]

* 6 tickets that apparently just need a rebase:
http://trac.sagemath.org/sage_trac/report/19

William has asked me to announce a Review Day next Tuesday, 22
September. The goal is take care of every ticket that has "needs
review" status and either declare it positive review or give specific
feedback and put it in the needs work queue.

The wiki page for the review day is: http://wiki.sagemath.org/review1

If we finish early, then there are a total of 288 tickets with patches
in various states of readiness: http://trac.sagemath.org/sage_trac/report/13

Minh has been doing a tremendously wonderful job of getting positive
reviews merged promptly. Let's see how many positive reviews he can
handle :).

Thanks,

Jason


[1] see http://trac.sagemath.org/sage_trac/ticket/3297, or a full report
sorted by last modified date at
http://trac.sagemath.org/sage_trac/query?status=assigned&status=new&status=reopened&max=300&summary=~needs+review&col=id&col=summary&col=type&col=priority&col=component&col=changetime&order=changetime

--
Jason Grout

Minh Nguyen

unread,
Sep 15, 2009, 6:36:49 PM9/15/09
to sage-...@googlegroups.com
Hi folks,

On Wed, Sep 16, 2009 at 6:46 AM, Jason Grout
<jason...@creativetrax.com> wrote:

<SNIP>

> William has asked me to announce a Review Day next Tuesday, 22
> September. The goal is take care of every ticket that has "needs
> review" status and either declare it positive review or give specific
> feedback and put it in the needs work queue.

I'll be around on #sage-devel on IRC.

I should release alpha2 before Tuesday 22nd September 2009 so people
can base their work on that release.


> Minh has been doing a tremendously wonderful job of getting positive
> reviews merged promptly. Let's see how many positive reviews he can
> handle :).

Don't encourage me :-)

--
Regards
Minh Van Nguyen

Marshall Hampton

unread,
Sep 20, 2009, 7:11:15 PM9/20/09
to sage-devel
Cool. Tuesdays aren't very good for me, but I will do my best to do
some reviews that day. I will try to at least address some of the
polytope-related patches.

-Marshall Hampton

On Sep 15, 3:46 pm, Jason Grout <jason-s...@creativetrax.com> wrote:
> We have been extremely blessed lately with a load of great
> contributions. Currently, there are:
>
> * 96 tickets that have patches/spkgs and are waiting for review:http://trac.sagemath.org/sage_trac/report/10. These tickets have been
> waiting for review for up to 10 months! [1]
>
> * 6 tickets that apparently just need a rebase:http://trac.sagemath.org/sage_trac/report/19
>
> William has asked me to announce a Review Day next Tuesday, 22
> September. The goal is take care of every ticket that has "needs
> review" status and either declare it positive review or give specific
> feedback and put it in the needs work queue.
>
> The wiki page for the review day is:http://wiki.sagemath.org/review1
>
> If we finish early, then there are a total of 288 tickets with patches
> in various states of readiness:http://trac.sagemath.org/sage_trac/report/13
>
> Minh has been doing a tremendously wonderful job of getting positive
> reviews merged promptly. Let's see how many positive reviews he can
> handle :).
>
> Thanks,
>
> Jason
>
> [1] seehttp://trac.sagemath.org/sage_trac/ticket/3297, or a full report
> sorted by last modified date athttp://trac.sagemath.org/sage_trac/query?status=assigned&status=new&s...
>
> --
> Jason Grout

William Stein

unread,
Sep 20, 2009, 7:13:18 PM9/20/09
to sage-...@googlegroups.com
On Sun, Sep 20, 2009 at 4:11 PM, Marshall Hampton <hamp...@gmail.com> wrote:
>
> Cool.  Tuesdays aren't very good for me, but I will do my best to do

It'll only be on Tuesday this time. There are now 4 people up at

http://wiki.sagemath.org/review1

I wish there were more than 4, but that is a good start.
What time should we start?

William
--
William Stein
Associate Professor of Mathematics
University of Washington
http://wstein.org

Jason Grout

unread,
Sep 21, 2009, 8:41:21 AM9/21/09
to sage-...@googlegroups.com
William Stein wrote:
> On Sun, Sep 20, 2009 at 4:11 PM, Marshall Hampton <hamp...@gmail.com> wrote:
>> Cool. Tuesdays aren't very good for me, but I will do my best to do
>
> It'll only be on Tuesday this time. There are now 4 people up at
>
> http://wiki.sagemath.org/review1
>
> I wish there were more than 4, but that is a good start.
> What time should we start?
>


I plan on starting by 8:30AM Central Time. I realize that might be kind
of early for the folks on the west coast, though. If Karl or other East
Coast people want to start even earlier, they are more than welcome.

Thanks,

Jason

--
Jason Grout

Nicolas M. Thiery

unread,
Sep 21, 2009, 3:40:17 PM9/21/09
to sage-...@googlegroups.com
On Tue, Sep 15, 2009 at 03:46:50PM -0500, Jason Grout wrote:
>
> We have been extremely blessed lately with a load of great
> contributions. Currently, there are:
>
> * 96 tickets that have patches/spkgs and are waiting for review:
> http://trac.sagemath.org/sage_trac/report/10. These tickets have been
> waiting for review for up to 10 months! [1]
>
> * 6 tickets that apparently just need a rebase:
> http://trac.sagemath.org/sage_trac/report/19
>
> William has asked me to announce a Review Day next Tuesday, 22
> September. The goal is take care of every ticket that has "needs
> review" status and either declare it positive review or give specific
> feedback and put it in the needs work queue.
>
> The wiki page for the review day is: http://wiki.sagemath.org/review1
>
> If we finish early, then there are a total of 288 tickets with patches
> in various states of readiness: http://trac.sagemath.org/sage_trac/report/13
>
> Minh has been doing a tremendously wonderful job of getting positive
> reviews merged promptly. Let's see how many positive reviews he can
> handle :).

Please, please, please, finish up to review the remaining categories on
http://sagetrac.org/sage_trac/wiki/CategoriesCategoriesReview!

I'll try to join, but Greenwich time won't be very compatible ...

Cheers,
Nicolas
--
Nicolas M. Thiéry "Isil" <nth...@users.sf.net>
http://Nicolas.Thiery.name/

Robert Bradshaw

unread,
Sep 21, 2009, 3:54:20 PM9/21/09
to sage-...@googlegroups.com

Depends on how late you like to stay up :).

I won't be able to spend all day on it, but will try to put in a
couple of hours.

- Robert

Tim Joseph Dumol

unread,
Sep 22, 2009, 5:39:12 AM9/22/09
to sage-...@googlegroups.com
I'll try and join. I'll be up until around noon CST.
--
Tim Joseph Dumol <tim (at) timdumol (dot) com>

Nicolas M. Thiery

unread,
Sep 22, 2009, 6:40:48 AM9/22/09
to sage-...@googlegroups.com
On Mon, Sep 21, 2009 at 12:54:20PM -0700, Robert Bradshaw wrote:
> > Please, please, please, finish up to review the remaining
> > categories on
> > http://sagetrac.org/sage_trac/wiki/CategoriesCategoriesReview!
> >
> > I'll try to join, but Greenwich time won't be very compatible ...
>
> Depends on how late you like to stay up :).

Or early! I am already online :-) I'll be keeping an eye on IRC; but
if I don't answer, please ping me by e-mail; I might be on my way to
pickup the girls, preparing dinner, ...

> I won't be able to spend all day on it, but will try to put in a
> couple of hours.

Thanks!

Harald Schilly

unread,
Sep 22, 2009, 9:48:17 AM9/22/09
to sage-devel
On Sep 15, 10:46 pm, Jason Grout <jason-s...@creativetrax.com> wrote:
>   * 96 tickets...

I haven't followed each thread lately, but are there any ideas how to
improve the review situation?
It might help if someone is responsible, therefore I propose to create
"review coordinators", who are responsible for one or more category
(e.g. I'm the website guy), and they review or ask others to review.
We can create a site on trac's wiki for that. Otoh, about my #6612
ticket, i have no idea who would be responsible for the "distribution"
component and besides dropping everything on william, i have no idea
who is able to review it.
Another idea is some kind of reward: if you review more, your patches
are reviewed with higher priority .. but i'm not sure if this could
work ;)

H

William Stein

unread,
Sep 22, 2009, 9:52:03 AM9/22/09
to sage-...@googlegroups.com, Robert Bradshaw


Robert Bradshaw, who worked all summer at Google, said that they have
an incredibly good code review webapp there, which Guido (author of
Python) wrote, and which is open source. He said that because of
how nice this app is, their code review process goes pretty well.
Maybe if we did something like that our process would improve a lot?
Ideas?

William

Tim Joseph Dumol

unread,
Sep 22, 2009, 10:15:31 AM9/22/09
to sage-...@googlegroups.com
Since it's open source, can we not use it ourselves?

William Stein

unread,
Sep 22, 2009, 10:28:06 AM9/22/09
to sage-...@googlegroups.com, Robert Bradshaw
On Tue, Sep 22, 2009 at 7:15 AM, Tim Joseph Dumol <t...@timdumol.com> wrote:
> Since it's open source, can we not use it ourselves?

Yes, I hope we will! Any volunteers to set it up?

Robert Bradshaw would be a good candidate since he used it all summer,
etc., however as his thesis adviser I really hope he *won't* work on
this, and will spend the time working on his Ph.D. thesis instead.

William

Harald Schilly

unread,
Sep 22, 2009, 10:47:10 AM9/22/09
to sage-devel
On Sep 22, 3:52 pm, William Stein <wst...@gmail.com> wrote:
> Robert Bradshaw, who worked all summer at Google, said that they have
> an incredibly good code review webapp there,...

rietveld? Well, it's better than trac i guess, but software alone
isn't everything.
Here an example for a sympy review in rietveld:
http://codereview.appspot.com/20078

h

Harald Schilly

unread,
Sep 22, 2009, 10:49:28 AM9/22/09
to sage-devel


On Sep 22, 4:47 pm, Harald Schilly <harald.schi...@gmail.com> wrote:
> Here an example for a sympy review in rietveld..

Ah, that's nice, click on "expand comments" to see annotations inside
diffs:
http://codereview.appspot.com/20078/diff/1/3

h

Tim Joseph Dumol

unread,
Sep 22, 2009, 10:57:10 AM9/22/09
to sage-...@googlegroups.com
It seems that he was referring to <a href="http://code.google.com/p/rietveld/">Rietveld</a>. If so, it doesn't seem we can use it without a bit of modification, since it's for Subversion.

Tim Joseph Dumol

unread,
Sep 22, 2009, 11:22:04 AM9/22/09
to sage-...@googlegroups.com
Sorry, I mean't we *can't* use it without a bit of modification.

Jason Grout

unread,
Sep 22, 2009, 11:51:29 AM9/22/09
to sage-...@googlegroups.com
Tim Joseph Dumol wrote:
> Sorry, I mean't we *can't* use it without a bit of modification.


A google search for "rietveld mercurial" shows lots of work on this problem.

Jason

--
Jason Grout

Robert Bradshaw

unread,
Sep 23, 2009, 11:07:18 PM9/23/09
to sage-...@googlegroups.com
On Sep 22, 2009, at 8:51 AM, Jason Grout wrote:

> Tim Joseph Dumol wrote:
>> Sorry, I mean't we *can't* use it without a bit of modification.
>
>
> A google search for "rietveld mercurial" shows lots of work on this
> problem.

Very cool.

As mentioned, I used this system all summer, and it is very nice
compared to how we're using trac. Trac is great at what it was meant
for--bugtracking, but it was not meant to be part of a manual
revision control system. I actually started thinking about this well
before this summer, but now that I've used a fairly nice setup it's
even more clear how good it is when the system gets out of the way
and lets you focus on the work.

Here's my ideal system (which rietveld may or may not be a part of,
but certainly it'd be better to not start something from scratch).

(0) I hack on my own code, committing as I want, the normal mercurial
way.
(1) I run a sage command that takes all my changes and uploads them
to trac (or rietveld, or Review Board, or ...). It may or may not
flatten them. The ticket description should be no more than the
commit description (i.e. our commit descriptions should be a lot more
verbose.)
(2) This patch automatically gets applied to the current alpha, any
merge problems, build problems (including documentation), or test
failures get reported and bounced back. Linting could happen at this
point too (everything from no tabs to coverage to "Sage:" in docstrings)
(3) Upon a successful build (or even not) this build gets saved and
can be accessed and played with (via the notebook or by ssh-ing into
a virtual machine). The code can be browsed and commented on.
(4) Reviewer makes comments, as a whole or line-by-line. They can use
(3), browse the code, and have a sage command like sage -merge that
will quickly update their local copy to the state of the ticket, and
allow them to push as well.
(5) I can quickly address those comments via a sage -b into that
branch (or otherwise using queues/etc. to get back to my ticket
state), make my changes, and re-push to trac. Building and tests
happen after every push. This point here is not to be underestimated--
I think it will greatly improve the quality of the code to lower the
overhead of the feedback loop.
(6) When the reviewer is happy, he marks it as positive review.
(7) The release manager(s) approve the patch. At this point they know
it builds, passes tests, and has at least one positive review. It
goes into a queue.
(8) Patches in the queue get applied to the current alpha. This
implies a moving alpha, which one knows builds and passes all tests
on at least one system. It should be easy to upgrade to that alpha.
(9) People can (easily) donate computer time to build and test
alphas, which will greatly increase hardware coverage. Bisection can
be used to locate the offending ticket in case of failure.

Note that one can still work "out of the system" mailing around
mercurial bundles and patches, so there's no lock-in. I'm sure there
are a lot of more details to work out (like how to handle spkgs,
which I think could still fit into the above system), but I think a
system like the above would make the job easier for both developers
and release managers. It might tax the computer systems a bit, but
that's not what we're short on now (or, probably, into the
foreseeable future). Every step above that involves a human needs a
human, and doesn't require tons of overhead.

- Robert

P.S. I won't be the one setting this up, at least I hope someone
beats me to it before I have time to tackle something like this next
year.

Tim Joseph Dumol

unread,
Sep 24, 2009, 6:10:34 AM9/24/09
to sage-...@googlegroups.com
That is *very* cool. We should put that in a wiki page so we can plan a way to transition to a system like that.

Tim Joseph Dumol

unread,
Sep 30, 2009, 12:54:16 PM9/30/09
to sage-...@googlegroups.com
On Thu, Sep 24, 2009 at 11:07 AM, Robert Bradshaw <robe...@math.washington.edu> wrote:

If you don't mind, I put this in SageTasks (http://wiki.sagemath.org/SageTasks)

Note that one can still work "out of the system" mailing around
mercurial bundles and patches, so there's no lock-in. I'm sure there
are a lot of more details to work out (like how to handle spkgs,
which I think could still fit into the above system), but I think a
system like the above would make the job easier for both developers
and release managers. It might tax the computer systems a bit, but
that's not what we're short on now (or, probably, into the
foreseeable future). Every step above that involves a human needs a
human, and doesn't require tons of overhead.

- Robert

P.S. I won't be the one setting this up, at least I hope someone
beats me to it before I have time to tackle something like this next
year.



Nicolas M. Thiery

unread,
Oct 4, 2009, 7:28:46 AM10/4/09
to sage-...@googlegroups.com

A big +1! The overhead of the current workflow is killing us (well, at
least seriously slowing us down).

Reply all
Reply to author
Forward
0 new messages