[python-committers] My (positive) feedback on the new CPython workflow

[Brett Cannon]

On Tue, 18 Jul 2017 at 03:24 Victor Stinner <victor.stinner at gmail.com>
wrote:

> Hi,
>
> 2017-07-18 11:36 GMT+02:00 Antoine Pitrou <antoine at python.org>:
> > Can I take the opportunity to say thank you again (both you and Larry)
> > for the "blurb" tool?  It really makes an important difference when
> > contributing.
>

Glad it's working out! And a thanks from me to everyone who participated in
the discussions that led to blurb (and of course Larry for coding it up and
putting up with me throughout the whole process :) .


> >
> > Regards
> >
> > Antoine.
>
> I concur with Antoine, I'm now *very* happy with the new workflow. The
> code became better, many things are much simpler for me (especially
> the tasks that I don't have to do myself anymore ;-)), my productivity
> increases and I see more and more active contributors, much more than
> before the move to GitHub and new workflow!
>

Great! That was the goal the whole time so I'm glad it's starting to pay
off.


>
>
> == Introduction ==
>
> Two friends asked me recently how is the new Python workflow going.
>
> Somehow, I decided to not express myself on the topic to avoid dummy
> knee-jerk reaction like "it sucks, it was better before, xxx doesn't
> work, now I have to xxx, etc."
>
> It took me at least 3 months to be used to the new workflow, while
> tools were developed and enhanced, and the workflow was adjusted based
> on our early feedback.
>
> So, here is my feedback :-)
>
>
> == pre-commit CI: less regressions ==
>
> From a pure technical point of view, IMHO pre-commit CI on Linux
> (Travis CI) and Windows (AppVeyor) is a *MAJOR* enhancement. (I don't
> count macOS, since it's not mandatory yet.)
>
> For example, before GitHub, it was common to break Windows since too
> few core developers were running tests *manually* on Windows.
>
> It's hard to prove my point, so try to trust me: we reduce a lot the
> number of regressions introduced by new changesets. Before, every two
> weeks, we had to rush to push fixes in urgency (which sometimes leaded
> to bad code which required multiple iterations until we find the best
> fix).
>
> It was especially annoying when a dev pushed a big change just before
> being away for 8-12 hours, which means having to push a fix without
> the review of the change author :-(
>

I have to admit I also appreciate the CI testing a lot since I worry a lot
less about whether Python is in some broken state. And then thanks to all
the hard work you've done, Victor, we know that if the buildbots go red
that we have actually regressed and need to roll back a change instead of
going "that buildbot is just always broken so you can ignore it".

It's getting to the point that I'm toying with the idea of making a Google
Assistant command for my Google Home so I can say, "Hey Google, how is
CPython doing?" and have it tell me if the buildbots are all green. :) And
I still dream about having some LED light or grid for sprints where we can
visibly see the buildbot statuses to know quickly when something has
broken. But none of that was possible previously, so it's really exciting
to have reached a stability point where this isn't a stupid idea. :)


>
>
> == More contributors, more contributions, faster reviewed/merged ==
>
> In term of contributions, I looked at statistics yesterday and it
> became clear the number of different authors is significantely much
> higher, around 25 contributors / month before, now closer to 50:
> https://www.openhub.net/p/python/contributors/summary
>
> Well, Git allows to store the original author, so statistics are
> simplify higher just because previously the tools failed to identify
> the real author.
>

I'm going to be interested to see how the numbers look in Feb 2018 compared
to now and see if there's a trend since the transition.


>
> But I'm watching the bug tracker, pull requests, and Git commits:
> there are a lot of *new* contributors, we get more contributions, and
> we are *faster* to integrate them.
>
> I also saw again inactive core contributors starting to review again,
> or even write new PRs! It's a very good sign of the good health of our
> project!
>

As Serhiy pointed out, though, some people have not made the transition. To
help with this I plan on offering to do video chats with any core devs who
want a personal walkthrough of the workflow, but I'm holding off on that
until all of the critical workflow changes have landed as I don't want to
be talking with someone and saying, "but this will be changing in a month
or so". :)


>
> More generally, the whole development seems to be more active, more
> productive and more *healthy*.
>
>
> == Better scale horizontally ==
>
> The new workflow better scales horizontally since we delegated much
> more work to the contributor. Previously the core developers were more
> the bottleneck.
>
> Tasks delegated to the contributors:
>
> * create the PR itself (can you still remind when we had to download a
> patch file and create a commit? ;-)),
>
> * write the commit message (I always hated to do that without any kind
> of review...),
>
> * add the NEWS entry (before it was common to explicitly ask to *not*
> write it... to avoid conflicts with Misc/NEWS...).
>

Once again, glad the goals are panning out. :) Key thing has always been to
increase our bandwidth and part of that was always to push more on to
contributors so they can be more self-servicing.


>
> * most contributors create backports (using cherry-pick) themself.
> Before, this painful/error-prone task was usually done by a single
> developer without any review, without pre-commit tests, etc.
>

Nice! Maybe we should have a message from Bedevere after a PR gets merged
saying something like, "thanks for the PR and congrats on getting it
merged! If you're interested in helping to backport your PR to older
versions of Python, let us know and follow the instructions at XXX to
create backporting PRs."


>
> Note: I didn't see any contributor complaining about having to do
> these tasks. It seems like it's more the opposite, they are happy to
> be more *involved* in the project!
>
>
> == GitHub ==
>
> I never had to explain how to create an account on GitHub, nor how to
> create a PR.
>
> Signing the CLA doesn't seem to be a blocker point neither,
> contributors are able to sign it and the bot tracks correctly the CLA
> status (thanks Brett for the bot! it's nice to feel safe for legal
> issues ;-)).
>

Yeah, I sleep a little better at night thanks to that thing. :)


>
> Maybe we have an excellent documentation. Maybe developers already
> know well the GitHub platform.
>
> Even for backports, I didn't have to explain how to cherry-pick a
> commit, but it was common that I pointed to the devguide section when
> I proposed the author to do the backport himself/herself.
>
> Having explicitly a list of "Approve" and "Requet changes" votes helps
> to *quickly* have an overview of the review status.
>
> I'm just not unconfortable with the fact that an approval is kept even
> if the PR is modified after the review :-/ I would expect a list a
> notice "changed modified after the review" or something like that. At
> least, for my own reviews, to remind me to review again.
>

That wouldn't be too hard to add. I'm currently working on
https://github.com/python/core-workflow/issues/94 which is going to label
PRs based on their current stage (much like the stage field on
bugs.python.org but much more automated). One of the labels is going to be
"awaiting change review" for when a contributor has addressed requested
changes, but it could be applied when the latest review is an approval but
a post-approval change was made (along w/ a comment to point out that's why
the label was added). Does that sound like what you're after?


>
> The [First time contributor] label on PR helps to remind Misc/ACKS:
> ask the author to add himself/herself to ACKS.
>

That's a great trick! I really wish we could programmatically access that
more easily than scouring the git repo for committer details.


>
> Compared to Rietveld, GitHub review tool is as "a good" (not much
> better, not much worse). Sometimes, I'm lost in reviews: my comments
> are hidden, I have to unfold many widgets. But it's not that bad. It
> seems like avoiding rebase works around some of these issues.
>
>
> == Git branches ==
>
> Having to create a local Git branch, push it to my repository and
> create PR is more work than attaching a patch file to bugs.python.org.
> It requires to be more organized to track my "local" Git branches.
>
> I'm now trying to remove a local branch after pushing the PR, since it
> also exists in my "haypo" remote anyway. It helps to have a shorter
> list of branches.
>
> I also wrote a shell script to run "git fetch haypo --prune" to remove
> "merged" branches (technically, I have to click on a button to remove
> my branch, after I merge my own PR).
>
> Sometimes, I also see branches in the upstream repository
> (python/cpython), but it seems that we have less and less of them.
>

I think people are pretty good about cleaning up after themselves since
those branches usually only come up when someone makes a change through the
web UI.


>
>
> == NEWS.d and blurb ==
>
> Thank you Larry Hastings, Brett Cannon and others who helped to write
> and integrate blurb: no more Misc/NEWS conflict, it's just amazing :-)
> We *all* wanted that for *years*!
>

Just to keep people up-to-date with what I personally have planned or I
know people are working on:

   1. Status check for a news entry (
   https://github.com/python/bedevere/pull/22)
   2. Add a link to the end of the PR body back to bugs.python.org (thanks
   to Kushal; https://github.com/python/bedevere/issues/3 and
   https://github.com/python/bedevere/pull/24)
   3. Label PRs based on what stage they are at (
   https://github.com/python/core-workflow/issues/94; started work on this)
   4. Make the CLA bot a status check instead of a label to make it a
   required status check (
   https://github.com/python/the-knights-who-say-ni/issues/42)
   5. Commit emails w/ diffs (thanks to Berker;
   https://github.com/python/bedevere/pull/27)

I think #2 and #5 cover the last initial complains people had about the new
workflow, #1 and #3 will make things even better, and #4 is so I can sleep
even better at night. ;)

My wishlist that I don't think anyone is actively working on ATM is:

   1. Bot to merge approved PRs (e.g. Rust's Homu)
   2. Bot to backport PRs (which could also be automatically merged)
   3. Automate Misc/ACKS (or do away with it and do something like
   https://thanks.rust-lang.org/)
   4. Make test coverage reports consistent
   5. Automatically close stale PRs (e.g. not signing CLA, changes
   requested but not being made, etc.)
   6. Bot to nudge core devs when they forget something (e.g. post-merge, a
   comment if someone forgot to change the commit message or PR number to have
   a "GH-" prefix)

I think that wishlist would get us as automated as reasonably possible
while reminding people when the manual parts are accidentally done wrong.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-committers/attachments/20170718/40262224/attachment-0001.html>