Skip to content

Unstable Features for faster iterations #52

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.

Sign up for GitHub

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Unstable Features for faster iterations #52

Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a85a7c3
start rfc for unstable features
mockersf Mar 5, 2022
b532dc5
add pr number
mockersf Mar 5, 2022
2eaad9a
Update rfcs/52-unstable_features.md
mockersf Mar 5, 2022
59342db
propose a convention for feature gate name
mockersf Mar 5, 2022
2f37437
small nits
mockersf Mar 5, 2022
19574c1
nit on name convention
mockersf Mar 5, 2022
d0c0d43
add details on RFC <-> unstable feature
mockersf Mar 5, 2022
2f05fae
add a potential drawback
mockersf Mar 5, 2022
1de8d91
propose organisation and discovery through github project
mockersf Mar 5, 2022
53f3d1f
add drawback on potential breakage of an unstable feature from other PR
mockersf Mar 6, 2022
4cc6784
typo
mockersf Mar 6, 2022
76b7e38
typos and reformulations
mockersf Mar 7, 2022
0009331
adding alternative
mockersf Mar 7, 2022
accd039
adding discussion on Rust RFCs
mockersf Mar 7, 2022
a37bd9c
other motivations
mockersf Mar 7, 2022
c063ad9
details on drawback
mockersf Mar 7, 2022
3d5766f
add info on other PR interactions
mockersf Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions rfcs/52-unstable_features.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Feature Name: `unstable_features`

## Summary

Accepting PRs for new features that may not have reach the high bar we set for Bevy, and disable their changes with a feature gate.

## Motivation

Some features would benefit from iterative work in Bevy, but that may be hard to do when a PRs need a high level of quality to be merged in.

In this RFC, "feature" means the new feature or change under development, and "feature gate" the conditional compilation flag. "unstable" means a feature that is subject to change, not something that will break Bevy.

## User-facing explanation

So you want to contribute a new feature to Bevy! You have a few paths available to you, depending on the scope of the feature:
- A small feature with a clear implementation? Go ahead and open that PR!
- A large feature with an open design space that could use some discution about how to implement it? A RFC would be good.
- Something in between that would benefit from iterative work? Here come the unstable feature workflow for you!
Copy link
Member

Choose a reason for hiding this comment

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

I think large RFC'ed changes should also be subject to this workflow too. It'd also help isolate the effects of implementing individual parts of the larger design before it becomes stabilized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I agree, but in those case I think the workflow should be RFC first, then unstable feature once the RFC is more or less agreed on. I tried to mention that in the "implementation" part, would that work for you?

Copy link
Member

Choose a reason for hiding this comment

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

I see it now, but it's not particularly focused upon. Could you expand on each potential workflow down below?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried!


You will need to open a tracking issue to explain the new feature you want to add. If it looks good, you'll be ready to open that PR with a feature gate for your changes!

## Implementation strategy
Copy link
Member

Choose a reason for hiding this comment

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

I think a convention for feature flags naming should be kept so that users can quickly discern which features are unstable, and we should have some log or central location to track these features.

Also unfortunately feature flag doc generation are currently bound to nightly. Not sure how to get around this before that feature gets stabilized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed on the convention.

I think it's good that the doc won't be built for those unstable features by default. If someone wants it, the doc can be built on demand with the feature gate enabled.

Copy link
Member

Choose a reason for hiding this comment

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

On one hand, I'm inclined to agree that we shouldn't be putting this forth front and center, but that also hides these features from those who might both benefit from them in the short term and be a good testbed for stabilization. By showing them and explicitly labeling APIs as "unstable, expect bugs", I think that should deter any user expecting stability. The same goes for nightly-bound unstable features in Rust: it garners both testers and active feedback.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a GitHub project showing tracking the stabilisation issues should be good enough for discovery, what do you think?
I added that proposition to the RFC. Keeping the project up to date could be done by GitHub Actions, but I think that's out of scope of this RFC.

Copy link
Member

Choose a reason for hiding this comment

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

I think a GitHub project showing tracking the stabilisation issues should be good enough for discovery

For comparison, Rust has the RFC book. A GitHub Project might be good enough as a start, but IMO if we want this to be a publicly recognizable format, we need more than that in the long run.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having something like the RFC book requires quite a bit more work...

It is needed in Rust where they need to keep track of everything back to the pre 1.0 time for people still using an old version, they have feature opened 7 years ago not yet closed, and they track a lot of them.
This is not what I want to Bevy, where I would rather have around 10 unstable features open at any given time, and for no more than a year.

I think filtering issues on the label, and presenting them in a project would be enough for some time

Copy link

Choose a reason for hiding this comment

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

I think a GitHub project showing tracking the stabilisation issues should be good enough for discovery, what do you think?

This is hard to discover for Bevy users which are not contributors.

Copy link
Member

Choose a reason for hiding this comment

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

One edge case I would like to see explored is changes where a feature is controversial / not-yet-ready, but it needs small engine tweaks to support it. Presumably, we would have PRs that submit a mix of feature-gated and ungated changes.

IMO this is better, because it keeps the feature flag localized, rather than creating branching throughout the entire code base.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the small engine tweaks are non controversial, they should be split in another PR, that's already what we try to promote

Copy link

Choose a reason for hiding this comment

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

The small engine tweaks cannot always be purely additive, I can imagine sometimes they're breaking changes.


The new workflow for this kind of feature would be:
- Submit an issue explaining the feature you want to add under a feature gate. This will be the tracking issue for stabilization once approved. This issue gets a new label `S-Stabilization`.
- If approved, submit a PR with the new feature and the feature gate
- This PR can be merged by someone with the merge rights on the related part of Bevy as soon as it's `S-Ready-For-Final-Review`, without an approval by the main Bevy maintainers.
Copy link
Member

Choose a reason for hiding this comment

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

We should further expand this. Currently there are three people with merge rights, and only one of them is unscoped. This, at best, seems to still have a heavy bottleneck here. We already have engine teams for specific focus areas. Would it be possible to set it up such that areas of the engine could have these unstable features merged with significant focus area engine team buy-in? The only other way is to expand the number of org members with scoped merge rights, and that seems to be the slowest area of growth for the organization as a whole.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that is out of scope of this RFC. I wrote it mentioning roles, who has those roles is not the subject. What you propose would still work in the context of this RFC, as it can be read as "giving the focus area teams the scope to merge approved unstable features PRs".

Copy link

Choose a reason for hiding this comment

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

Can we mark this as resolved now that all maintainers have merge rights?

- Subsequent PRs on the feature must be linked to the tracking issue and follow the same process.
- Once the feature is finished, enter the stabilization period. Open a PR that will remove the feature gate. This PR need to be approved by the main Bevy maintainers.

An existing PR can be retroffited in this process on the suggestion of Bevy community members, and if the PR author agrees.

The RFCs and the unstable features are closely related. An RFC can be implemented as an unstable feature to help resolve issues that need more experimentation. An unstable feature could require an RFC to reach stabilization.

Each PR will still need to be approved by Bevy community members, and would still need to have the same level of quality as other PRs merged.
Copy link

Choose a reason for hiding this comment

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

Review this inline with more recent rules about maintainers and reviewers? The rules is 2 community reviews I think.


The PRs for the initial feature, various changes during its finalization and for the stabilization can be opened by different persons.

The feature gate should not be exposed on the main `bevy` crate, but only on the subcrates where it is relevant. It can be enabled as a user by adding dependencies directly on the subcrates and enabling the feature.
Copy link
Member

Choose a reason for hiding this comment

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

How would we tackle breaking new ground with new official crates then? Should these be added as unstable features to the main bevy crate?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, a new crate can be added directly as an optional under feature gate dependency to the existing crate that need it, or directly as an added dependency. It shouldn't be any different than an existing subcrate

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if that's a good model to follow since many new crates will be building on top of existing foundational crates, not as dependencies for them. Or is the intention to have it added to bevy_internal in those cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think having as dependencies:

bevy = "0.6"
bevy_ecs = { version = "0.6", features = [ "unstable-4242-my-great-feature" ] }
bevy_new_crate = { version = "0.6" }

or targeting main

bevy = { git = "https://github.com/bevyengine/bevy" }
bevy_ecs = { git = "https://github.com/bevyengine/bevy", features = [ "unstable-4242-my-great-feature" ] }
bevy_new_crate = { git = "https://github.com/bevyengine/bevy" }

should work?

The other option would be indeed to raise all the unstable feature gates to bevy_internal, but I'm not sure that's worth the (very small) cost


Copy link
Member

Choose a reason for hiding this comment

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

Not something to add to this RFC, but it would be useful to come up with some "current" PRs that would benefit from being unstable features.

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR that motivated me to write that RFC is bevyengine/bevy#3884

As you said yourself

But this feels like a "let people yell at us if we did something wrong" sort of situation.

This RFC generalises that possibility: to have an advanced feature that we want to test, and let the interested people manage until it's ready for common use

## Drawbacks

This will increase complexity in Bevy code.

It will also increase complexity of a contribution that would like to use this process, but it is opt in and not mandatory. Think of it as a fast track for unstable features.

## Rationale and alternatives

- Improve the process to get new features in Bevy that would benefir from iterative work.
- Alternatives:
- Continue as it is now, with semi large PRs that bitrot and RFCs that don't often move forward
- Work harder to implement a new feature as a third party crate (this is not always possible)
- Fork Bevy to merge unnaproved PRs and check how they work / interact


## Prior art

[The Rust process](https://rustc-dev-guide.rust-lang.org/implementing_new_features.html)

## Unresolved questions

- Do we want to limit the number of feature gates in Bevy to reduce complexity?
- this could help control the increased complexity in Bevy
- Do we want to limit the maximum age of a unstabilized feature?
Copy link
Member

Choose a reason for hiding this comment

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

With the Bevy train release schedule, it'd be nice to have a minimum time for this too, to allow for folks using the released versions of Bevy to have to test out and file bugs. This could be mitigated by telling users to use the main branch, but that might not always be possible with the growing ecosystem and dependency compatibility.

As an upper bound, if a unstable feature doesn't get stabilized within two train releases, even with external users testing it out, I'd say we either need to reconsider the design of it as a full RFC or just scrap the idea entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

With Rust as an example where stabilising a feature can take a few years, I think a good scope would be at least one version, at most 4 (so between 3 months and a year)

Copy link
Member

Choose a reason for hiding this comment

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

I like both the upper and lower bounds here. The upper bound in particular results in a nice forcing function to actually ship things or cut our losses.

- this could help control the increased complexity in Bevy, and requires being ready to cut a feature off if it's not getting the use / stabilization expected