Simplified state diagram

[blaggacao]

Coming from #29, superseding #40, abiding by #29 (comment) :

• automatic reviewer assignment (making sure no PR gets drowned out in the queue),
• merger assignment (making sure reviews of non-committers mean something),
• easy discovery of PRs that need attention and
• automatic triage (making sure no PR gets lost due to inactive reviewers).

Also supersedes #22, #44

Link To SVG

Notes:

• /marvin go menas to gitub.undraft and LastMile.label
• /marvin merge means to assign another github reviewer (really: "merger") , which has committer rights

marmaidjs source

stateDiagram

  %% definitions
  Draft : Github
  Draft : Draft

  NoReviewPending : Github
  NoReviewPending : NoReviewPending ("all resolved")

  Merged : Github
  Merged : Merged

  Closed : Github
  Closed : Closed

  Stale : Stalebot
  Stale : Stale

  LastMile : Marvin
  LastMile : LastMile.label

  LastMeter : Marvin
  LastMeter : LastMeter.label

  %% going stale
  Draft --> Stale
  Stale --> Closed
  Closed --> [*]

  %% going marvin
  Draft --> LastMile : `/marvin go`.commented - anyone
  LastMile --> Assigned : <mv assignment heuristic>.done
  Assigned --> NoReviewPending : <github transition>.done
  NoReviewPending --> Merged : <manual action>.done - committers only
  NoReviewPending --> LastMeter : `/marvin merge`.commented - reviewers only
  LastMeter --> Merged : <manual action>.done - committers only
  Merged --> [*]

  %% internal marvin states during state Assinged & LastMeter
  state Assigned {
    _ReviewerIsUnresponsive : Marvin
    _ReviewerIsUnresponsive : ReviewerIsUnresponsive.internal_state
    [*] --> _ReviewerIsUnresponsive : <3 days w/o review>.passed
    _ReviewerIsUnresponsive --> [*] : <mv assignment heuristic>.done
  }
  note left of Assigned
      Github ...
  end note


  state LastMeter {
    _MergerRequested : Marvin
    _MergerRequested : MergerRequested.internal_state
    [*] --> _MergerRequested
    _MergerRequested --> [*] : <mv assignment heuristic-committers>.done
    --
    _MergerIsUnresponsive : Marvin
    _MergerIsUnresponsive : MergerIsUnresponsive.internal_state
    [*] --> _MergerIsUnresponsive : <3 days w/o merge>.passed
    _MergerIsUnresponsive --> [*] : <mv assignment heuristic-committers>.done
  }
  note left of LastMeter
      Marvin Label ...
  end note

Loading

Features:

• only introduces two actionable labels:
  • LastMile -> in review
  • LastMeter -> towards merge
• subjective reviewer primarily actionable filters are "one of those labels combined with assignment to self"
• subjective reviewer helping hands filters are "one of those labels; optionally with or—explicitly—without pending review"
• subjective author needs action filters are "one of those labels combined with pending review"

Short forms: (#41)

- /marvin go -> /m g
- /marvin merge -> /m m
- /marvin delegate @timokau -> /m d @timokau

[timokau]

I'm having a bit of a hard time reading that diagram, I'm not familiar with the notation. Maybe you could clarify, how does it solve these problems:

• identification of PRs where the reviewer has gone unresponsive without those where the author is unresponsive (that is what the awaiting_reviewer/awaiting_changes distinction is for)
• differentiating between PRs which currently need a reviewer and PRs which area already in review.

only introduces two actionable labels

The same is true for #29. Only needs_reviewer and needs_merger are actionable.

Re: short forms: I'm actively trying to make "needs merge" a very explicit action, so /m m is not desirable. I even thought about making people type out "I would merge this". Other short forms should not be necessary, since most other interaction is implicit (without explicit commands).

[blaggacao]

identification of PRs where the reviewer has gone unresponsive without those where the author is unresponsive (that is what the awaiting_reviewer/awaiting_changes distinction is for)

It is in the two boxes where the arrow has a description such as <3 days without ...>, meaning when one of the two labels is assigned and for X (eg. 3) days either:

• no review is submitted
• or no merge is done

then reassigns the review request to another reviewer. This can also be done manually by the first reviewer aka "delegation", but in any case after X days of not completing what's requested, marvin cycles.

• awaiting_changes is naturally any PR with "changes requested", either during LastMile or LastMeter which sets everyone's moods for a proper "Endspurt". Nice thing: the github PR list shows this quite neatly:
  

Any review which doesn't explicitly request changes and just does a comment without putting /marvin merge is maybe a zero-sum game that somebody's playing. There might be legit cases, though. Te keep everyone actionable, in such case, marvin should reassign.

differentiating between PRs which currently need a reviewer and PRs which area already in review.

The github APi exposes pending reviews, meaning: as long as any review is pending, it's in need of a review. In order to signal kind of the same, a merger is "disguised" as a Github review request combined with the LastMile flag.

Note that spontaneous reviews at any stage never affect any state, they are just plain additionally useful.

Re: short forms. Good point! Slang is repelling to new contributors.

[blaggacao]

The LastMile loop might actually need completion:

To cover the case a reviewer did approve or neutrally reviewer but did not assign another reviewer and also did not dare to request /marvin merge.

If the reviewer then decides to comment with /marvin merge, marvin should ensure the at least one pending merger reviewer is assigned, and if the current active reviewer is not, add also a merger reviewer.

[timokau]

identification of PRs where the reviewer has gone unresponsive without those where the author is unresponsive (that is what the awaiting_reviewer/awaiting_changes distinction is for)

It is in the two boxes where the arrow has a description such as <3 days without ...>, meaning when one of the two labels is assigned and for X (eg. 3) days either:

* no review is submitted

* or no merge is done

Yes, but how does the bot distinguish between PRs abandoned by the author and PRs abandoned by the reviewer?

then reassigns the review request to another reviewer. This can also be done manually by the first reviewer aka "delegation", but in any case after X days of not completing what's requested, marvin cycles.

This already happens.

* `awaiting_changes` is naturally any PR with "changes requested", either during `LastMile` or `LastMeter` which sets everyone's moods for a proper "Endspurt". Nice thing: the github PR list shows this quite neatly:
  ![image](https://user-images.githubusercontent.com/7548295/86994127-47e3e300-c16b-11ea-92c3-32730dcb842c.png)

Note that requesting reviews is not always possible, we can only request reviews from people who have "explicitly been granted read permission" to the repository. Marvin falls back to @mentions otherwise.

Any review which doesn't explicitly request changes and just does a comment without putting /marvin merge is maybe a zero-sum game that somebody's playing. There might be legit cases, though. Te keep everyone actionable, in such case, marvin should reassign.

I don't understand this point.

differentiating between PRs which currently need a reviewer and PRs which area already in review.

The github APi exposes pending reviews, meaning: as long as any review is pending, it's in need of a review. In order to signal kind of the same, a merger is "disguised" as a Github review request combined with the LastMile flag.

It is not possible to search for PRs that have any assigne/review requested (1, 2). Its also not always possible to assign reviewers, as mentioned before.

It would also make the information somewhat more implicit and harder to change manually / harder to understand the flow.

Note that spontaneous reviews at any stage never affect any state, they are just plain additionally useful.

I don't think that should be true. As said in #47 (comment), I'm trying to complement the "natural" review flow as much as possible and not replace it / force people to always actively work with the bot.

Re: short forms. Good point! Slang is repelling to new contributors.

👍

[timokau]

if the current active reviewer is not, add also a merger reviewer.

Again a technical limitation: We cannot reliably determine who is a "merger" and who is not (as long as they don't happen to be signed up as reviewers for marvin).

[blaggacao]

identification of PRs where the reviewer has gone unresponsive without those where the author is unresponsive (that is what the awaiting_reviewer/awaiting_changes distinction is for)

It is in the two boxes where the arrow has a description such as <3 days without ...>, meaning when one of the two labels is assigned and for X (eg. 3) days either:

* no review is submitted

* or no merge is done

Yes, but how does the bot distinguish between PRs abandoned by the author and PRs abandoned by the reviewer?

If an open review request is not "submitted" within the time frame, then the abandonment is of the reviewer.

If the author abandons, the PR get's stale, but since the author is supposed to have an interest to get PRs merged, I'd argue that there is no need for signalling that state other than closing stale PRs.

then reassigns the review request to another reviewer. This can also be done manually by the first reviewer aka "delegation", but in any case after X days of not completing what's requested, marvin cycles.

This already happens.

* `awaiting_changes` is naturally any PR with "changes requested", either during `LastMile` or `LastMeter` which sets everyone's moods for a proper "Endspurt". Nice thing: the github PR list shows this quite neatly:
  ![image](https://user-images.githubusercontent.com/7548295/86994127-47e3e300-c16b-11ea-92c3-32730dcb842c.png)

Note that requesting reviews is not always possible, we can only request reviews from people who have "explicitly been granted read permission" to the repository. Marvin falls back to @mentions otherwise.

For the sake of using those github-native states, would it be feasible to only assign reviews to those people or have marvin grant those rights automatically to all reviewers that signup for marvin?

Any review which doesn't explicitly request changes and just does a comment without putting /marvin merge is maybe a zero-sum game that somebody's playing. There might be legit cases, though. Te keep everyone actionable, in such case, marvin should reassign.

I don't understand this point.

I'm referring to the case a requested reviewer just comments, and does neither approve nor request changes. So this review isn't really actionable. It might in fact be an atrempt to delegate the review. At any rate, the ball should be passed on to another reviewer (to either approve or request changes).

differentiating between PRs which currently need a reviewer and PRs which area already in review.

The github APi exposes pending reviews, meaning: as long as any review is pending, it's in need of a review. In order to signal kind of the same, a merger is "disguised" as a Github review request combined with the LastMile flag.

It is not possible to search for PRs that have any assigne/review requested (1, 2). Its also not always possible to assign reviewers, as mentioned before.

I tacitly assumed that we can query any combination of github native states. However, when not possible, it shoukd be easy to compose the set based on set operations on available queries (against open & non-draft PRs).
This idea depends on beeing able to assign any marvin-reviewer.

It would also make the information somewhat more implicit and harder to change manually / harder to understand the flow.

I'd argue a little against this cause girthub has very prominent visuals and build in filters. It might rather feel more "natively" supported.

Note that spontaneous reviews at any stage never affect any state, they are just plain additionally useful.

I don't think that should be true. As said in #47 (comment), I'm trying to complement the "natural" review flow as much as possible and not replace it / force people to always actively work with the bot.

Re: short forms. Good point! Slang is repelling to new contributors.

---

Again a technical limitation: We cannot reliably determine who is a "merger" and who is not (as long as they don't happen to be signed up as reviewers for marvin).

For this idea, marvin could regard any assigned reviewer in LastMile as "merger". If this person happens to complete the review:

• with a merge -> finish
• with an approval -> look for another kbown merger
• with requested change -> do nothing, not even assign a new merged: it's the author's turn.

[blaggacao]

Or is the idea that marvin on one hand revourses on an internal list of reviewers while at the same time pretends signaling to an unkown third party? I think LastMile / LastMeter together with "approved" are powerful third party signallers.

[timokau]

identification of PRs where the reviewer has gone unresponsive without those where the author is unresponsive (that is what the awaiting_reviewer/awaiting_changes distinction is for)

It is in the two boxes where the arrow has a description such as <3 days without ...>, meaning when one of the two labels is assigned and for X (eg. 3) days either:

* no review is submitted

* or no merge is done

Yes, but how does the bot distinguish between PRs abandoned by the author and PRs abandoned by the reviewer?

If an open review request is not "submitted" within the time frame, then the abandonment is of the reviewer.

Let me rephrase that question: How does marvin discover those PRs using the GitHub API in a scalable manner?

If the author abandons, the PR get's stale, but since the author is supposed to have an interest to get PRs merged, I'd argue that there is no need for signalling that state other than closing stale PRs.

There are a lot of stale PRs in nixpkgs, life gets in the way.

Note that requesting reviews is not always possible, we can only request reviews from people who have "explicitly been granted read permission" to the repository. Marvin falls back to @mentions otherwise.

For the sake of using those github-native states, would it be feasible to only assign reviews to those people or have marvin grant those rights automatically to all reviewers that signup for marvin?

Possible yes (NixOS/rfcs#39), but it restricts our pool of potential reviewers and I'm not sure if it adds enough value to justify it.

Any review which doesn't explicitly request changes and just does a comment without putting /marvin merge is maybe a zero-sum game that somebody's playing. There might be legit cases, though. Te keep everyone actionable, in such case, marvin should reassign.

I don't understand this point.

I'm referring to the case a requested reviewer just comments, and does neither approve nor request changes. So this review isn't really actionable. It might in fact be an atrempt to delegate the review. At any rate, the ball should be passed on to another reviewer (to either approve or request changes).

People often use comments to request changes. I don't think its realistic to ask everybody to change their habits here -- people will just not use marvin and get annoyed by it. We should try to work with people's workflows, not against them.

I tacitly assumed that we can query any combination of github native states. However, when not possible, it shoukd be easy to compose the set based on set operations on available queries (against open & non-draft PRs).
This idea depends on beeing able to assign any marvin-reviewer.

Not sure what you mean by that. Exhaustively going through the search results and requesting the details on every PR will quickly eat through the rate limit.

It would also make the information somewhat more implicit and harder to change manually / harder to understand the flow.

I'd argue a little against this cause girthub has very prominent visuals and build in filters. It might rather feel more "natively" supported.

In order to find actionable PRs, potential reviewers would now have to look up the GitHub search syntax for review requests (which doesn't even really exist), assignees, drafts. Contrast that to simply searching for needs_reviewer. The potential reviewer would essentially have to repeat marvins internal heuristics. Its also not possible for the people involved in the PR to override these heuristics (currently that is possible and in my opinion valuable).

Note that spontaneous reviews at any stage never affect any state, they are just plain additionally useful.

I don't think that should be true. As said in #47 (comment), I'm trying to complement the "natural" review flow as much as possible and not replace it / force people to always actively work with the bot.

Re: short forms. Good point! Slang is repelling to new contributors.

Again a technical limitation: We cannot reliably determine who is a "merger" and who is not (as long as they don't happen to be signed up as reviewers for marvin).

For this idea, marvin could regard any assigned reviewer in LastMile as "merger". If this person happens to complete the review:

* with a merge -> finish

* with an approval -> look for another kbown merger

* with requested change -> do nothing, not even assign a new merged: it's the author's turn.

Again, this makes things more complicated and less transparent. Its also again not possible to discover these PRs and run triage on them. What do we gain?

Or is the idea that marvin on one hand revourses on an internal list of reviewers while at the same time pretends signaling to an unkown third party? I think LastMile / LastMeter together with "approved" are powerful third party signallers.

What do you mean by "pretends signaling to an unknown third party"?

[timokau]

I'm inclined to close this, as I don't see what the changes would give us. In my view, they simply move some of the external state to internal logic. While that may seem like a simplification at first, it only hides/moves the complexity. It makes the logic harder / impossible to implement withing GitHubs restrictions. It also the bots actions less transparent, makes it harder for people to filter PRs and harder to spot and manually fix up errors in the heuristic.

Do you disagree?

[blaggacao]

I really apreciate the discussion. I'm not able to defend the idea further in an actionable manner.

My gut tells me the "flight height" of LastMile and LastMeter concept is very easy to grasp and eventually even more concice since it's linear and only two step: It abstracts away all those complex edge cases which will be, so is my gut fear and main motivation, almost impossible to implement without resulting in a feature creep induced dead lock at some point. (Forward looking statement, at risk of beeing wrong).

I also think technical limitations can eventually be circumvented pulling the right levers. With internal state definitely more easily than with external/label state.

For now, I'm really happy to have discussed it with you and that you probably are in a position where you mught draw inspiration from some of those ideas if that seems like a good fit later on, so I'm closing this. Thanks! 👍

[timokau]

Thanks for the ideas! For the moment, I do think that external state is both easier to implement and easier to understand. Especially since its easy to override the heuristics. Lets see how that works out. A similar discussion will probably come up again when I revive rfc#30.