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

Hey, I just met you, and this is crazy, but lets put routes in a module... Blueprints maybe? #651

Closed
wants to merge 5 commits into from

Conversation

ewdurbin
Copy link
Contributor

Use case

I have been working on a chalice based app lately that is growing a bit beyond having all routes set up in app.py.

Being heavily influenced by Flask's API, I figured Blueprints might be a welcome addition for Chalice.

Status of this PR

This is mostly intended as a provisional or "Request for Comment" PR. If the maintainers of Chalice like this approach, I'd be happy to continue work on adding tests and documentation for this :)

Currently only very basic functionality exists:

  • Construct a chalice.blueprint.Blueprint object
  • Add routes to your chalice.blueprint.Blueprint instance with the route decorator, which should accept all the same Keyword Arguments as a vanilla Chalice.route... though i'm not sure if they'll all work :)
  • Support for current_request access in Blueprint routes
  • Support for url_for in Blueprint routes.

Example!

/app.py

from chalice import Chalice

from chalicelib.blueprint import foo

app = Chalice(app_name="helloworld")
app.debug = True

@app.route("/")
def index():
    return {"hello": "world"}

app.register_blueprint(foo, url_prefix='/foo/')

/chalicelib/blueprint.py

from chalice.blueprint import Blueprint

foo = Blueprint('foo')

@foo.route('/bar', methods=["GET", "POST"])
def bar():
    request = foo.current_request
    return {"result": "bar!", "method": request.method}

@foo.route('/baz')
def baz():
    return {"result": foo.url_for('bar')}

Demo!

blueprintsmaybe ewdurbin$ chalice local > /dev/null 2>&1 &
[1] 10084
blueprintsmaybe ewdurbin$ curl -w "\n" http://localhost:8000/
{"hello": "world"}
blueprintsmaybe ewdurbin$ curl -w "\n" http://localhost:8000/foo/bar
{"result": "bar!", "method": "GET"}
blueprintsmaybe ewdurbin$ curl -w "\n" -XPOST http://localhost:8000/foo/bar
{"result": "bar!", "method": "POST"}
blueprintsmaybe ewdurbin$ curl -w "\n" http://localhost:8000/foo/baz
{"result": "/foo/bar"}

@jamesls
Copy link
Member

jamesls commented Jan 5, 2018

👍 I really like the idea of blueprints for chalice.

One thing that I think we should consider is how (or if) this is going to affect other decorators other than @app.route(): @app.lambda_function(), @app.schedule(), @app.authorizer(), etc. I don't think there's anything else that we'd need to add to the register() api, we'd just need to make sure those were also handled during the blueprint registration.

What do others think? cc @kyleknap @stealthycoin

@kyleknap
Copy link
Contributor

kyleknap commented Jan 5, 2018

Just giving my initial thoughts so far. Will do a little deeper review later...

I like the idea as well. In terms of the register_blueprint() API, I do not think there would be any additional parameters needed for now.

Not sure if it needs to be included in this initial implementation, but I do think adding support for the other decorators in the blueprint would be useful. Specifically, I think it could be useful for logically assigning lambda functions to a particular blueprint so that they are better organized and can potentially be reused. Furthermore, I think it will be even more powerful once we implement managed resources as I imagine you would be able to register resources to a blueprint as well.

@leason
Copy link

leason commented Jan 27, 2018

Just want to add an additional voice from someone using Chalice for production grade purposes. My app.py is over 300 lines now and I'd love to be able to split that up by object with something like Blueprints just to improve readability. Not a deal breaker by any means, but I like the PR.

@tedivm
Copy link

tedivm commented Nov 18, 2018

I'm wondering what the status of this is, as I would absolutely love this feature.

@jamesls
Copy link
Member

jamesls commented Dec 6, 2018

Hi everyone, just following up here. This is a fairly large (and useful!) change to chalice that I think will require some experimentation before we get a solid API. Once it lands in master we do have to preserve backwards compatibility. At the same time I don't want this to sit any longer in a PR until then.

Here's what I'd propose. I think we should create an experimental branch in this repo where we merge larger changes like this where we want them to bake for a while. It will be a similar in standards to the master branch (must pass tests, have full test coverage, etc), it's just that it may have features and APIs that will change as we get feedback and find issues with the API. After some time (and I'm not sure what the appropriate criteria is), the features that are solid get merged to master and subsequently released in an official chalice release. The experimental branch would have no backward compatibility guarantees. We could even put a disclaimer in the CLI somewhere when people use the experimental branch.

Let me know what you all think about this.

cc / @aws/aws-sdk-python-team

@kyleknap
Copy link
Contributor

kyleknap commented Dec 6, 2018

@jamesls I like the idea of the experimental branch and this would be a good feature to start it off. I think we would probably want to put managed resource and deployment hooks in this branch as well once we get around to implementing them.

@dstufft
Copy link

dstufft commented Dec 6, 2018

I kind of feel like an experimental branch makes things harder not easier. Work that lands in master will have to get regularly merged back into experimental, and the longer lived experimental is, the more likely it will be that there will be merge conflicts. Plus, there's no good way to take one feature out of experimental besides copy/pasting code out (which is error prone), you can only reasonably bring over the entire state of experimental or none of it.

I think a better way of implementing it is to use feature flags to enable experimental features. This allows people to opt into experimental features on a feature by feature basis. It reduces the drift problem (because master is still the only real long lived branch, but doesn't eliminate it because you can have differences in behavior between flag on/off). Promoting a feature from experimental to non-experimental is easy to do, you just flip the flag from default off to default on and/or remove the flag all together.

This gets a lot harder to deal with when you have whole new experimental functions rather than experimental data paths in existing functions, but that largely comes down to how the feature flags are implemented. At worst you can have a experimental namespace that stuff can live in, before it moves into the non experimental namespace.

@leason
Copy link

leason commented Dec 6, 2018

I've not seen too many open source projects manage new things with flags like that, @dstufft but I have done that plenty of times with customer facing projects.

Either way though, I'm up to 650 lines in my app.py file, so I'd be happy to help develop this and have a good real world production app to help test it against.

@tedivm
Copy link

tedivm commented Dec 6, 2018

I also think that experimental branches cause more trouble than they are worth and ultimately increase the time (and work) it takes to merge features.

I honestly think that simply declaring a portion of the code as unstable but including it in the main branch is the way to go. This is how the Github API gets updated- they release the new features but you opt into using them knowing that you'll need to put more work into maintaining compatibility until they are stable.

Even outside of that I think this is an important enough feature that it should be fast tracked. Once blueprints are added we get considerably easier testing and the ability to create third party extensions (which is currently very difficult because of the need to work off of the primary app object). Those features will seriously speed up third party development, which is where most experimental code should be happening anyways.

@jamesls
Copy link
Member

jamesls commented Dec 6, 2018

I like the idea of feature flags, but I've never seen it used in the context of a library. I've seen it in web apps and backend APIs, but how does it work with a python API?

So let's take this PR as an example. Are you saying that until you enable some feature flag (in the app config file?) the app.register_blueprint wouldn't be available on the Chalice object?

Or what about the Resources proposal? #516

This would add new modues such as from chalice.resources.dynamodb import Table. If you don't turn on the feature flag what should happen? Import error? Error on instantiation? At deploy time?

If that's true then it feels like you'd have the code littered with

if feature_enabled(some_flag):
    class NewAPI(object): ...

and then any consuming code that uses NewAPI would also have to check if the feature flag has been enabled.

It seems like you're trading off complex repo management via complicated cherry-picked merges vs. more complex code.

Are there any existing libraries that use feature flags that we could use as inspiration?

@tedivm
Copy link

tedivm commented Dec 6, 2018

I think the easiest way to handle this for things where the API isn't finished but people want to start using the code you could place things into an experimental package, such that app.register_blueprint becomes app.experimental.register_blueprint.

This method is nice because it requires the user to explicitly acknowledge the experimental status during import without requiring the writers of the code to do a bunch of "is feature enabled" checks.

A downside to this method is that it makes changes to the application class itself difficult to do in an experimental feature- using this case as an example the register_blueprint currently relies on having access to the app object (via self)- the function would either have to be refactored to take app as an argument or an initialization function for the experimental module (ie, app.experimental.blueprints.init_app(app)) would need to be created to add that new function in.

I want to stress though that blueprints will make this a lot easier in the future, as it will allow these types of features to be created in completely standalone modules until they're ready to be merged into core (if there even is a need for that).

@jamesls
Copy link
Member

jamesls commented Dec 7, 2018

Yeah so just to be clear, I'm working on getting this PR in a shape that we can merge, regardless of where we decide to put it.

That being said, the only downside to the app.experimental.foo approach is that if a feature goes from experimental to "official" then customers using the experimental feature would have to change their code (app.experimental.register_blueprint() to app.register_blueprint()). One benefit of the experimental branch is that it would just be a merge and would require no change to existing apps using the experimental feature.

@tedivm
Copy link

tedivm commented Dec 7, 2018

@jamesiri - yeah, it would result in a code change, but a fairly minimum one.

If there's real concern that people aren't testing code before pushing upgrades into production that can be solved by a simple wrapper at the experiments location that calls the real code while also throwing a warning saying the experimental package is going to be removed.

With the experimental branch you're either committing to keeping that branch in sync (lots of work for you) or forcing users to pick between new features in the master branch or the experimental features they need.

In my opinion it really comes down to who ends up having to do more work (you maintaining the experiments branches or users having to test their code), and in my experience the more work put on the maintainer the slower the features get pushed out to users.

@tedivm
Copy link

tedivm commented Dec 7, 2018

As a user of this code I'd rather get the features faster- knowing that I'll have to put in extra work if I'm using experimental features- than have a long period of development before the features are available.

@leason
Copy link

leason commented Dec 9, 2018

If we feel like this code can be done with app.experiemental.blueprints then I think we should go forward that way. Some PR's may not be able to be done like that, necessitating a branch, but if the main submitter here feels like this is doable with the simpler method then let's just move ahead that way. @ewdurbin any objection from you? I think we're all eager to start helping with it.

@jamesls
Copy link
Member

jamesls commented Dec 14, 2018

Just wanted to give an update:

I've created a separate tracking issue for what to do about experimental features (#1019) so we can keep this PR about blueprints.

I have an update that adds support for all the decorators. The only big deviation between what I have and what Flask can do is that you can register a blueprint multiple times in a single app. The way this is implemented now is you can only register a blueprint to an app once. This simplifies the implementation and will likely result in this getting merged quicker. Does anyone need the ability to register a single blueprint to an app multiple times?

@tedivm
Copy link

tedivm commented Dec 14, 2018

@jamesis - the biggest argument for being able to register a blueprint repeatedly to different apps is for testing purposes. Removing that ability makes testing a little more complicated, but ultimately isn't the biggest deal.

@ewdurbin
Copy link
Contributor Author

ewdurbin commented Jan 4, 2019

Glad to see that this has been picked up! I'm going to close in favor of #1023, thank you @jamesls!

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.

6 participants