changed 5 years ago  
Linked with GitHub

Major Change Process

Summary

  • We want some kind of system where people advertise changes that they are making or plan to make
    • and the team can give high-level feedback early
    • and – if we decide to go with the change – we can ensure there is a reviewer beforehand
  • This document describes motivations and a specific "early draft" proposal

Motivations

Proposal is to add some sort of notification / lightweight process before making major changes.

In contrast, today there is no stated process for a "major change" beyond just opening a PR.
Of course some changes get a lot of discussion before hand, but others do not.

Some problems that we see with today's system:

  • Sometimes we get large PRs that have attempted a major change without any discussion beforehand
    • There may not be a reviewer with time and interest
    • Also reviewing without context is very difficult
    • There may be concerns about the technical approach or direction
    • Starts the discussion with "accept or reject this PR" versus "what is best approach"
      • On the other hand, a more concrete discussion can be more effective
  • Often, as a result, these PRs can sit for a very long time without any feedback
    • this is frustrating for everyone involved
  • Current "lack of system" can also be a turnoff
    • do a lot of work to prep PR, but can you know if that change is welcome?
  • Over time, a focus on PRs (versus explaining the idea) leads to
    • more tech debt and less overall cohesion
    • lack of documentation

Some strengths of today's system that we would like to preserve:

  • Low barrier to entry, not a lot of "bureaucratic overhead"
  • People should be able to experiment without "authorization" before hand
  • We don't want a lot of overhead for the compiler team to manage some authorization process

We would know the system is working if:

  • We have a better idea of what is being done and by whom
    • and also whether the team has approved of that direction
  • Major changes will be discussed before they become a PR that is up for active review
    • reviewers will be identified beforehand
  • When reviewing a PR, reviewers will have a better idea of its goals and how it fits into the bigger picture
  • We still have a "high throughput" system and we don't spend a ton of time on "bureaucratic overhead"
    • in particular we should be able to "green light" changes fairly quickly and we should do that most of the time

Initially proposed process

  • When considering or experimenting with a "major change", open an issue on compiler-team repo to let people know
    • Describe the general idea in a sentence or two
    • Identify mentors or reviewers, if you are working with one
  • These issues will be reviewed weekly and classified
    • New triage process? Part of existing process?

      Felix S Klock II Note we already often go-over allotted time at the Thursday triage meeting. We can add this to the agenda, but we need to figure out what else will get de-prioritized.

    • Is weekly a good frequency?
      • mark: this is pretty high latency for some of these changes,
        we might otherwise merge them in a week's time
        Maybe that's not a bad thing though :)
  • Some possible outcomes:
    • Closed – this doesn't seem like something we want to do
    • Requires design meeting – requires a larger group

    Felix S Klock II should also add "Needs RFC" as potential outcome (a more extreme variant on "Needs Design Mtg")

    • Deferred – not deciding for now, or trying to find a reviewer
    • Approved for experimentation
      • once PR is ready, nominate for discussion
      • may request "mitigation", such as a -Z flag
      • implementor should understand that we may just decide the idea isn't worth it
    • Approved to land – requires a willing reviewer
      • no special approval required to land, just r+
  • If a "major change" PR is opened without going through this process
    • Close with a friendly note recommending an issue be opened (we should write a standard template with a link)

What is a "major change"?

  • "You know it when you see it"
  • If it is a major time commitment to review it, it probably qualifies
    • This might be beause it affects many parts of the compiler
    • But it might also be a narrow change with subtle implications
    • Or require reading up on a relevant RFC or other background material
  • Examples of major changes:
    • Allocate HIR on an arena:
      • Yes, because it touches a set of data structures used throughout the whole compiler
      • However, presuming we could find a reviewer, this would be something we'd like approve quickly within the initial triage because it is fairly mechanical and doesn't require a design meeting
    • Introducing chalk, nll, or polonius
      • Yes, replacing a major component
      • This kind of effort might be "redirected" to forming a working group to help lead the design and implementation
        • (which is of course what we have done)
    • Changing universally used internal APIs
  • Grey zone, let's discuss:
  • Examples of minor changes:
    • Fix some ICE
    • Local optimizations

Why this will help

  • For people who have very full calendars, being able to have a "heads up" of larger changes and to integrate into a review schedule could be helpful
    • but it will take discipline to use effectively

Notes from the meeting

  • simulacrum points out that it would be good to have some kind of "fast path" if you have a reviewer and you have documented things, so that no meeting is required at all
    • the only "hard block" would be if you don't have a "partner" or "sponsor" from compiler team
    • somewhat analogous to the project group lang team concepts
  • reviewer not expected to be a pair programmer
  • one possible definition for "what is a major change" might be "what would modify the rustc-guide"
    • or, since rustc-guide is always a WIP, "welp this should've in the guide and if it were, it would've required a modification"
  • what to do with new PRs that don't follow the process?
    • should we close them?
    • maybe have a canned comment and give them some amount of time
    • this comment might also emphasize the role of documentation
  • we discussed and settled on
    • leave a nice message, which it S-waiting-on-author
    • close per usual triage process if no issue is filed within a certain amount of time
    • if an issue is filed but it is not "green lighted", then we can close the PR
      • i.e., if we decide that a design meeting or broader project group is needed
  • when a project is proposed, what are the possible responses?
    • I have concerns
    • I approve but don't have ability/time to review
    • I approve on an experimental basis; we should discuss again when we gain more experience
    • I am happy to review but I would like another to approve too
    • I am happy to review and I think we can just go forward (only possible for "members")
  • can we make a Zulip stream where each issue creates a topic?
  • how to handle experimentation?
    • we should have some way to add "caveats", like
      • would like to review performance results
      • we need a -Z flag
      • we need docs :)
  • final discussion point was about exactly how to handle requests for rustc-guide edits
    • since a major change is part of a rustc-guide change, it makes sense that it should come accompanied with a rustc-guide write-up
    • ideally this would come along with the compiler-team issue
      • but maybe it would be more something we wait for until issue is approved or, in extreme cases, co-develop with author
    • if we want to see more docs, we are going to have to start holding the line somewhere
    • sometimes it's not possible or desirable to write complete docs before-hand
      • details may change through review process
      • person may not know enough context to write the docs, need help with that
    • but the bar should be that the issue can explain the change in sufficient detail for it to be understood
      • the actual rustc-guide changes themselves can come later
      • it may be that the role of the learning wg can be to help with some of that

Final proposed process

  • When considering or experimenting with a "major change", open an issue on compiler-team repo to let people know
    • Describe the general idea in a sentence or two
    • Identify mentors or reviewers, if you are working with one
    • There will be some "prototype" to guide people in this
  • What is a "major change"?
    • something where it would make sense to update rustc-guide
    • if rustc-guide doesn't cover this code yet, then you may have to use your imagination about what ought to be documented :)
  • These issues will be reviewed by compiler team members
    • Compiler team members and contributors can leave concerns and approvals asynchronously (see below)
      • maybe we can make a dedicated Zulip stream where new things get
    • In particular, note that a compiler team member who is confident something is correct and will not be controversial can just go ahead and approve and act as reviewer
      • though there should still be an issue
    • But there should also be some synchronous sweep, not clear when that should occur
      • maybe as part of meta wg?
      • perhaps just team co-leads do it on a regular basis?
      • existing triage meeting is too full, that's clear
  • Feedback from a compiler team member or contributor typically has the form (these are not fully orthogonal):
    • I have concerns (give details)
      • this might lead to more details
      • or a design meeting
      • or an RFC
      • or just closing the idea
    • I approve but don't have ability/time to review
    • I approve but with some caveats, e.g. we should examine perf afterwards, would like to re-review, or want a -Z flag
    • I am happy to review but I would like someone else to approve too (must be a "compiler team contributor")
    • I am happy to review and I think we can just go forward (only possible for "members")
      • the idea here is that if you are an expert on the code and confident this is a good path, that's fine, do it
  • If a "major change" PR is opened without going through this process
    • We post a standard comment that directs people to open an issue
    • And the PR is marked as waiting on author
      • It can be closed per the usual triage process if author does not respond
    • If the issue turns out to be controversial (i.e., nobody steps up as reviewer write away), then we close the PR and just focus on discussing in the issue
Select a repo