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

Unstable Features for faster iterations #52

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

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Mar 5, 2022

rfcs/52-unstable_features.md Outdated Show resolved Hide resolved
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!


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


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.


- 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.

rfcs/52-unstable_features.md Show resolved Hide resolved
rfcs/52-unstable_features.md Show resolved Hide resolved
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?

rfcs/52-unstable_features.md Outdated Show resolved Hide resolved
rfcs/52-unstable_features.md Show resolved Hide resolved
rfcs/52-unstable_features.md Outdated Show resolved Hide resolved
rfcs/52-unstable_features.md Outdated Show resolved Hide resolved

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.

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.

rfcs/52-unstable_features.md Outdated Show resolved Hide resolved
rfcs/52-unstable_features.md Outdated Show resolved Hide resolved
rfcs/52-unstable_features.md Show resolved Hide resolved
rfcs/52-unstable_features.md Outdated Show resolved Hide resolved
rfcs/52-unstable_features.md Outdated Show resolved Hide resolved
@MiniaczQ
Copy link

MiniaczQ commented Mar 7, 2022

How do you imagine CI/CD working with unstable features on the main bevy?

@alice-i-cecile
Copy link
Member

How do you imagine CI/CD working with unstable features on the main bevy?

IMO one-experimental-feature at a time would be the strategy, for tests only. That'll increase load on our CI though, and may tip us over into "actually a priority to upgrade".

@mockersf
Copy link
Member Author

mockersf commented Mar 7, 2022

How do you imagine CI/CD working with unstable features on the main bevy?

With only GitHub runners, no CI/CD for unstable features. This is actually already an issue in Bevy as we don't test non default configurations.

If we can get more capacity, we could run more feature gate combinations, but I don't think that should be blocking for merging a PR.
If a PR break an unstable feature:

  • the PR author may fix it if they have the possibility
  • or it can be reported on the unstable feature tracking issue, then someone with more knowledge on that feature can fix it

mockersf and others added 6 commits March 7, 2022 13:08
Co-Authored-By: Alice Cecile <alice.i.cecile@gmail.com>
Co-Authored-By: Alice Cecile <alice.i.cecile@gmail.com>
Co-Authored-By: Alice Cecile <alice.i.cecile@gmail.com>

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. The tracking issue should list the subcrates impacted.
Copy link
Member

Choose a reason for hiding this comment

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

What is the rationale behind this choice? Piping unstable features up to the main crate would both be more convenient for consumers, and would help document the "scope" of the feature. It does add a bit more complexity, but it seems "warranted".

Copy link
Member Author

@mockersf mockersf Mar 17, 2022

Choose a reason for hiding this comment

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

I would prefer to not have different classes of features on the main crate. Currently we have a few to enable functionalities, feature gates for unstable features would be a different type.

Moving the feature up to bevy_internal would provide most of the benefit in usability while keeping them separate

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. The tracking issue should list the subcrates impacted.

For organization and discovery, a GitHub project can be used to track the stabilization issues.

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 we should discuss adding an "easily reversible" constraint for unstable features. I can see people wanting to make large, controversial architectural changes to code "outside" of the feature flag (aka in "stable bevy code"). If we decide an unstable feature is "undesirable", removing code inside feature flag blocks should be enough to undo all of the changes. Phrased another way: people shouldn't be able to entangle "unstable" code with "stable" code. All unstable code must be contained in "unstable feature flag blocks".

This constraint would make it harder to implement certain classes of features, but it would help maintain the integrity of the code base.

For organization and discovery, a GitHub project can be used to track the stabilization issues.

While it should be avoided, if another PR breaks an unstable feature, it is not the PR author responsability to fix it. They may do it if they want, otherwise it must be reported on the unstable feature tracking issue.

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

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 17, 2022

Some current PRs that would benefit from being unstable features:

  • animation
  • geometric primitives
  • viewports
  • raycasting
  • Command error handling
  • runtime system insertion
  • window icons
  • rumble support
  • system graphs

I've tried to select things that I think are:

  • large features
  • things we probably want
  • complex APIs that should be experimented on, played with and iteratively developed

There are obviously other PRs that I think should just be merged outright (or merged after a bit of cleanup), but those are a distinct category.

@james7132
Copy link
Member

james7132 commented Mar 17, 2022

IMO anything that would introduce a new crate or engine capability (looking at animation here), should probably fit into the RFC category and go through a more rigorous design process, as much as I would like for animation to be developed and integrated faster.

Though that brings up question of ecosystem crates depending on unstable features. We should probably discourage ecosystem crates from developing stable features of their own on top of unstable bevy features.

@cart
Copy link
Member

cart commented Mar 17, 2022

I think it's worth calling out what our policy is on "competing implementations". Are we going to allow merging 3 different competing unstable skeletal animation systems? What if they conflict with each other? If we only allow one, how do we pick a "winner" without going through a "full" review process (including pulling in "lead maintainers")?

I also think its worth explicitly calling out what should be a "3rd party crate" and what should be a "first party unstable feature". I personally think we should still push experimental / un-prioritized features to 3rd party crates when possible. 3rd party crates still allows for collaboration without bloating bevy internals (or bloating the bevy dev process / getting blocked on maintainer approval). It also more cleanly accommodates competing implementations, and in somes ways is a simpler user-facing conceptual model than "unstable bevy features". I think the decision tree should be "can this be expressed as a third party crate? if yes ... do that. If not (because it needs to make internal changes), use an unstable feature. Skeletal animation would probably need to be an "unstable feature" because the PBR pipeline can't (yet) easily accommodate 3rd party changes to mesh bindings (and im not convinced it should). On the other hand, "geometric primitives" and "system graphs" can be (and have been) expressed as third party crates. Maybe to help incentivize and streamline the "experiment in 3rd party crates first" process, we could have a way to "boost" 3rd party crates that are currently being considered for inclusion in bevy?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 17, 2022

On the other hand, "geometric primitives" and "system graphs" can be (and have been) expressed as third party crates. Maybe to help incentivize and streamline the "experiment in 3rd party crates first" process, we could have a way to "boost" 3rd party crates that are currently being considered for inclusion in bevy?

Boosting would be valuable. It's often hard to attract contributions to third-party crates, because it's not clear whether the effort will be wasted.

I'm also wondering whether unstable features could be a good way to start the upstreaming process, without blocking on your full review attention. bevy_mod_picking and bevy_kira_audio are both nearly ready IMO (and are obvious "this needs to be core to the engine" functionality), and are the clear "winners" within the ecosystem.

Fundamentally, the upstreaming process is likely to be rather involved, and trying to get all of the details from both a usability and code quality perspective ironed out in the initial code review is likely to be less effective, slower and more frustrating than taking a "merge and refine" approach.

@cart
Copy link
Member

cart commented Mar 17, 2022

Another quick thought: as this is pitched as a way to resolve "scaling issues" and it does come with a number of tradeoffs (which are already called out), we should fully discuss alternatives first. I think @mockersf already called out the clearest alternative. Rather than creating a new class of "unstable" code, we could instead just horizontally scale merging "stable" code (ex: some type of "subject matter expert merge rights" approach). It would require a lot more care to maintain the integrity of the engine (especially from my perspective ... there have been a lot of things that many "active" community members have voted to merge that I wouldn't merge without tweaks). But I think its something we should be "growing into".

@james7132
Copy link
Member

james7132 commented Mar 17, 2022

Another quick thought: as this is pitched as a way to resolve "scaling issues" and it does come with a number of tradeoffs (which are already called out), we should fully discuss alternatives first. I think @mockersf already called out the clearest alternative. Rather than creating a new class of "unstable" code, we could instead just horizontally scale merging "stable" code (ex: some type of "subject matter expert merge rights" approach). It would require a lot more care to maintain the integrity of the engine (especially from my perspective ... there have been a lot of things that many "active" community members have voted to merge that I wouldn't merge without tweaks). But I think its something we should be "growing into".

Just wanted to call out in this thread that there has been a sizable discussion of these kinds of alternatives in bevyengine/bevy#4131

@alice-i-cecile
Copy link
Member

there have been a lot of things that many "active" community members have voted to merge that I wouldn't merge without tweaks

One of the perspectives (that I'm not sure how I personally feel about) is that we take a merge-first, tweak-later approach, especially with more folks with merge rights.

The majority of PRs we receive are clear improvements over the existing situation, and while they aren't always perfect in terms of code quality / docs / API design, collaboration is generally much easier once the code is merged in (and we'll have initial user feedback). PRs-to-PRs are a thing, but there's a lot of steps and they're not obvious: without assurance that "yes this will get merged soon" it's often hard to prioritize work done to review or improve others' PRs.


## Summary

Experimental new features are merged into main behind a feature gate before stabilization.
Copy link
Member

Choose a reason for hiding this comment

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

I would propose we call these experimental features something other than "unstable", since a "stable feature" implies that we won't change the API of that feature going forwards; we don't make that guarantee about basically anything in Bevy at this point.

Rust has this to say about their own "unstable/stable" feature process:

Code that works and runs on stable should not break.

Until [it has been stabilized], the feature is not set in stone: every part of the feature can be changed, or the feature might be completely rewritten or removed.

Even a "stable feature" under the text of this RFC, as with almost all features in Bevy right now, is subject to change and might be completely rewritten or removed.

Plenty of good alternatives: experimental, prototype, canary, proto, future, et c.

Copy link
Member

Choose a reason for hiding this comment

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

prototype is my favorite of those. Definitely like it better than unstable in the context of Bevy.

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 it too - especially given the parallel to bevy_prototype_<name> community crates which, from other discussion here, it sounds like will still be encouraged where possible.

Copy link
Member

@MrGVSV MrGVSV Mar 24, 2022

Choose a reason for hiding this comment

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

This is a valid point and I’d like to cast my vote on experimental. I think the others are a bit more vague/formal/rigid whereas experimental feels more approachable. To me it says, "this is an experiment 🧪 to gain feedback and toy around with implementations to see if we (as a community) want to pursue this feature or a feature like this."


Edit: I also just realized the pun with canary.... Nicely done 😂

- Implement the RFC for the unstable feature


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants