Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Create an RFC Process #1662

Closed
wants to merge 9 commits into from

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Mar 15, 2021

Large PRs that are either complex or out of scope of current "focus area" efforts can often sit around, gated by feedback from cart. Creating an RFC process would give the community a useful tool to discuss complex proposals, especially if the proposed PR has implications for features that will get built on top.

A standard format would make it more likely that all the information needed to make a decision is accounted for and in a predictable format for cart and the broader community to digest. The intent here is not to create burden for contributors, but give the community a way of making headway on big decisions without PRs rotting in the queue.

I'd like to collaborate on a template, and discuss this meta-RFC here! We can make this RFC into the first RFC as a way of testing out the process. I've copied the Rust RFC template, per @alice-i-cecile's recommendation, as a starting point.

Edit 2021-04-01:

Current Status

Summary

We have consensus on:

  • Supporting an implementation-first process that has minimal overhead for contributors
  • Supporting a design-first process if community members want to collect information before implementing anything
  • Hosting the RFCs in a separate repo

Next Steps

  1. Create a new RFC repo in the bevy org
  2. Add the RFC template to the RFC repo
  3. Add the welcome message to the RFC repo README
  4. Remove the template and RFC folder from this PR
  5. Add info about RFCs to the bevy CONTRIBUTING.md as part of this PR

@alice-i-cecile alice-i-cecile added the A-Meta About the project itself label Mar 15, 2021
@cart
Copy link
Member

cart commented Mar 15, 2021

We should also discuss doing rfcs in a separate repo for a clean separation between implementation and spec (like rust does).

docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
@aevyrie
Copy link
Member Author

aevyrie commented Mar 15, 2021

We should also discuss doing rfcs in a separate repo for a clean separation between implementation and spec (like rust does).

Yeah, there's probably no need to carry rfcs around with the source.

@alice-i-cecile
Copy link
Member

We should also discuss doing rfcs in a separate repo for a clean separation between implementation and spec (like rust does).

I don't like this: it increases friction, makes it harder to notice / check for RFCs that need attention and makes cross-linking way more annoying. Github's autocomplete issue numbers are such a time saver and that will just stop working if we use a seperate repo.

@aevyrie
Copy link
Member Author

aevyrie commented Mar 15, 2021

That's a great point. Would only accepted PRs be merged into the repo? I guess it could also be useful for project history. Why did rust separate the rfcs?

Add Alice's suggested modifications.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile
Copy link
Member

Summarizing discussion on Discord.

Arguments in favor of One Repo

  • reduced friction
  • lower complexity
  • easier to discover new RFCs
  • slightly easier crosslinking of RFCs and issues / PRs, mostly due to autocomplete

Arguments in Favor of Two Repos

  • avoid cluttering source repo with docs
  • avoid cluttering main PR list with RFCs and vice versa
  • a single place to look for RFCs without filters
  • no or very minimal CI / other testing needed for RFCs
  • RFCs stay open until associated feature is merged
  • no need for either a) ultra-long RFC + implementation PR or b) two PRs per RFC in the same repo
  • RFCs can be frozen once approved
  • cleaner PR history to make release notes from

Tangential Points

  • Rust uses https://rfcbot.rs/; we don't need anything this complex and can just wire stuff up to a Github Projects Kanban board if we want something similar
  • the RFC repo should be wired into Discord like the main repo if we use a separate repo. Hopefully in a separate channel

@aevyrie
Copy link
Member Author

aevyrie commented Mar 15, 2021

no or very minimal CI / other testing needed for RFCs

This one seems like it is going to get more burdensome over time, especially if we start to test examples in some headless renderer.

no need for either a) ultra-long RFC + implementation PR or b) two PRs per RFC in the same repo
avoid cluttering source repo with docs
avoid cluttering main PR list with RFCs and vice versa

I think this alone is enough reason. Splitting all the meta discussion into a focused repo would prevent a good amount of cruft and clutter.

Something else to add in favor, is org members might be given extra permissions in the rfc repo, which cart might not be comfortable giving to the source repo. Not sure how significant this is, but it might mean more administrative work could be offloaded from cart.

Edit:

@alice-i-cecile, we can continue to work on the RFC template, the issue of where to host it won't be a blocker until we have consensus that the RFC template is good.

@ibraheemdev
Copy link

Wouldn't a git submodule address most of the listed concerns?

@alice-i-cecile
Copy link
Member

This one seems like it is going to get more burdensome over time, especially if we start to test examples in some headless renderer.

I'm not sure there's any way to get RFC examples to compile: by definition the required Bevy features don't exist ;) You could force people to mock out all of the new infrastructure but that sounds awful and pointless.

@aevyrie
Copy link
Member Author

aevyrie commented Mar 15, 2021

This one seems like it is going to get more burdensome over time, especially if we start to test examples in some headless renderer.

I'm not sure there's any way to get RFC examples to compile: by definition the required Bevy features don't exist ;) You could force people to mock out all of the new infrastructure but that sounds awful and pointless.

Oh, I meant that we wouldn't even be using the CI, so the amount of waste it adds is only going to go up in time as we add more tests.

@aevyrie
Copy link
Member Author

aevyrie commented Mar 16, 2021

Wouldn't a git submodule address most of the listed concerns?

That actually makes a ton of sense. What do you think @alice-i-cecile ?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 16, 2021

Wouldn't a git submodule address most of the listed concerns?

That actually makes a ton of sense. What do you think @alice-i-cecile ?

Seems sensible on its surface, but I worry about increased complexity and clutter still. I'm also not experienced enough with CI / git submodules to verify if it would work on a technical level.

However: quote from helpful Rust Discord user:

[8:04 PM] jynelson: submodules are very painful IMO
[8:05 PM] jynelson: since it's just documentation it should be a little easier, but they've caused rust-lang/rust no end of grief
[8:05 PM] jynelson: rust-lang/rust#76653 has lots of blathering about it

@alice-i-cecile
Copy link
Member

So, the other main issue we need to answer on this is "what needs an RFC, and what should I do if I just want to submit code".

Consensus is that simple PRs that implement:

  1. Bug fixes.
  2. Docs improvements.
  3. Simple quality of life features.
  4. Code quality improvements.

should be fully excluded from the RFC process.

IMO, new features should generally go through the RFC process if any of:

  1. There's more than one serious contender for the appropriate API or implementation.
  2. The feature is likely to be built on top of by other Bevy features.
  3. There's a high degree of complexity involved.
  4. There's enough work that the series of changes should be split into several PRs to make them easier to review.

@cart indicated that he would rather start the process overly strict for the 0.6 cycle, and then loosen back up as seems warranted.

@jamadazi's request was that the RFC process not be mandatory before new code PRs can be submitted.

With that in mind, I think we should support two workflows. The design-first workflow is more thoughtful and easier on Cart, but the implementation-first workflow is better for simple features and imposes less overhead.

Design-first Workflow

  1. An issue or discussion is opened laying out a limitation or feature request.
  2. Initial discussion occurs on the issue or discussion to get a feel for the challenges involved and the desirability of the feature.
  3. An RFC is opened, using the RFC PR template being created here.
  4. The RFC is discussed and refined by the community.
  5. The RFC is approved by Cart.
  6. Implementation work begins on a new code PR.
  7. Implementation is reviewed and polished by the community for the code PR.
  8. The final implementation is approved by Cart.

Implementation-first Workflow

  1. An issue or discussion is opened laying out a limitation or feature request.
  2. Initial discussion occurs on the issue or discussion to get a feel for the challenges involved and the desirability of the feature.
  3. A code PR is opened, laying out an initial attempt at solving the problem.
  4. If the scope is small and the solution is uncontroversial, go to step 7 of the design-first workflow.
  5. If the implementation needs more design work, the PR author or another community member creates a new RFC for the PR, linking the existing work. The existing code is put on hold and we go to step 3 of the design-first workflow.

@inodentry
Copy link
Contributor

Implementation-first Workflow

Just wanna say, I really like this. 😍

The way you have described the workflows makes perfect sense to me and i think it would work well. 👍

@alice-i-cecile
Copy link
Member

Just wanna say, I really like this. 😍

I'm thrilled to hear that: you're a great representative of the "process-lite" crowd and it's important that we don't make the bar / overhead too high.

@jyn514
Copy link

jyn514 commented Mar 16, 2021

However: quote from helpful Rust Discord user:
[8:04 PM] jynelson: submodules are very painful IMO

👋 helpful discord user here, I'd like to expand on why I think submodules were a bad choice for rust-lang/rust specifically. A lot of these don't apply to bevy, mostly I just feel like ranting 😆

  1. The main benefit submodules have over subtrees is they mean you don't have to download the history for the submodule immediately; this can be really helpful if you never end up needing the submodule (which is the point of Don't clone submodules for tools that aren't being built rust-lang/rust#76653; right now x.py throws away this benefit completely). However, in our case, and it sounds like yours too, most of the submodules have very little history compared to the main repository; the only exceptions are LLVM and rust-by-example.
  2. Submodules don't fit in with a normal git workflow. You have to learn a whole new command even if you're not changing the submodule. It's very easy to accidentally commit changes to the submodule (in fact, triagebot has a check specifically for it: https://github.com/rust-lang/highfive/blob/a5ef0920fabee02a77d3869918e519bc9a269741/highfive/newpr.py#L164-L165) and it's not obvious how to update the submodule once it gets in the wrong state.
  3. Submodules enforce an ordering: you have to update the repo of the submodule, then sync the submodule, and only then can you make the actual change. This can cause long delays (Update the bootstrap compiler rust-lang/rust#82076 (comment), Enable -W semicolon_in_expressions_from_macros in bootstrap rust-lang/rust#83089) and in some cases prevents changes that have to be done in sync (Make rustdoc lints a tool lint instead of built-in rust-lang/rust#80527 (comment)). It also adds a lot more maintenance burden for the submodule if it depends on the downstream repo - you may remember that rustfmt was unavailable for about 10 days before the latest beta release; submodules are the reason why. Rust had to invent the concept of "toolstate" for this. This might be less of an issue for bevy, since the RFCs aren't strongly tied to the code itself.

The alternative that rust-lang/rust is currently looking at is subtrees (rust-lang/rust#82208, rust-lang/rust#70651), which have different tradeoffs. They lose the benefit of 1, because all the history is kept in-tree, but 2 and 3 go away because you use normal git commands to make changes and don't have any enforced ordering. The main downside is that they are still mildly buggy (gitgitgadget/git#493), but that should only pop up for repos with lots of git history. Like submodules, you'll need to occasionally sync the RFCs repo downstream; if you modify the RFCs in-tree, you'll need to sync them back upstream.

For bevy specifically, you could also use neither and just have the folder in-tree. This isn't practical for rust-lang/rust because the tools have their own issue trackers and different reviewers than the main repository.

@inodentry
Copy link
Contributor

inodentry commented Mar 16, 2021

I believe that any changes to bevy's development process (1) should {improve,optimize,streamline} the workflow for {cart,reviewers,authors}, but also importantly (2) must not introduce any significant barriers to entry and overhead for contributions.

I am of the very strong opinion that (2) is more important than (1). If a proposal makes it more difficult or intimidating for people to contribute, it should not be accepted, no matter how much efficiency it would gain / how much work it would save for cart or reviewers. We should try to come up with proposals that can accomplish as much of (1) as can be done without compromising on (2).

Bevy has greatly benefited from its community and low barriers to entry and contribution. IMO maintaining that is essential to bevy's success.

Copy link
Contributor

@Ratysz Ratysz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would recommend rearranging lines with manual linebreaks until they come out at around 100 (or should it be 70) symbols.

docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
@Ratysz
Copy link
Contributor

Ratysz commented Mar 16, 2021

Re: design-first / implementation-first: agreed, it would be best if we allowed for both. Ideally, the "cut-off" point is not a rule either, and instead something that's decided on per-feature basis.

It should be noted that implementation-first workflow should not exempt the PR in question from providing a basic breakdown, ideally with an example or at least a link to the new tests/APIs/docs serving the same purpose.

@mockersf
Copy link
Member

I think we could try and be a little more welcoming, with something like that near the beginning:

You can open an incomplete RFC, and ask for help from the community to fill the remaining parts. If you are not ready to open an RFC, don't hesitate to open an issue to discuss your proposition

@alice-i-cecile
Copy link
Member

Should we include PR templates as part of this discussion, or in a follow-up? I'd like to have one for each of:

  1. Bug fix: link to the issue or briefly describe the bug. Use the "Closes #X" syntax.
  2. Code quality / docs. No text needed; read the code changes instead.
  3. Feature. Basic description, with links etc to the RFC process.

@aevyrie
Copy link
Member Author

aevyrie commented Mar 16, 2021

I think that will be better suited for a separate issue, and that we should keep this focused on the RFC template and process. I don't believe those are required for the RFC process to work, but would certainly be nice to have.

Co-authored-by: Alexander Sepity <ratysz@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some simple formatting improvements.

docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
docs/rfcs/0000-template.md Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

For the curious, I've used this PR as the base for my own RFC process / template for my game design work. I went with an archived / active split within the development repository to better fit the small team.

(There's no license in the repo yet, but consider this my Official Binding Blessing to use any of the RFC-related content in that repo under the MIT License.)

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@alice-i-cecile
Copy link
Member

A common pattern that I'm seeing in my planned RFC's is the need to compare several paths forward. The existing template doesn't cover it well; I think there's a good case for a second one.

For a real example of this pattern, see #1471.

@aevyrie
Copy link
Member Author

aevyrie commented Mar 26, 2021

Are you suggesting everything after the Motivation header becomes a sub-item of a list of proposals? E.g.:

  • Summary
  • Motivation
  • Proposal 1
    • Explanation
    • Drawbacks
    • Prior art, etc
  • Proposal 2
    • Explanation
    • Etc.

@alice-i-cecile
Copy link
Member

Are you suggesting everything after the Motivation header becomes a sub-item of a list of proposals?

Not quite: the pattern is different enough that I think it warrants an alternative template to better fit the use case. For example, you want to define a common set of metrics then evaluate all of the proposals across it.

I'll work on my own version of this for Leafwing Studios then link it here in a couple days :)

@aevyrie aevyrie mentioned this pull request Apr 2, 2021
@aevyrie
Copy link
Member Author

aevyrie commented Apr 2, 2021

Hey @alice-i-cecile, any progress on the aforementioned RFC version you were working on? I think we are at a good enough place here that we can kick this off as soon as 0.5 is merged, if cart is happy with this.

I'll update the first post to summarize the state of this PR.

@alice-i-cecile
Copy link
Member

any progress on the aforementioned RFC version you were working on

Delayed due to illness mostly. I don't think it's worth blocking the rest of the RFC process on: we can handle it internally on the RFC repo once that's made.

I think we are at a good enough place here that we can kick this off as soon as 0.5 is merged, if cart is happy with this.

Agreed. I expect that we'll actually want PR templates for this repo as part of this PR, and then strip the RFC template from this PR and move it into the new repo.

@alice-i-cecile
Copy link
Member

@aevyrie here's the Proposal Comparison RFC template that I'm using: link.

As you can see, it's quite different: a little terser, with a nice feature comparison grid or two. I think something similar would be very useful for Bevy in some cases, but I'm fine to add it in at a later date.

@alice-i-cecile
Copy link
Member

@huhlig suggests that we may also want an ADR template to go along with the RFC for less exploratory work. The template linked is quite similar to the Proposal Comparison template I listed just above.

@alice-i-cecile
Copy link
Member

I've made a temp RFC repo for now. Feel free to use it and open PRs against it if you'd like; with the understanding that we'll need to migrate soon.

Cart, when you're migrating, feel free to download and steal (rather than forking) that repo; I'm going to do my best to implement everything the official Bevy RFC repo should need in terms of files.

@aevyrie
Copy link
Member Author

aevyrie commented Apr 7, 2021

Thanks @alice-i-cecile.

@cart
Copy link
Member

cart commented Apr 8, 2021

Great work here everyone! I'm pretty much caught up now. In general I agree with the decisions made here. My biggest nits are:

  1. I think we should only have RFCS
    • I don't think "proposal comparisons" provide enough utility over RFCs to merit the additional cognitive load. RFCs should already provide comparisons to alternatives. Multiple "proposed rfcs / rfcs still in review" for a given design space should be adapted to account for each other.
    • I also think ADRs aren't "different" enough from RFCs to merit a separate template / system. RFCs feel open-ended enough to cover that problem space.
    • Multiple design doc types creates confusion / increases friction when creating a design. I don't want to have the conversation "should we create a proposal comparison doc for this" or "should this be an ADR instead". RFCs should both compare their implementation to alternatives and "record architectural decisions".
  2. This follows from (1): id rather have a flat structure than a folder structure for each RFC. Folders make navigation harder and deciding how to group rfcs feels like wasted effort.
  3. I don't see much utility in numbering RFCs. We can already reference them via their "github issue number" and their "unique feature name" (which we require in the title). Ensuring that we increment numbers correctly / avoid collisions feels like wasted effort.

@alice-i-cecile
Copy link
Member

Great. I can get behind all of these. We can revisit e.g. the proposal comparisons if we find a compelling need later.

I'll make the changes in the draft repo for you to save you some time :)

@aevyrie
Copy link
Member Author

aevyrie commented Apr 8, 2021

I'm on board with the nits.

My preference, based on the feedback here, is to err on the side of keeping a light touch with the RFC process. We can always add constraints to the process in the future, but there's no sense making things complex out of the gate.

I'd also like to avoid the perception that this represents Bevy becoming bureaucratic and process-heavy.

@JeanMertz
Copy link
Contributor

I don't see much utility in numbering RFCs. We can already reference them via their "github issue number" and their "unique feature name" (which we require in the title). Ensuring that we increment numbers correctly / avoid collisions feels like wasted effort.

Just to note that I like it when big projects add the RFC number (which can just be the PR number that merges the initial RFC) in the file name. This allows you to quickly t on the GitHub repository page, and type the PR number to find the relevant RFC (this differs from finding the PR with that PR number, since the RFC might have been updated in follow-up PRs since that PR was merged).

@inodentry
Copy link
Contributor

I'd also like to avoid the perception that this represents Bevy becoming bureaucratic and process-heavy.

Completely agree. That was my immediate concern when adding an RFC process to bevy was first proposed. I made sure to push back vocally. 😄

I think we now all seem to agree about this and the current proposal seems like it would work well for the "casual" and "freeform" nature of bevy. AIUI, it's mostly an option available to be invoked when necessary/beneficial, to give structure to community design discussions.

It's also why I think we should avoid introducing further "processes" and "bureaucracy", like ADRs or whatever. I see that cart agrees. 🙂

@cart
Copy link
Member

cart commented Apr 14, 2021

The Bevy RFC process is now live! Great work everyone!

https://github.com/bevyengine/rfcs

@cart cart closed this Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Meta About the project itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet