[Caml-list] Recent hooks design and general github PR issues

[Caml-list] Recent hooks design and general github PR issues

[Yotam Barnoy]

I would like to alert the list to a potential problem with github PRs,
and a recent example of such a problem.

I've noticed that recently, there has been an insertion of a
particular feature into the compiler. This feature involves
potentially long-term commitment in terms of maintenance, and yet I
have not seen it or its costs vs benefits being discussed fully in any
particular place. I'm specifically talking about PRs
https://github.com/ocaml/ocaml/pull/648,
https://github.com/ocaml/ocaml/pull/668 and
https://github.com/ocaml/ocaml/pull/647.

The problem with github PRs is that they allow you to insert features
piecemeal. This splits up the discussion across multiple PRs, making
it very difficult to have a discussion about the feature as a whole,
and making it seem like there is consensus when there might not be.

As a rule, I recommend that any such large feature spanning multiple
PRs first require discussion either on the list or on mantis, to
concentrate discussion in one place.

It may also be worthwhile to say that except for rare exceptions
(mostly bug fixes), PRs should not be merged by the same person who
authored them, as this makes the process seem biased and questionable.

I don't know if this feature in itself is problematic, but I've seen
several core members trying to voice concerns about the feature in one
PR, while other member PRs were simply merged into the tree. In my
opinion, questions on sub-PRs of a feature should inhibit merging any
parts of said feature.

This may be a false alert, but I think it is worth clarifying if it is
indeed one, and at the very least, the protocol should be set for the
future.

-Yotam

Re: [Caml-list] Recent hooks design and general github PR issues

[Fabrice Le Fessant]

[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]

Hi,

>   several core members trying to voice concerns about the feature in one PR,
while other member PRs were simply merged into the tree

Could you be more specific ? I think I have taken the time to discuss all
these PRs (for 20 days for 2 of them, 13 days for the other one), I have
modified the PRs to follow the requests that were voiced (for example, by
removing the default -fPIC) and they were all reviewed by other core
developers. If you think some concerns have not been taken into account,
please, tell me.

>  yet I have not seen it or its costs vs benefits being discussed fully in
any particular place

Probably because the cost vs benefit is very hard to measure in most PR:
the benefit might be small for some users, and big for other ones; some
features might be hard to understand and to use, yet one developer can use
them to develop a tool that can be used by many users; some features might
look of little use at the beginning, yet, once they are present, other
users might find usages that were not foreseen in the PR discussion.

--Fabrice


On Tue, Jul 19, 2016 at 5:09 PM Yotam Barnoy <yotambarnoy@gmail.com> wrote:

> I would like to alert the list to a potential problem with github PRs,
> and a recent example of such a problem.
>
> I've noticed that recently, there has been an insertion of a
> particular feature into the compiler. This feature involves
> potentially long-term commitment in terms of maintenance, and yet I
> have not seen it or its costs vs benefits being discussed fully in any
> particular place. I'm specifically talking about PRs
> https://github.com/ocaml/ocaml/pull/648,
> https://github.com/ocaml/ocaml/pull/668 and
> https://github.com/ocaml/ocaml/pull/647.
>
> The problem with github PRs is that they allow you to insert features
> piecemeal. This splits up the discussion across multiple PRs, making
> it very difficult to have a discussion about the feature as a whole,
> and making it seem like there is consensus when there might not be.
>
> As a rule, I recommend that any such large feature spanning multiple
> PRs first require discussion either on the list or on mantis, to
> concentrate discussion in one place.
>
> It may also be worthwhile to say that except for rare exceptions
> (mostly bug fixes), PRs should not be merged by the same person who
> authored them, as this makes the process seem biased and questionable.
>
> I don't know if this feature in itself is problematic, but I've seen
> several core members trying to voice concerns about the feature in one
> PR, while other member PRs were simply merged into the tree. In my
> opinion, questions on sub-PRs of a feature should inhibit merging any
> parts of said feature.
>
> This may be a false alert, but I think it is worth clarifying if it is
> indeed one, and at the very least, the protocol should be set for the
> future.
>
> -Yotam
>
> --
> Caml-list mailing list.  Subscription management and archives:
> https://sympa.inria.fr/sympa/arc/caml-list
> Beginner's list: http://groups.yahoo.com/group/ocaml_beginners
> Bug reports: http://caml.inria.fr/bin/caml-bugs
>

[-- Attachment #2: Type: text/html, Size: 4482 bytes --]

Re: [Caml-list] Recent hooks design and general github PR issues

[Yotam Barnoy]

See the comments at the bottom of
https://github.com/ocaml/ocaml/pull/668 by avsm and gasche, which can
be seen as asking question about the whole feature (not just the PR).
See also Shinwell's concerns at the bottom of
https://github.com/ocaml/ocaml/pull/647. And note that since they are
on separate PRs, they most likely did not see each others' comments,
which is a problem.

In general, I think the process has to be refined. As this is all part
of one feature, that feature should be discussed first before any
merging begins. And merging should not be done by the person
suggesting the feature.

-Yotam

On Tue, Jul 19, 2016 at 11:36 AM, Fabrice Le Fessant
<Fabrice.Le_fessant@inria.fr> wrote:
> Hi,
>
>>   several core members trying to voice concerns about the feature in one
>> PR, while other member PRs were simply merged into the tree
>
> Could you be more specific ? I think I have taken the time to discuss all
> these PRs (for 20 days for 2 of them, 13 days for the other one), I have
> modified the PRs to follow the requests that were voiced (for example, by
> removing the default -fPIC) and they were all reviewed by other core
> developers. If you think some concerns have not been taken into account,
> please, tell me.
>
>>  yet I have not seen it or its costs vs benefits being discussed fully in
>> any particular place
>
> Probably because the cost vs benefit is very hard to measure in most PR: the
> benefit might be small for some users, and big for other ones; some features
> might be hard to understand and to use, yet one developer can use them to
> develop a tool that can be used by many users; some features might look of
> little use at the beginning, yet, once they are present, other users might
> find usages that were not foreseen in the PR discussion.
>
> --Fabrice
>
>
> On Tue, Jul 19, 2016 at 5:09 PM Yotam Barnoy <yotambarnoy@gmail.com> wrote:
>>
>> I would like to alert the list to a potential problem with github PRs,
>> and a recent example of such a problem.
>>
>> I've noticed that recently, there has been an insertion of a
>> particular feature into the compiler. This feature involves
>> potentially long-term commitment in terms of maintenance, and yet I
>> have not seen it or its costs vs benefits being discussed fully in any
>> particular place. I'm specifically talking about PRs
>> https://github.com/ocaml/ocaml/pull/648,
>> https://github.com/ocaml/ocaml/pull/668 and
>> https://github.com/ocaml/ocaml/pull/647.
>>
>> The problem with github PRs is that they allow you to insert features
>> piecemeal. This splits up the discussion across multiple PRs, making
>> it very difficult to have a discussion about the feature as a whole,
>> and making it seem like there is consensus when there might not be.
>>
>> As a rule, I recommend that any such large feature spanning multiple
>> PRs first require discussion either on the list or on mantis, to
>> concentrate discussion in one place.
>>
>> It may also be worthwhile to say that except for rare exceptions
>> (mostly bug fixes), PRs should not be merged by the same person who
>> authored them, as this makes the process seem biased and questionable.
>>
>> I don't know if this feature in itself is problematic, but I've seen
>> several core members trying to voice concerns about the feature in one
>> PR, while other member PRs were simply merged into the tree. In my
>> opinion, questions on sub-PRs of a feature should inhibit merging any
>> parts of said feature.
>>
>> This may be a false alert, but I think it is worth clarifying if it is
>> indeed one, and at the very least, the protocol should be set for the
>> future.
>>
>> -Yotam
>>
>> --
>> Caml-list mailing list.  Subscription management and archives:
>> https://sympa.inria.fr/sympa/arc/caml-list
>> Beginner's list: http://groups.yahoo.com/group/ocaml_beginners
>> Bug reports: http://caml.inria.fr/bin/caml-bugs

Re: [Caml-list] Recent hooks design and general github PR issues

[Fabrice Le Fessant]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

On Tue, Jul 19, 2016 at 6:24 PM Yotam Barnoy <yotambarnoy@gmail.com> wrote:

> See the comments at the bottom of
> https://github.com/ocaml/ocaml/pull/668 by avsm and gasche


I replied to Gabriel, who was asking for a different API, that could be
done, but is not my intent (OCamlPro needs what I implemented, not what he
was discussing, so if somebody else wants to contribute the complementary
approach, he is welcome), and Anil just said he didn't see the benefit of
the interception hooks, which is (1) only one part of the PR (cplugins can
be used to do other things, such as have a more portable version of
OCamlPro's Memory Profiler) and (2) there are several examples on
github.com/OCamlPro/ocp-cplugins that show that the restriction to the
stdlib provides already interesting use cases.


> See also Shinwell's concerns at the bottom of
> https://github.com/ocaml/ocaml/pull/647


Mark was commenting on the maintainability of ppx at Jane Street, not about
the maintainability of the ppx code inside the compiler. The discussion was
diverging from the actual subject of the PR at that point.


> . And note that since they are on separate PRs, they most likely did not
> see each others' comments, which is a problem.
>

I don't understand that part: there is no link between the two PRs, one is
about plugins in the runtime, the other one about plugins in the compiler,
which have completely different use cases and cost/benefits.


> In general, I think the process has to be refined. As this is all part
> of one feature, that feature should be discussed first before any
> merging begins. And merging should not be done by the person
> suggesting the feature.


I think it is quite common in the core team, especially before a code
freeze. Check all the closed/merged PRs with the black tag "Spacetime
prerequisite" for an easy example. The current process is to have at least
one code review by another core developer and no strong objection by
another core developer. I think it is a good trade-off between fast
evolution while keeping good code quality.

Finally, about the maintainability, I think having plugins both in the
runtime and the compiler will help externalizing some code from the
distribution, thus decreasing the maintenance cost of the distribution,
while allowing external contributors to extend the runtime and compiler
through OPAM packages.

Best regards,
--Fabrice

[-- Attachment #2: Type: text/html, Size: 3519 bytes --]

Re: [Caml-list] Recent hooks design and general github PR issues

[Alain Frisch]

On 19/07/2016 17:08, Yotam Barnoy wrote:
> It may also be worthwhile to say that except for rare exceptions
> (mostly bug fixes), PRs should not be merged by the same person who
> authored them, as this makes the process seem biased and questionable.

Open source projects have leaders (Xavier and Damien, for OCaml), and 
other core maintainers with more decision power than the community.  And 
in the community, the opinions of some people have more weight than 
those of others.  Of course, the process is biased!

But compare to the situation before the switch to Git and GitHub: new 
developments were almost always pushed/committed by the author if they 
had commit rights, sometimes discussed (well, usually evoked) on 
caml-devel before, and even less often on Mantis.

The new situation is much more open and the level of review has 
increased significantly.  When someone with commit rights submits a PR, 
this is more community-friendly than pushing directly to trunk (which is 
not forbidden, and still happens), even if the same person finally does 
the merge after taking comments (from everyone) into account.  Possibly 
nobody feels like commenting, reviewing, nor endorsing the proposal; 
this is not ideal, but I don't think this should prevent merging after a 
reasonable amount of time, preferably also after mentioning the imminent 
merge.


Alain

Re: [Caml-list] Recent hooks design and general github PR issues

[Goswin von Brederlow]

On Tue, Jul 19, 2016 at 10:21:14PM +0200, Alain Frisch wrote:
> On 19/07/2016 17:08, Yotam Barnoy wrote:
> > It may also be worthwhile to say that except for rare exceptions
> > (mostly bug fixes), PRs should not be merged by the same person who
> > authored them, as this makes the process seem biased and questionable.
> 
> Open source projects have leaders (Xavier and Damien, for OCaml), and other
> core maintainers with more decision power than the community.  And in the
> community, the opinions of some people have more weight than those of
> others.  Of course, the process is biased!
> 
> But compare to the situation before the switch to Git and GitHub: new
> developments were almost always pushed/committed by the author if they had
> commit rights, sometimes discussed (well, usually evoked) on caml-devel
> before, and even less often on Mantis.
> 
> The new situation is much more open and the level of review has increased
> significantly.  When someone with commit rights submits a PR, this is more
> community-friendly than pushing directly to trunk (which is not forbidden,
> and still happens), even if the same person finally does the merge after
> taking comments (from everyone) into account.  Possibly nobody feels like
> commenting, reviewing, nor endorsing the proposal; this is not ideal, but I
> don't think this should prevent merging after a reasonable amount of time,
> preferably also after mentioning the imminent merge.
> 
> 
> Alain

Many other projects use the rule that PR author != merger. Even
leaders make mistakes and this simple rulte gives each PR a second set
of eyes.

+1 to that suggestion.

MfG
	Goswin

Re: [Caml-list] Recent hooks design and general github PR issues

[Fabrice Le Fessant]

[-- Attachment #1: Type: text/plain, Size: 214 bytes --]

On Thu, Jul 21, 2016 at 2:09 PM Goswin von Brederlow <goswin-v-b@web.de>
wrote:

> Many other projects use the rule that PR author != merger.


But even more other projects don't use that rule, no ? ;-)

--Fabrice

[-- Attachment #2: Type: text/html, Size: 499 bytes --]

Re: [Caml-list] Recent hooks design and general github PR issues

[Hendrik Boom]

On Thu, Jul 21, 2016 at 12:38:56PM +0000, Fabrice Le Fessant wrote:
> On Thu, Jul 21, 2016 at 2:09 PM Goswin von Brederlow <goswin-v-b@web.de>
> wrote:
> 
> > Many other projects use the rule that PR author != merger.
> 
> 
> But even more other projects don't use that rule, no ? ;-)

Certainly one-man projects don'e/

-- hendrik