Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[FRAME/Meta/PM] "experimental" FRAME features #14345

Closed
ggwpez opened this issue Jun 12, 2023 · 16 comments
Closed

[FRAME/Meta/PM] "experimental" FRAME features #14345

ggwpez opened this issue Jun 12, 2023 · 16 comments
Labels
J1-meta A specific issue for grouping tasks or bugs of a specific category. T1-runtime This PR/Issue is related to the topic “runtime”.

Comments

@ggwpez
Copy link
Member

ggwpez commented Jun 12, 2023

Analogous to the deprecation process it would be nice to have an on-ramp for new features in FRAME.
This could be used to introduce new feature in a non-semver way, so that we can start integrating them before stabilizing the design.
I think it could increase our development velocity since we can iterate quicker on features without having to worry too much of breaking downstream. Currently I often try to keep breaking changes to a minimum - even when a feature is not used outside of Parity yet…

Possible implementations

There were some internal discussions about how to achieve this. Some ideas:

  • A) Some attribute macro like frame_unstable(id) or similar and a matching allow_unstable(id) where ID is the identifier of the issue, similar to rust nightly features. This would be the more fine-grained and elegant solution - although not sure how achievable without something like allow(deprecated) is too coarse-grained, should take a path rust-lang/rust#62398
  • B) Rust compiler features where we have one feature that guards all unstable features. This would need to be propagated to all dependencies… we already have enough problems just from try-runtime and runtime-benchmarks though.
  • C) Simplest would be to have a frame_support::unstable module, that export all unstable types. Or in the future just frame::unstable. The unstable features should not be accessible through another path. We can do this in frame_support since the modules can be made pub(super), such that the unstable module can export them.

Inside of pallets we have more control and could opt for a better DevEx. For example with the upcoming Task API we could mark that as unstable and emit a warning that can be silenced through pallet(unstable) or something.

Process

Without making this too cooperate-style bureaucratic I think we should still have some kind of process. For example:

  1. Put new non-trivial traits and types, where future changes are expected, into the unstable module. For example we could use that for the VersionedRuntimeUpgrade or the Multi Block Migrations.
  2. Start integrating the feature to discover sharp edges and improvements.
  3. Set a deadline for further changes and move it out to the normal FRAME crates afterwards.
  4. Maybe announce the feature somehow.

cc @kianenigma @sam0x17 @juangirini

@ggwpez ggwpez added J1-meta A specific issue for grouping tasks or bugs of a specific category. S0-design Issue is in the design stage. labels Jun 12, 2023
@kianenigma
Copy link
Contributor

@xlc
Copy link
Contributor

xlc commented Jun 12, 2023

unstable doesn’t means insecure right? if that’s the case, why do we need warnings? you need to implicitly import from unstable module to use it and I think that’s enough

@sam0x17
Copy link
Contributor

sam0x17 commented Jun 13, 2023

I think it's more like "could change at any time"

Maybe the word "experimental" would be better

@bkchr
Copy link
Member

bkchr commented Jun 13, 2023

  • something like an unstable module seems like the most reasonable path forward for now.

How should this work for things that require changes to any Config trait?

IMO, as annoying as features can be, they are probably the best way forward.

@sam0x17
Copy link
Contributor

sam0x17 commented Jun 13, 2023

IMO, as annoying as features can be, they are probably the best way forward.

Yeah I agree that unless this is only occurring within pallets, in which case we have outer macro pattern powers and can make up a syntax, something like a feature is unfortunately our best bet.

If we're doing features, IMO an easy albeit backwards way to do this is to mark all the experimental things as deprecated, with a deprecation message saying they are experimental, but if the experimental feature is present, we don't emit the deprecation attribute.

So to opt in to using something experimental, you simply enable the experimental feature and now you won't get deprecation warnings on any of those things.

A simpler approach ofc is to just only emit those things if the experimental feature is present, but doing it this way might aid easier discovery of experimental features while they are still being worked on, which is something that I think we want

@xlc
Copy link
Contributor

xlc commented Jun 13, 2023

Please don't abuse deprecation message. Only mark deprecated features deprecated.

@juangirini
Copy link
Contributor

unstable doesn’t means insecure right? if that’s the case, why do we need warnings? you need to implicitly import from unstable module to use it and I think that’s enough

Independently of whether we go for the features or the modules solution, this is a very good point and it would simplify things. I guess it was meant read explicitly import, in this or in the features case the developer is already aware of the experimental feature.

Maybe the word "experimental" would be better

+1 on this one, it sounds less scary

@kianenigma
Copy link
Contributor

Having thought about this a bit, I am leaning toward backing using a feature flag for this.

@juangirini
Copy link
Contributor

Having thought about this a bit, I am leaning toward backing using a feature flag for this.

Yes, me too, I think it's the best way to go.

@sam0x17
Copy link
Contributor

sam0x17 commented Jun 16, 2023

And consensus seems to be we call it "experimental" instead of "unstable"

@sam0x17 sam0x17 changed the title [Meta/PM] Unstable FRAME features [Meta/PM] "experimental" FRAME features Jun 16, 2023
@juangirini juangirini moved this from Backlog to Shaping up in Runtime / FRAME Jun 16, 2023
@ggwpez
Copy link
Member Author

ggwpez commented Jun 16, 2023

So its settled then; we use an experimental feature that is never enabled per default.
We can even apply this to pallets and non-FRAME crates 🎉


We could use #14311 as an example to try it out.

@ggwpez ggwpez added T1-runtime This PR/Issue is related to the topic “runtime”. and removed S0-design Issue is in the design stage. labels Jun 16, 2023
@ggwpez ggwpez moved this from Shaping up to To Do in Runtime / FRAME Jun 22, 2023
@kianenigma kianenigma changed the title [Meta/PM] "experimental" FRAME features [FRAME/Meta/PM] "experimental" FRAME features Jun 23, 2023
@liamaharon
Copy link
Contributor

I believe we need to update the Substrate CI to run experimental tests? It seems that the test CI is configured outside of this repo, what's the way to go about tweaking that?

@bkchr
Copy link
Member

bkchr commented Jun 23, 2023

No, the CI is configured in here: https://github.com/paritytech/substrate/blob/master/scripts/ci/gitlab/pipeline/test.yml

There you will need to change/add a new job.

@kianenigma
Copy link
Contributor

@juangirini @ggwpez @paritytech/frame-coders any reason to not close this?

@juangirini
Copy link
Contributor

@ggwpez #14311 should have close this issue, right? or is there anything else that you want to do?

@github-project-automation github-project-automation bot moved this from Draft to Closed in Parity Roadmap Jul 23, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Runtime / FRAME Jul 23, 2023
@ggwpez
Copy link
Member Author

ggwpez commented Jul 23, 2023

Yea it should be good. There were some follow up discussions on how to separate feature-gates and modules, but we can reopen it then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J1-meta A specific issue for grouping tasks or bugs of a specific category. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Status: Done
Development

No branches or pull requests

7 participants