[Python-Dev] Re: Increase of Spammy PRs and PR reviews
On Sat, Jan 29, 2022 at 7:38 PM Inada Naoki wrote: > On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith wrote: > > > > As a non-committer, I want to make a plea for non-committer approval > reviews, or at least that they have a place. When asked how outsiders can > contribute I frequently see "review open PRs" as a suggested way of > contributing to CPython. Not being able to approve PRs that are good would > be a barrier to those contributions. > > > > Furthermore, I am collaborating with a couple of core devs, it would > make collaboration more difficult if I couldn't review their work and say > that I thought the changes looked good. > > > > You can still write a review comment, including "LGTM". Fair enough, I suppose commenting with "LGTM" works just as well. > What you can > not is labeling PR as "Approved." > So I don't think it makes collaboration difficult. > By preventing approval from others, we can easily find PRs approved > from core-devs or triage members but not merged yet. > > > I know "drive by approvals" are annoying but I think it is unfortunately > part of open source projects. > > > > Sorry, but I don't think so. > Well, if you disallow drive-by approvals, you will still get drive-by LGTM comments. People seem to believe that adding their approval will expedite a PR merge, for some reason (or want to bump a stale PR in hopes of a quicker merge). > > -- > Inada Naoki > ___ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-le...@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/QJ5QBB4XCFU3DFSOFBRUJB5XC4UMX7TK/ Code of Conduct: http://python.org/psf/codeofconduct/
[Python-Dev] Re: Increase of Spammy PRs and PR reviews
A non-core approval changes the label from "awaiting review" to "awaiting core review". I find this useful, and occasionally filter on "awaiting core review" because it indicates that at least one other person found the PR sound / interesting. I would not like this to change - I think high quality reviews from non-core contributors are valuable for the project and are a very quick way for the contributor to get the right kind of attention from core devs. If people spam the approvals (i.e., approve PRs without reviewing them) then the distinction between the labels becomes meaningless, of course. Though I do wonder what the motivation for doing that repeatedly would be. My basic assumption is that people usually try not to make fools of themselves. Note that contributors can still approve a perfect PR - a quality review in this case would include a brief comment indicating that you understand the change and perhaps why you find it valuable. On the matter of spammy PRs - I agree with both Rupert and Ethan (F): trivial PRs can add value, and a large number of them can be annoying. You can add value and be annoying at the same time (my triage work on b.p.o is probably in this category. Thankfully it's clear I'm not after "triage points", because there aren't any). At the end of the day, it doesn't really matter what a contributor's motivation is - it's up to the core devs to decide which PRs are valuable enough to merge, or even to review, and who they enjoy working with. These things tend to sort themselves out on their own. I don't think we need to restrict access for non-core contributors compared to the status quo. What I do think we need is to make it easier for core devs to close issues and PRs. We have a huge pile of open issues and PRs, some of which we know will never happen (stale or otherwise) and we don't close them because it's an unpleasant task to let someone down, and sometimes they argue, and we're volunteers and why should we bother with this emotional labour. Through triage I found many 6 year old issues that, once I refreshed and perhaps put an 'easy' label on, got fixed. The useless issues we don't want to close are obscuring the ones we can and should fix. I'm sure this has been discussed before. My only idea is that we formalize some guidelines/criteria for closing old issues and PRs that make it more of a joint decision of the core devs and less of a personal issue between the core dev and the contributor. I would not suggest a blanket 'close issues/PRs with no activity for X months', as I said I found useful 6 year old issues. It has to be more sophisticated than that. In summary - my view is that rather than making contributors contribute less, we should be more effective in reviewing the contributions, including rejecting those that should be rejected. On Sun, Jan 30, 2022 at 8:06 AM Ethan Smith wrote: > > > On Sat, Jan 29, 2022 at 7:38 PM Inada Naoki > wrote: > >> On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith wrote: >> > >> > As a non-committer, I want to make a plea for non-committer approval >> reviews, or at least that they have a place. When asked how outsiders can >> contribute I frequently see "review open PRs" as a suggested way of >> contributing to CPython. Not being able to approve PRs that are good would >> be a barrier to those contributions. >> > >> > Furthermore, I am collaborating with a couple of core devs, it would >> make collaboration more difficult if I couldn't review their work and say >> that I thought the changes looked good. >> > >> >> You can still write a review comment, including "LGTM". > > > Fair enough, I suppose commenting with "LGTM" works just as well. > > >> What you can >> not is labeling PR as "Approved." >> So I don't think it makes collaboration difficult. >> > By preventing approval from others, we can easily find PRs approved >> from core-devs or triage members but not merged yet. >> > >> > I know "drive by approvals" are annoying but I think it is >> unfortunately part of open source projects. >> > >> >> Sorry, but I don't think so. >> > > Well, if you disallow drive-by approvals, you will still get drive-by LGTM > comments. People seem to believe that adding their approval will expedite a > PR merge, for some reason (or want to bump a stale PR in hopes of a quicker > merge). > > >> >> -- >> Inada Naoki >> > ___ > Python-Dev mailing list -- python-dev@python.org > To unsubscribe send an email to python-dev-le...@python.org > https://mail.python.org/mailman3/lists/python-dev.python.org/ > Message archived at > https://mail.python.org/archives/list/python-dev@python.org/message/QJ5QBB4XCFU3DFSOFBRUJB5XC4UMX7TK/ > Code of Conduct: http://python.org/psf/codeofconduct/ > ___ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-le...@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/
[Python-Dev] Re: Increase of Spammy PRs and PR reviews
On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel wrote: > > If people spam the approvals (i.e., approve PRs without reviewing them) then > the distinction between the labels becomes meaningless, of course. Though I > do wonder what the motivation for doing that repeatedly would be. My basic > assumption is that people usually try not to make fools of themselves. > Some people may do "approve without review" to get attention from core dev. They just want to the issue be fixed soon. This is not so bad. Some people may do "approval without review" to make their "Profile" page richer, because GitHub counts it as a contribution. Creating spam issues or pull requests can be reported as spam very easily. But "approve without review" is hard to be reported as spam. So approving random issue is the most easy way to earn contributions without reported as spam. For example, see this user's contribution. They reviewed 32 pull requests in cpython. It seems they approves random pull requests after someone approved it. https://github.com/raghavthind2005 Of course, approving approved pull requests without review don't change the pull request status. So I can ignore them. I just explain "what the motivation approve without review repeatedly". I don't watch the cpython repository so I am not suffered from spammy approvals. So I have no vote for it. I just mention to an option we have. Regards, -- Inada Naoki ___ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-le...@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/UO6ZSNWLLXWU7AZ7NGQDTOQ2WVX2ZAZN/ Code of Conduct: http://python.org/psf/codeofconduct/
[Python-Dev] Re: Increase of Spammy PRs and PR reviews
On 1/30/22 04:45, Inada Naoki wrote: > On Sun, Jan 30, 2022 at 7:37 PM Irit Katriel > wrote: > Some people may do "approval without review" to make their "Profile" > page richer, because GitHub counts it as a contribution. > Creating spam issues or pull requests can be reported as spam very > easily. But "approve without review" is hard to be reported as spam. > So approving random issue is the most easy way to earn contributions > without reported as spam. Whnever there are metrics, some will find a way to game the system to make theirs look better - this certainly isn't limited to github, or to tech, or in any way a recent thing. ___ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-le...@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/CXMXAYP4JYNCCSDH3ISVMRZLWB7J6CPH/ Code of Conduct: http://python.org/psf/codeofconduct/
[Python-Dev] Re: Increase of Spammy PRs and PR reviews
El dom, 30 ene 2022 a las 2:40, Irit Katriel via Python-Dev (< python-dev@python.org>) escribió: > > A non-core approval changes the label from "awaiting review" to "awaiting > core review". > Interestingly, it also changes if a non-core dev requests changes (e.g., see https://github.com/python/cpython/pull/31021). Should that be considered a bug? > I find this useful, and occasionally filter on "awaiting core review" > because it indicates that at least one other person found the PR sound / > interesting. I would not like this to change - I think high quality > reviews from non-core contributors are valuable for the project and are a > very quick way for the contributor to get the right kind of attention from > core devs. > > If people spam the approvals (i.e., approve PRs without reviewing them) > then the distinction between the labels becomes meaningless, of course. > Though I do wonder what the motivation for doing that repeatedly would be. > My basic assumption is that people usually try not to make fools of > themselves. > > Note that contributors can still approve a perfect PR - a quality review > in this case would include a brief comment indicating that you understand > the change and perhaps why you find it valuable. > > On the matter of spammy PRs - I agree with both Rupert and Ethan (F): > trivial PRs can add value, and a large number of them can be annoying. You > can add value and be annoying at the same time (my triage work on b.p.o is > probably in this category. Thankfully it's clear I'm not after "triage > points", because there aren't any). At the end of the day, it doesn't > really matter what a contributor's motivation is - it's up to the core devs > to decide which PRs are valuable enough to merge, or even to review, and > who they enjoy working with. These things tend to sort themselves out on > their own. > > I don't think we need to restrict access for non-core contributors > compared to the status quo. What I do think we need is to make it easier > for core devs to close issues and PRs. We have a huge pile of open issues > and PRs, some of which we know will never happen (stale or otherwise) and > we don't close them because it's an unpleasant task to let someone down, > and sometimes they argue, and we're volunteers and why should we bother > with this emotional labour. > > Through triage I found many 6 year old issues that, once I refreshed and > perhaps put an 'easy' label on, got fixed. The useless issues we don't want > to close are obscuring the ones we can and should fix. > > I'm sure this has been discussed before. My only idea is that we formalize > some guidelines/criteria for closing old issues and PRs that make it more > of a joint decision of the core devs and less of a personal issue between > the core dev and the contributor. I would not suggest a blanket 'close > issues/PRs with no activity for X months', as I said I found useful 6 year > old issues. It has to be more sophisticated than that. > Agree, the count of 1.6k open PRs is not a good look for new contributors. We should be OK with closing an issue or PR if we are not going to make the change. > > In summary - my view is that rather than making contributors contribute > less, we should be more effective in reviewing the contributions, including > rejecting those that should be rejected. > > > On Sun, Jan 30, 2022 at 8:06 AM Ethan Smith wrote: > >> >> >> On Sat, Jan 29, 2022 at 7:38 PM Inada Naoki >> wrote: >> >>> On Sun, Jan 30, 2022 at 12:03 PM Ethan Smith wrote: >>> > >>> > As a non-committer, I want to make a plea for non-committer approval >>> reviews, or at least that they have a place. When asked how outsiders can >>> contribute I frequently see "review open PRs" as a suggested way of >>> contributing to CPython. Not being able to approve PRs that are good would >>> be a barrier to those contributions. >>> > >>> > Furthermore, I am collaborating with a couple of core devs, it would >>> make collaboration more difficult if I couldn't review their work and say >>> that I thought the changes looked good. >>> > >>> >>> You can still write a review comment, including "LGTM". >> >> >> Fair enough, I suppose commenting with "LGTM" works just as well. >> >> >>> What you can >>> not is labeling PR as "Approved." >>> So I don't think it makes collaboration difficult. >>> >> By preventing approval from others, we can easily find PRs approved >>> from core-devs or triage members but not merged yet. >>> >> >>> > I know "drive by approvals" are annoying but I think it is >>> unfortunately part of open source projects. >>> > >>> >>> Sorry, but I don't think so. >>> >> >> Well, if you disallow drive-by approvals, you will still get drive-by >> LGTM comments. People seem to believe that adding their approval will >> expedite a PR merge, for some reason (or want to bump a stale PR in hopes >> of a quicker merge). >> >> >>> >>> -- >>> Inada Naoki >>> >> ___ >> Py
[Python-Dev] Re: Increase of Spammy PRs and PR reviews
On Sun, Jan 30, 2022 at 08:36:43AM -0800, Jelle Zijlstra wrote: > Agree, the count of 1.6k open PRs is not a good look for new contributors. How does that compare to other comparable open source projects? Number of open PRs per KLOC seems like a more useful metric than just the number of open PRs. -- Steve ___ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-le...@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/QV4TXPY6KOERZTYG6FWPYRM7NGRIWH6K/ Code of Conduct: http://python.org/psf/codeofconduct/
[Python-Dev] Re: Increase of Spammy PRs and PR reviews
On Sun, 30 Jan 2022 at 20:36, Steven D'Aprano wrote: > > On Sun, Jan 30, 2022 at 08:36:43AM -0800, Jelle Zijlstra wrote: > > > Agree, the count of 1.6k open PRs is not a good look for new contributors. > > How does that compare to other comparable open source projects? How it compares is a separate question from how it looks to someone coming from "outside". This is a common problem in community-supported open-source but it is still a problem whichever way you look at it: most significant open source projects have large numbers of unresolved attempts at improvements. The problem is that an issue/PR tracker becomes meaningless when it's mostly filled with no chance of success suggestions/changes. > Number of open PRs per KLOC seems like a more useful metric than just > the number of open PRs. Maybe but either way it's better if there are fewer unresolved PRs sitting there. As Irit says above it's common (in CPython and also many other projects) that something sits there as unresolved even though it's obvious to the experienced that it will never go anywhere. The difficulty is that closing a PR entails awkwardness that is well described by Irit as "emotional labour". It's immediately easier to leave something to languish than to close it off regardless of what is better in the long term. Taking an alternate approach though: if a community cannot cope with the number of already incoming attempts to make changes then maybe it doesn't really benefit from signalling to new contributors that making changes is easy... -- Oscar ___ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-le...@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/OORVM4I5O7JBSLRI6UXBHSIED4S2OHXI/ Code of Conduct: http://python.org/psf/codeofconduct/
[Python-Dev] Re: [Steering-council] Re: PEP 651, Robust Stack Overflow Handling, Rejection notice
-cc: python-steering-council
On Fri, Mar 5, 2021 at 4:26 PM Guido van Rossum wrote:
> On Fri, Mar 5, 2021 at 11:11 AM Brett Cannon wrote:
>
>> Speaking for myself ...
>>
>
> Ditto ...
>
> On Fri, Mar 5, 2021 at 7:04 AM Mark Shannon wrote:
>> [...]
>>
>>> In some cases, the PEP would have improved the situation.
>>>
>>> For example:
>>> sys.setrecursionlimit(5000)
>>> def f():
>>> f()
>>>
>>> Currently, it raises a RecursionError on linux, but crashes the
>>> interpreter on Windows.
>>> With PEP 651 it would have raised a RecursionError on both platforms.
>>>
>>> Am I missing something here?
>>>
>>
>> So your example shows a user already comfortable in raising their
>> recursion limit to work around needing more stack space to reach
>> completion. What is stopping the user from continuing to raise the limit
>> until they still reach their memory limit even with PEP 651? If you're
>> worried about runaway recursion you will very likely hit that with the
>> default stack depth already, so I personally don't see how a decoupled
>> stack counter from the C stack specifically makes it any easier/better to
>> detect runaway recursion. And if I need more recursion than the default,
>> you're going to bump the recursion depth anyway, which weakens the
>> protection in either the C or decoupled counter scenarios. Sure, it's
>> currently platform-specific, but plenty of people want to push that limit
>> based on their machine anyway and don't need consistency on platforms they
>> will never run on, i.e. I don't see a huge benefit to being able to say
>> that an algorithm consistently won't go past 5000 calls on all platforms
>> compared to what the C stack protection already gives us (not to say
>> there's zero benefit, but it isn't massive or widespread either IMO). I
>> personally just don't see many people saying, "I really want to limit my
>> program to an exact call stack depth of 5000 on all platforms which is
>> beyond the default, but anything under is fine and anything over --
>> regardless of what the system can actually handle -- is unacceptable".
>>
>> Tack on the amount of changes required to give a cross-platform stack
>> count and limit check compared to the benefit being proposed, and to me
>> that pushes what the PEP is proposing into net-negative payoff.
>>
>
> To me, the point of that example is as a reminder that currently fiddling
> with the recursion limit can cause segfaults.
>
> Mark's PEP proposes two, somewhat independent, changes: (1) don't consume
> C stack on pure Python-to-Python (pp2p) function calls; (2) implement
> fool-proof C stack overflow checks.
>
> Change (2) makes it safe for users to mess with the stack overflow limit
> however they see fit. Despite (1), the limit for pp2p calls remains at 1000
> so that users who unintentionally write some naively recursive code don't
> have to wait until they fill up all of memory before they get a traceback.
> (Of course they could also write a while-True loop that keeps adding an
> item to a list and they'd end up in the same situation. But in my
> experience that situation is less painful to deal with than accidental
> stack overflow, and I'd shudder at the thought of a traceback of a million
> lines.)
>
> Given that we have (1), why is (2) still needed? Because there are ways to
> recursively call Python code that aren't pp2p calls. By a pp2p (pure
> Python-to-Python) call, I mean any direct call, e.g. a method call or a
> function call. But what about other operations that can invoke Python code?
> E.g. if we have a dict d and a class C, we could create an instance of C
> and use it to index d, e.g. d[C()]. This operation is not a p2pp call --
> the BINARY_SUBSCR opcode calls the dict's `__getitem__` method, and that
> calls the key's `__hash__` method. Here's a silly demonstration:
> ```
> def f(c):
> d = {}
> return d[c]
>
> class C:
> def __hash__(self):
> return f(self)
>
> f(C())
> ```
> Note that the "traceback compression" implemented for simple recursive
> calls fails here -- I just ran this code and got 2000 lines of output.
>
> The way I imagine Mark wants to implement pp2p calls means that in this
> case each recursion step *does* add several other C stack frames, and this
> would be caught by the limit implemented in (2). I see no easy way around
> this -- after all the C code involved in the recursion could be a piece of
> 3rd party C code that itself is not at fault.
>
> So we could choose to implement only (2), robust C stack overflow checks.
> This would require a bunch of platform-specific code, and there might be
> platforms where we don't know how to implement this (I vaguely recall a
> UNIX version where main() received a growable stack but each thread only
> had a fixed 64kb stack), but those would be no worse off than today.
>
> Or we could choose to implement only (1), eliminating C stack usage for
> pp2p calls. But in that case we'd still need a recursion limit for non-pp2p
> calls.
