llvm dev - Nov 2019 - Enable Contributions Through Pull-request For LLVM

-1 -- switch to PR's
+1 -- Hal's compromise proposal (as long as I can continue to use Phab)

I agree that the documentation could be better, but I don't see that as a
justification for switching from a superior tool to an inferior one.  Let's
work on the documentation first, then if there's still a compelling reason
to switch, do it then,

I share most of the concerns already mentioned, but my biggest problem on a
daily basis is the use of force pushes to PR's, which loose information (no
way to tell what changed from the previous version -- I deal with this
everyday, and have to re-review the entire patch because I can't see what
was changed/fixed) . Since the branch is in someone's personal fork, I
don't see how it could be outlawed, and that's a problem and huge time
sync.

However, I do agree that the patches should be squashed into a single
commit when merged to master.

Btw, I've read several posts claiming github is willing to work with us to
improve the process.  Perhaps we could get them to adopt Phab.

thanks...
don

On Tue, Nov 12, 2019 at 9:52 AM Robinson, Paul via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> In general I mildly favor moving to GitHub PRs, mainly because it is
> a de-facto standard in the industry and many people are already
> familiar with it.  LLVM's idiosyncratic and essentially undocumented
> tool is not, whatever minor advantages it might have in certain
> technical respects.
>
> David Tellenbach wrote:
>
> > One thing I always liked at the Phabricator workflow is that it is
> > based on diffs only.
>
> And other people on the thread find this an annoyance.
>
> Being unable to chain related reviews is about the only case where
> GitHub would appear to be at a significant disadvantage, and offhand
> it would not seem like an insurmountable technical obstacle if we can
> persuade the GitHub folks that it's actually pretty useful.
>
>
> Alex Brachet-Mialot wrote:
>
> > Just because more people might be familiar with GitHub, doesn't
mean
> > it is superior.
>
> "The perfect is the enemy of the good."  Automobile controls are
> largely standardized; you might design a superior set of controls,
> but the simple fact of being different from all the others would be
> a barrier to adoption.  Even if GitHub's workflow is flawed, it is
> what new contributors will be used to, and new contributors are the
> lifeblood of the project. And as a major project hosted on GitHub,
> it is not impossible that we could influence GitHub to smooth it out
> to better suit our use-cases.
>
> Or, check out Hal Finkel's suggestion, which would allow casual
> contributors to at least start out with their accustomed workflow
> and then be funneled into LLVM's idiosyncratic review tool, which
> is fundamentally user-hostile but our community has gotten used to.
>
> > I think many people agree that Phabricator is really good,
>
> And many people agree that Phabricator is a major pain.  It has
> zero useful documentation (other than what the LLVM project has
> written for its own use) and IME makes every effort to make useful
> features difficult to find and use.  Ingenious people still manage
> to find and use them, but Phab absolutely does not make it easy.
>
> > Getting reviewers [on GitHub] is a much bigger barrier
>
> This would depend on how new reviews are advertised.  With Phab, we
> understand how they are advertised (i.e., mailing list).  With GitHub
> this is apparently not automatic, but still automatable, IIUC.
> --paulr
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191112/0246abe5/attachment.html>

For mutiple PRs, we have been using stacked PRs in the PyTorch repository.
Edward Yang wrote a tool for this called ghstack (
https://github.com/ezyang/ghstack) and my experience using it has been
pleasant - you put your patches as seperate commits on top of the master
and call ghstack, and ghstack separates out the commits into multiple PRs.
This does require the contributer to have permissions to create branches
(we do that by creating a team in the github project). The branches get
created with a prefix *ghstack/<contributor-name>/*.* After a PR is
merged,
stale branches are deleted using a script by a maintainer. You can see an
example stacked PR here: https://github.com/pytorch/pytorch/pull/21364.

Best,

Syed Ahmed

On Tue, Nov 12, 2019 at 1:53 PM Don Hinton via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> -1 -- switch to PR's
> +1 -- Hal's compromise proposal (as long as I can continue to use Phab)
>
> I agree that the documentation could be better, but I don't see that as
a
> justification for switching from a superior tool to an inferior one. 
Let's
> work on the documentation first, then if there's still a compelling
reason
> to switch, do it then,
>
> I share most of the concerns already mentioned, but my biggest problem on
> a daily basis is the use of force pushes to PR's, which loose
information
> (no way to tell what changed from the previous version -- I deal with this
> everyday, and have to re-review the entire patch because I can't see
what
> was changed/fixed) . Since the branch is in someone's personal fork, I
> don't see how it could be outlawed, and that's a problem and huge
time sync.
>
> However, I do agree that the patches should be squashed into a single
> commit when merged to master.
>
> Btw, I've read several posts claiming github is willing to work with us
to
> improve the process.  Perhaps we could get them to adopt Phab.
>
> thanks...
> don
>
> On Tue, Nov 12, 2019 at 9:52 AM Robinson, Paul via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> In general I mildly favor moving to GitHub PRs, mainly because it is
>> a de-facto standard in the industry and many people are already
>> familiar with it.  LLVM's idiosyncratic and essentially
undocumented
>> tool is not, whatever minor advantages it might have in certain
>> technical respects.
>>
>> David Tellenbach wrote:
>>
>> > One thing I always liked at the Phabricator workflow is that it is
>> > based on diffs only.
>>
>> And other people on the thread find this an annoyance.
>>
>> Being unable to chain related reviews is about the only case where
>> GitHub would appear to be at a significant disadvantage, and offhand
>> it would not seem like an insurmountable technical obstacle if we can
>> persuade the GitHub folks that it's actually pretty useful.
>>
>>
>> Alex Brachet-Mialot wrote:
>>
>> > Just because more people might be familiar with GitHub,
doesn't mean
>> > it is superior.
>>
>> "The perfect is the enemy of the good."  Automobile controls
are
>> largely standardized; you might design a superior set of controls,
>> but the simple fact of being different from all the others would be
>> a barrier to adoption.  Even if GitHub's workflow is flawed, it is
>> what new contributors will be used to, and new contributors are the
>> lifeblood of the project. And as a major project hosted on GitHub,
>> it is not impossible that we could influence GitHub to smooth it out
>> to better suit our use-cases.
>>
>> Or, check out Hal Finkel's suggestion, which would allow casual
>> contributors to at least start out with their accustomed workflow
>> and then be funneled into LLVM's idiosyncratic review tool, which
>> is fundamentally user-hostile but our community has gotten used to.
>>
>> > I think many people agree that Phabricator is really good,
>>
>> And many people agree that Phabricator is a major pain.  It has
>> zero useful documentation (other than what the LLVM project has
>> written for its own use) and IME makes every effort to make useful
>> features difficult to find and use.  Ingenious people still manage
>> to find and use them, but Phab absolutely does not make it easy.
>>
>> > Getting reviewers [on GitHub] is a much bigger barrier
>>
>> This would depend on how new reviews are advertised.  With Phab, we
>> understand how they are advertised (i.e., mailing list).  With GitHub
>> this is apparently not automatic, but still automatable, IIUC.
>> --paulr
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191112/bc3bcf03/attachment.html>

Don Hinton wrote:
> my biggest problem on a daily basis is the use of force pushes to PR's,
Internally we're using PRs and people respond to comments with additional
commits, not force pushes (in general).  I'm able to look at individual
commits in the sequence.  Is this not how github.com PRs work?

Being branches in an individual's fork, of course we can't procedurally
forbid force-pushes, but social pressure makes a lot of things work out
that don't have technical solutions.
--paulr

On Tue, Nov 12, 2019 at 12:57 PM Robinson, Paul <paul.robinson at
sony.com>
wrote:
> Don Hinton wrote:
>
> > my biggest problem on a daily basis is the use of force pushes to
PR's,
>
> Internally we're using PRs and people respond to comments with
additional
> commits, not force pushes (in general).  I'm able to look at individual
> commits in the sequence.  Is this not how github.com PRs work?
>
Yes, I believe you're correct, but you can also do force push that replaces
the previous commits.  Not the end of the world, but can make it a bit more
difficult to see what got changed in each force push.  The idea is not to
have a bunch of typo fixes in the commits.  I've ended up doing the same
thing, just to keep what's ultimately merged to master be a single commit
-- why fight it, right?

>
> Being branches in an individual's fork, of course we can't
procedurally
> forbid force-pushes, but social pressure makes a lot of things work out
> that don't have technical solutions.
>
It'll be interesting seeing how that turns out, but my guess is it'll be
the wild west...  That's why I prefer Phab in general.  It makes is easy to
do it the "right" way.

> --paulr
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191112/7f4d8c3d/attachment.html>

On Tue, Nov 12, 2019 at 9:37 PM Syed Ahmed via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> For mutiple PRs, we have been using stacked PRs in the PyTorch repository.
Edward Yang wrote a tool for this called ghstack
(https://github.com/ezyang/ghstack) and my experience using it has been pleasant
- you put your patches as seperate commits on top of the master and call
ghstack, and ghstack separates out the commits into multiple PRs. This does
require the contributer to have permissions to create branches (we do that by
creating a team in the github project). The branches get created with a prefix
ghstack/<contributor-name>/*. After a PR is merged, stale branches are
deleted using a script by a maintainer. You can see an example stacked PR here:
https://github.com/pytorch/pytorch/pull/21364.
This looks surprisingly usable. It's sad that people need to jump
through such hoops to make the workflow that Git was originally
designed for feasible on GitHub, but hey, it's something!

A couple of questions:

1. I see a bunch of `Update on "Blah"` commits on those pull requests.
I assume that ghstack automatically and transparently creates those?

2. Where does the "Resubmit of #20886" come from? Does that happen
when you rebase? Some other condition?

3. How are changes to the commit message itself reflected in the pull requests?

4. How does the final submit work? It appears that the vanilla GitHub
machinery would fail because the pull requests are not actually set up
as pull requests against master. It seems PyTorch is running some bot
for that, is that an easily reproducible setup? Of course, the
original submitter could just push the original commits in the usual
vanilla way (e.g., via the CLI), but then how do the pull requests get
closed?

5. Is there a convenient way to get at the actual underlying commits,
so that a reviewer can look at them without the ghstack branching if
so desired? It seems this should be possible via the */orig branches?

Cheers,
Nicolai

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.