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

Support experimental/provisional APIs #1019

Closed
jamesls opened this issue Dec 14, 2018 · 14 comments
Closed

Support experimental/provisional APIs #1019

jamesls opened this issue Dec 14, 2018 · 14 comments

Comments

@jamesls
Copy link
Member

jamesls commented Dec 14, 2018

This is a follow up that originally came from a discussion on a PR.

Chalice is now GA (> 1.0) so we maintain backwards compatibility of our API. Specifically, chalice apps that are written against chalice 1.x will continue to work for all future versions of 1.x.

At the same time, Chalice has a lot of potential for new ideas and new APIs, many of which will take several iterations to get right. I suspect that in many cases we'll need to implement an initial idea, get customer feedback, try it out ourselves, and make corresponding adjustments. This may include anything from backwards incompatible changes to outright removal of a feature if it ends up not being a good idea.

There's been a few ideas on how to do this that I'll try to summarize here.

Ideas

  • Documentation only - Clearly document if an API is experimental vs. stable, but otherwise make no changes to the code itself. Features/PRs for experimental features get merged into master and treated like any other code. This is somewhat similar to how python handles provisional APIs (pep 411).

  • Namespace - Add an experimental subpackage in chalice, and add experimental features under that namespace. You'd then have to import explicitly form the experimental package. I'm not sure what exactly this would look like (haven't done this before), but maybe if we wanted to add a new decorator we might have from chalice import experimental; app = Chalice(__name__); experimental.inject_some_new_app_decorators(app). My concern with this is that when a feature graduates to a fully supported feature, that in itself would be a breaking change (we've move it out of experimental), and you'd have to modify your code.

  • Experimental branch - This option proposes maintaining an experimental branch on Github. A PR would then either target the master branch for immediate inclusion or the experimental branch. After some period of time testing, we'd merge experimental into master after removing any features we want to drop from experimental. The downside to this is that we (the maintainers) have a lot more repo maintenance and merging between branches could be problematic. It's also possible that not as many people would be willing to try out an experimental branch.

  • Feature flags - The idea is to gate each feature behind a feature flag. You'd need an explicit opt in that you're using an experimental feature. After some period of time we'd either drop the feature entirely or support by default (no feature flag needed). This reminds me of how rust nightly works (https://doc.rust-lang.org/book/appendix-07-nightly-rust.html). The downside is I'm not sure how to actually implement this when it comes to APIs in a client side framework. It seems like we'd have to have conditional checks throughout our code to only support this feature if a given feature flag is enabled (or would the specific method not even exist unless you somehow enabled a feature flag?) I'm worried that this could clutter our code. If anyone has examples of a client framework doing this let me know.

Let us know what you'd prefer, or if you have any other ideas!

@tedivm
Copy link

tedivm commented Dec 17, 2018

I think the first two options- Documentation Only and Namespace- are the best two options. Both the Experimental Branch and the Feature Flags methods will add additional work for both the development team and anyone who wants to contribute new features, with the Experimental Branch idea also making it difficult for people to use experimental features regularly.

Out of the Documentation Only and Namespace ideas there's really one major question- should there be some sort of explicit notification (in this case, the package name) that the code is experimental, or is documenting it enough? Personally I think documentation is enough, but I understand the other argument as well.

@stealthycoin
Copy link
Contributor

My thoughts on each, in order from favorite to least favorite.

  • Experimental Branch: +2 - I think this is the best option with 2 stipulations. I think it will get difficult to manage the diff between master and experimental as features accumulate and bufixes/changes are pushed to master. So this would have to get synced on a fixed schedule to keep this from getting out of hand. Features would either be merged in or thrown out in the cycle after they were introduced. Second, to manage expectations we would release the experimental branch on pypi as a separate package called chalice-experimental. This makes it actually impossible to know a feature isn't experimental since you need to change dependencies to use an experimental feature.
  • Feature Flags: +1 - I think this would probably be my favorite if we had a more concrete way to mark a feature as experimental. A feature may be spread out through the codebase so anywhere it was included you would need to mark off that section with the feature flag. Especially if we think about something simple like a new experimental decorator. It would need handler functions spread across all the deployer classes, the model class itself. A runtime component would also need to optionally be included if the feature needed it, optionally a local mode copy of that feature, possibly even a packager component . Thats 6-7 different separate pieces of code that would need to be protected behind a gate. I think its manageable but not optimal. I also wouldn't want the experimental features to affect the structure of Chalice itself.
  • Namespace: -1 - I'm not a big fan of the breaking change that would be forced when updating from the experimental version to the final version that gets moved out of the experimental namespace. One of the big issues I have with this specifically is we already have the codebase partitioned into a runtime and a buildtime set of classes. We would now be adding a further partitioning between buildtime/stable buildtime/experimental runtime/stable runtime/experimental which probably isn't worth the book-keeping.
  • Documentation: -2 - In my experience people just flat out do not read the documentation. Its python, they hack on things and just jam values into functions until it works and blindly copy paste things from stack overflow. I think if we go this route we will wind up fielding a lot of avoidable issues. The upside is there is no need to adjust the implementation between experimental/ga.

@JordonPhillips
Copy link
Member

  • Feature Flags: +2 - While there are some possible implementation hurdles from feature flags, I think this is the best customer experience. Even if it's just a single 'experimental' flag. This could also possibly drive plugin support as experimental features could just be plugins.
  • Experimental Branch: +1 - Much easier to manage from code perspective, but a worse customer experience unless we publish a separate package as John suggested.
  • Namespace: - 1 - I'm not a fan of potentially having two whole versions of the same code in one release.
  • Documentation -∞ - People don't read docs. They will depend on the feature. This also prohibits doing reworks on existing functionality unless you have a flag to enable / disable, at which point you may as well do feature flags at install time.

@tedivm
Copy link

tedivm commented Jan 2, 2019

@JordonPhillips - why would namespaces require two versions of the same code in one release? Would that be a solution to the problem @stealthycoin brought up regarding moving code out of the experimental namespace and into the core?

If so another solution that doesn't involve duplicating code is to create a proxy object in the experimental namespace that just called the final code (basically run some __init__.py magic for a version).

@JordonPhillips
Copy link
Member

The advantage of namespacing is hard isolation - you don't have to worry about mucking about with existing builtins. But if you do need to, then you still need to isolate everything. You could just internally handle it the same way you would handle feature flags, but at that point you may as well do feature flags because then it's not necessarily a breaking change when a feature moves out of experimental status.

@kyleknap
Copy link
Contributor

kyleknap commented Jan 3, 2019

I would like a hybrid approach of a single opt-in flag and documentation:

  • All experimental features would be marked as provisional in the documentation (i.e. the feature may go away or break in future versions of chalice)
  • We have a single opt-in flag for all experimental features so if a user is using any experimental feature, they would have to opt-in or else they would hit a deployment error/runtime error telling they need to opt-in to experimental features. In terms of how to opt-in, I am not exactly sure quite yet. I was thinking either opt-in by adding a single config variable to the .chalice/config file or at install time (i.e. pip install chalice[experimental])
  • After merging in an experimental feature, immediately open a GitHub issue to start tracking feedback for the experimental feature. We could even add links to the issue in the Chalice documentation. Once we and other users have used it enough to feel comfortable about it, we just remove all of the checks and documentations that mark the feature as experimental.

Here is why I like this idea:

  • We want to be minimizing the time it takes a feature to make it to the master branch and an official chalice release. I suspect most people are really only going to want to be using the chalice package deployed to PyPI whether they are unaware of an experimental package/branch or do not want to rely on a separate package/branch that is not the official one. Minimizing the time to master is important because in order to get feedback that we need to make a decision for a feature, we need to maximize the number of people trying it out, especially have it be used in larger applications. Having it be in the master branch and an official release helps maximize this.
  • By having it be an opt-in flag, we give one additional safety net to make sure they are aware of the experimental nature of the functionality they are using and accept that they are using experimental features that could break (or go away) in the future.
  • It is a single opt-in flag instead of multiple flags. I think it is a bit overkill to have multiple feature flags that users would have to opt-in to. Also, it would be more overhead on our part to manage all of the different feature flags. A single opt-in flag would be much more manageable.
  • No code changes will be required from the user's end once a feature is unmarked as experimental.
  • We are not managing any additional branches/packages (i.e. constantly having to rebase off master or cutting additional chalice releases for the experimental package)
  • Opening the GitHub issue (and linking it in documentation) will help make sure we follow up on the experimental feature and encourage feedback from others .

The one thing I am not sure about now is how we would go about doing the opt-in checks. I think for things that can be checked at deployment time it should not be difficult, but if the experimental feature is being used in the the runtime of a Lambda function that may be tricky.

@kadrach
Copy link
Member

kadrach commented Jan 3, 2019

It seems to be a tradeoff between ease-of-use for maintainers, and users.

Users disregarding documentation or copy-pasting from StackOverflow and thus relying on experimental features will happen whether feature flags, documentation, namespaces are used.

Perhaps a big "deploy-time" warning that experimental features are being used?

@jamesls
Copy link
Member Author

jamesls commented Jan 8, 2019

Thanks for the feedback so far, to summarize what I'm reading:

It seems like the preferred options right now are feature flags and the experimental branch (with the caveats that @stealthycoin proposed, which I like).

My main concern with feature flags is that I don't think it's a general solution, it seems like it would only work for specific types of APIs (for example we could probably do this for any new decorators we add). I'm not sure how this would work for more "invasive" API changes such as blueprints, websockets, and resources. I've also not seen this done before in a client side library/framework, only in web apps/apis, so I figure there's probably some reason no one's done this before.

I can try to put together a POC for feature flags and see what we think about it.

@jamesls
Copy link
Member Author

jamesls commented Jan 23, 2019

Ok, here's a simple POC I have for feature flags. There's three parts to it:

  • The app object has two two attributes, experimental_feature_flags and experimental_features_used. The former is set by users and the latter is set by chalice internals. Actual names TBD.
  • Whenever an experimental feature is used, the feature needs to update the set of experimental_features_used. This means that anything we want to guard with a feature flag must eventually have a reference to the app object.
  • We have a validation function that guarantees that the set of experimental_features_used matches the experimental_feature_flags. Right now this is run at deploy time, but we could also update this to run whenever any chalice CLI command imports the app (perhaps in load_chalice_app).

Pros:

  • Opt-in flags are only checked at deploy time, or whenver we load the chalice app via a chalice CLI command. There's no chance of a deployed app failing at runtime due to feature flags.
  • Simple, easy to implement and understand. Easy to add new feature flags as needed.

Cons:

  • Unnecessary code that runs in Lambda. It's only updating a python set() so it's more a philosophical issue than anything that would have realistic runtime overhead.
  • Any feature we add has to somehow get a reference to an app object. All the features we were thinking about adding that were experimental match this criteria but it's not clear if this will always be the case.

Example

from chalice import Chalice
from chalice import Blueprint

app = Chalice(app_name='test-features')
bp = Blueprint(__name__)


@bp.route('/')
def index():
    return {'hello': 'world'}


app.register_blueprint(bp)
$ chalice deploy
You are using experimental features without explicitly opting in.
Experimental features do not guarantee backwards compatibility and may be removed in the future.
If you'd still like to use these experimental features, you can opt in by adding this to your app.py file:

app.experimental_feature_flags.update([
    'BLUEPRINTS'
])


See https://doclink/ for more details.

To opt in:

from chalice import Chalice
from chalice import Blueprint

app = Chalice(app_name='test-features')
app.experimental_feature_flags.update([
    'BLUEPRINTS'
])

bp = Blueprint(__name__)


@bp.route('/')
def index():
    return {'hello': 'world'}


app.register_blueprint(bp)

Now a chalice deploy/package/etc will work.

@jamesls
Copy link
Member Author

jamesls commented Jan 23, 2019

I wanted to consolidate what @stealthycoin proposed
previously into a more concrete proposal based on what I've heard so far.
I think this is the best version of the "experimental branch" I've heard
so far:

  • Any bugfix or uncontroversial feature would continue to target the master
    branch.
  • Any experimental feature would target the experimental branch.
  • We would publish nightly builds of the experimental branch (assuming there
    were changes to experimental since the last release). This would use a
    different package name, e.g chalice-experimental.
  • We would continue to cut releases as needed from the master branch. This
    would include bugfixes and small features.
  • We would merge experimental with a fixed cadence, say every 1 or 2 months.
    Before merging we'd remove any features we've decided to drop.

I don't think this would be that big of a burden on us because I'd expect the
number of experimental features to be much smaller than the number of bug fixes
and small features.

The main downside I can see is that I'm not sure if we'd get as much feedback
from people compared to using the feature flags.

@tedivm
Copy link

tedivm commented Jan 23, 2019

  • We would merge experimental with a fixed cadence, say every 1 or 2 months.
    Before merging we'd remove any features we've decided to drop.

Lets say the end of every even month (February, April, June, etc) you decide to merge the experimental back into master. Would this mean that you'd just delay/refuse pull requests if it was already close to the merge time, and then mass merge after a release?

I do like your feature flags POC, I think that could work out well.

@kyleknap
Copy link
Contributor

@jamesls I like the feature flag design. I really like that to enable a feature you have to do so in your app.py file. I think this will really help improve awareness of the experimental nature of a feature. I also like that the experimental checks will only happen at deploy time as of now.

@jamesls
Copy link
Member Author

jamesls commented Jan 25, 2019

Cool, I haven't heard any feedback against using the feature flags so I'll go ahead and clean up the POC and send a PR. This will allow us to merge the blueprints PR.

jamesls added a commit to jamesls/chalice that referenced this issue Jan 28, 2019
jamesls added a commit to jamesls/chalice that referenced this issue Jan 28, 2019
jamesls added a commit that referenced this issue Jan 30, 2019
@jamesls
Copy link
Member Author

jamesls commented Jan 30, 2019

Implemented in #1053. Thanks everyone for the feedback.

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

No branches or pull requests

6 participants