Using Flask-MAB with application factory pattern
justinmayer opened this issue · 32 comments
For a first release, the documentation for Flask-MAB is excellent. Well done! My only question is regarding how to use Flask-MAB with application factories. Specifically, it's not clear where one should put...
mab = BanditMiddleware(app,bandit_storage)
... and how one would access the above object from within the portion of the codebase that handles routes (i.e., where the bandits would also presumably be located).
I'm happy to submit a pull request to clarify this, but first I would need to understand how this would work. Would you be so kind as to shed some light on this?
Once again, excellent work on this project!
There's a lot wrong with the internals here, admittedly. I was very excited to get this pattern out there and so I stuff a lot of state on to the extension object, which is a big no-no. Let's work to fix this!
For now, have a look at my approach with this quick patch, which can be used with this latest master branch. This should suffice for now if you want to clone out of master 😃
Scratch that, this works as a general guideline but the reward and pull registration will still need to be broken off the extension and onto the app logic. Ok! Now that there's a blueprint I'll get this together 📝
Awesome. Many, many thanks for looking into this. Much appreciated! (^_^)
I think the approach that will work is to migrate all the stuff being registered to mab at present to a new proxy object on the app instance. This will support all the caveats for the factory pattern (in addition to being the proper way to do it in retrospect!)
All the middleware will be responsible for is registering the necessary post_ and pre_ request handlers and creating these proxy objects. We'll leave the composition logic in for now, which will be attached to something like app.mab.registerBandit etc. The decorators will work the same way.
I think this is striking a nice balance for now and brings the extension into compliance with this awesome pattern. I've created a separate issue, #6 for creating a better way of handling the composition.
I took a look at 0aa320e, and what I'm starting to realize is that perhaps my real question is how to access the MAB functionality from within Blueprints (example), where presumably routes are defined outside of the application context (and thus seemingly putting MAB out of direct reach). Any suggestions?
You're right: what I'm working on later tonight will obviate the need to touch MAB at all. You'll be able to place the required config on the object returned by current_app(), and the presence of the extension should resolve it further down the stack once the extension is attached by another factory. This is what I'm thinking presently, at least. That example on the flask docs specifically mentions this as a way to get an app reference from within blueprints. I'm not sure how to resolve the decorators this way though.
Perhaps the decorators could be restructured as independent functions, not methods on the main extension instance? I'd have to experiment with the way the before_request and after_request handlers get scheduled.
I think that might do the trick. How is the experimentation going so far? I'm excited to try testing MAB from within a blueprint when you have a chance to push the next iteration. :^)
Slowly 😉
This refactor is basically a big inversion of control. It's been awhile since I wrote the original handler code so it's taking a while to get back into the flow on it.
Understandable, of course. If you're ever in Santa Monica, drinks are on me. ;^)
This technique is proving indispensable for this refactor. Not sure how good the form I'm using is though. I'm attaching the reward and choose decorators to the app object after the fact. I'm pretty sure this will behave the same when the app in question is just a blueprint.
Tests are passing, at least on the core app tests.
Have a look! I think this new branch should do the trick, though it could admittedly use some more tests!
If you'd like, feel free to fork and write in some tests that better fit your use case 😄
Here's a diff to give details.
Hey Mark. Thanks again for all the work you put into this. I've tried to integrate the app_factory branch with an application that closely follows Matt's overholt pattern, and I can't quite seem how to figure out how to do this due to the blueprints. Is there any way you could finish what you started with tests/test_blueprints.py so I can see if I can replicate it?
Sorry! I was away on a company trip for the latter half of last week and didn't have a chance to write the final test for this. I'll wrap it up tonight (I'm in Stockholm at present, GMT +1) and have it on the same branch.
Once again I apologize for the delay on this. But Sweden has been pretty epic!
I think the latest version I have here will work. It's a big change (even from the one I previously pushed and you tried), but now the decorators can be imported directly and used on endpoints before the middleware is even attached to an application instance. Par example:
from flask.ext.mab import choose_arm, reward_endpt
from flask import Blueprint
bp = Blueprint(__name__)
@route("/")
@choose_arm("some_bandit")
def root():
return root.some_bandit
@route("/clicked")
@reward_endpt("some_bandit", 1.0)
def reward():
return "you clicked me!"
And then within the app:
from flask import Flask
from flask.ext.mab import BanditMiddleware
from flask.ext.mab.bandits import EpsilonGreedyBandit
app = Flask(__name__)
BanditMiddleware().init_app(app)
bandit = EpsilonGreedyBandit()
bandit.add_arm("one_outcome", "fun")
bandit.add_arm("other_outcome", "not as fun")
app.add_bandit("some_bandit", bandit)
This new system raises its own exception, MABConfigException when the app instance hasn't had an bandit of an appropriate name attached and a decorator calls for it (for the scenario where your blueprint decorators use some_bandit but it hasn't been defined by the app factory later down the stack.)
I have a lot of documentation updating to do if this works, it's essentially a whole new api, but it's a lot cleaner and now only the decorators are necessary.
I'm doing some final testing and then you can try it out @justinmayer
This has been pushed to #7 with a test demonstrating how it works (route bandit declaration before any app initialization as above). Feel free to pull and try it out. I have to fix the docs before I merge to master.
Many thanks for all your work on this, Mark. I've done some testing, and when I include the following in a blueprint...
def root():
return root.some_bandit
... I get:
AttributeError: 'function' object has no attribute 'some_bandit'
I'm sure I'm simply misunderstanding the intended usage. Could you possibly point me in the right direction?
@justinmayer Are you using the new decorators like the example above? The new ones are module level functions you can just import and use along with any route. Could you maybe gist your code so I can have a look?
I am indeed using the new decorators noted in the example above. I will push up some code as soon as I can.
Hi Mark. I did some additional testing but still ran into the same problem I described previously. This time, I did my testing directly on the Overholt boilerplate application, which I set up via:
mkvirtualenv overholt
git clone https://github.com/mattupstate/overholt ~/Projects/overholt
cd ~/Projects/overholt
pip install -r requirements.txt
I then installed Flask-MAB via:
pip install bunch # New Flask-MAB dependency?
pip install -e "git+https://github.com/DeaconDesperado/flask_mab@app_factory#egg=flask-mab"
I then added the following stanza to the create_app function within overholt/frontend/init.py:
# Flask-MAB
BanditMiddleware().init_app(app)
app.storage = JSONBanditStorage('./bandits.json')
submit_button = EpsilonGreedyBandit(0.2)
blue_btn = "btn-primary"
green_btn = "btn-success"
submit_button.add_arm("blue",blue_btn)
submit_button.add_arm("green",green_btn)
app.add_bandit("submit_button",submit_button)
Next up, I changed the dashboard routing to the following:
@route(bp, '/')
@choose_arm("submit_button")
def index():
"""Returns the dashboard interface."""
return render_template('dashboard.html', submit_button=index.submit_button)
@route(bp, '/clicked')
@reward_endpt("submit_button", 1.0)
def reward():
return "you clicked me!"
I then changed the button link to incorporate the submit_button variable inside the dashboard template:
<a href="/clicked" class="btn-add-store btn {{ submit_button }}">Add Store</a>
Finally, I then created a database, created an admin user, launched redis, launched the celery worker in a separate terminal session, and ran python wsgi.py to start the application.
When I visit http://localhost:5000/ and log in, Flask tries to load the dashboard but cannot due to the aforementioned error: AttributeError: 'function' object has no attribute 'submit_button'
Any suggestions?
Did my explanation above make sense, Mark? Would you prefer I put up the entire code somewhere?
Yes, explanation is great. Let me see if I can get it to reproduce.
Also, bunch is a new dependency. It made the refactor a little easier by maintaining attribute access for the extension namespace.
I suspect this should be a pretty easy (and hopefully final) fix, and then we can release a new version 😄
Will work on it as soon as I'm off the plane.
So this is weird!
Defining your routes with @route(bp, '/') does not allow the attribute to be set, but using @bp.route('/') does!
I'm not quite sure why this is, gonna have to dig deeper, but I have your code example working great here with the bandits using the attribute access instead for route.
I see now, there's a custom @route decorator that add functionality into the regular core flask one.
Let me investigate why mine is incompatible here.
So the function name in flask's internal url map is blueprintname.functioname, whereas after going through the overholt decorator, I'm seeing just functionname. The lookup I'm using to apply function properties for the bandit outcomes is looking up via the fully qualified name. Still investigating why this would happen...
Ok so once again I have a fix! I think this is going to surface another significant API improvement, as the bandit values will now be passed in as kwarg functions much like request values:
@route(bp,"/")
@choose_arm("submit_btn")
def root(submit_btn):
return "hello world"
This is the only significant change, now the bandit values are passed by-name as kwargs just like values you'd put in your route following the flask convention. Note that you'll also pass them into your render template with just the regular function scoped name now. I think this is a significant improvement over function properties, so it's good we had this problem!
This is tested with your exact overholt setup as describe above and should work for you 😄
Got a ton of documentation to change if this works for you!
I updated to the latest code, and it seems like a tremendous improvement. Nicely done!
The only issue I'm having at the moment is that I only ever see the first "arm" — i.e., the blue button. I am never presented with the second green button. Any idea why that might be? Or am I misunderstanding the intended behavior?
Disregard my previous comment — I do indeed see the other arm... Just much less frequently than I expected. Perhaps with further testing I'll get a better handle on the various bandit algorithms' respective settings and behavior.
In short, I think this is good to go. Bravo! If you'd like assistance with the documentation, just say the word.
@justinmayer perhaps play with the value for epsilon (while deleting cookies between requests of course)? The present winner is included in the "experiment" pull (it's uniform random) so you will sometimes see the same outcome very often.
You are quite right, Mark. After tweaking the epsilon value and running some additional tests, I now see both arms with similar frequency.
Is there anything I can do to help get this branch merged and released? No rush — just want to be of assistance if I can. (^_^)
Thanks! I'll have some bandwidth to work on the docs and finally merge this tonight.
There's only one last detail before release. The add_bandit function is still applied as a method of an application instance, which now that I look at the rest of the changed API might make more sense as an importable function that uses current_app to get the right instance.
Opinions? I think since almost all the rest of the API has been changed to this pattern it might be best to keep it consistent.