api: refactor as separate Flask Blueprint

[rjw57]

Increase modularity by moving API into the tawhiri.api.v1 module and re-expressing it as a Flask blueprint. This lets one specify the location of the API as a single URL prefix when creating the Flask app. It also allows single-API only applications as used, for example, in testapi.py.

This PR makes no change to the logic of api.py. It merely moves the file and allows central configuration of the URL prefix. See http://flask.pocoo.org/docs/0.10/blueprints/ for more discussion of the advantages of using Blueprints when considering application modularity.

The rationale for this modification is that it allows a) experimental or "v2" APIs to be cleanly added and b) for APIs to share common code/interfaces within the api module.

It is envisaged that some light re-factoring will lead to a module within api which can be cleanly mocked to unit-test the web request logic separately from the actual landing prediction code.

[rjw57]

Note: I'll propose said "slight re-factor" as a separate PR if this one meets with people's approval.

[coveralls]

Coverage decreased (-0.2%) when pulling 06e7d67ed29444d04fe915d1d29a8a27a5b4646c on rjw57:api-refactor into f8aca1b85dbfda1b1dbaa67773ee5ccb3fe59c4f on cuspaceflight:master.

[adamgreig]

Neat. I like the blueprint thing. Maybe app.py should be api/__init__.py?

[rjw57]

I was in two minds about that but, yes, after sleeping on it, I think that while there is only one API in the repo, having it be the tawhiri.api module makes sense.

[rjw57]

I'll push a fix which does that in a mo'

[rjw57]

Done. I also tidied up the name of the API blueprint to reflect the fact that it actually is a blueprint.

[danielrichman]

I think adamgreig was suggesting(?)

keep tawhiri.api.v1 move tawhiri.app to tawhiri.api.__init__

which is indeed what I would have suggested

[coveralls]

Coverage decreased (-0.2%) when pulling c0fb239c6f519b6665cd07cd7099c05e6d85b36b on rjw57:api-refactor into f8aca1b85dbfda1b1dbaa67773ee5ccb3fe59c4f on cuspaceflight:master.

[rjw57]

Oh, I see. I think I mentally auto-corrected a typo which didn't exist. One could do indeed do that but I think it's orthogonal to this PR which modularises only the api code. For example, should the manager also be moved? If so should the module be called something like "webapp" with apiv1.py, app.py and manager.py all as sub-modules? I've no objection to doing such a thing but if we're going to do that then either a) this PR should be merged to move the v1 API and then app/manager get moved in a separate PR or we abandon this one and I propose a more aggressive "move all the things!" PR.

Make sense?

[danielrichman]

In my head the webapp is the API; they are one and the same to me. That is, the tawhiri webapp will serve the API only; the UI is static files and would be served by nginx. Indeed, they're independent in the sense that the UI is a client of the API as much as any other.

I know we have a flask app that adds routes for the UI to facilitate easy testing.

I prefer tawhiri.api.v1; I see you've reverted it :-)

I defer to adamgreig.

[rjw57]

Right. I've gone back to tawhiri.api.v1. Do we think that api is the right name for this module? If we want manager.py and app.py in there as well, tawhiri.webapp seems more appropriate.

I too am happy to let @adamgreig make the call. I think I'm too close to the wood to check whether the trees are currently named in good taste or not :).

[danielrichman]

IDK it's just naming now and doesn't really matter, but fwiw I associate the word webapp with something that has models and views and generates server side HTML.

[rjw57]

IDK it's just naming now and doesn't really matter, but fwiw I associate the word webapp with something that has models and views and generates server side HTML.

FWIW, manager has (debugging) logic to serve the UI so that's certainly quite webapp-y. I agree that's it's a bit bike-shedding to witter too much about it so I'm happy to let @adamgreig make the call. Someone needs to and @adamgreig is someone :).

[coveralls]

Coverage decreased (-0.2%) when pulling 8a04adb41e23394254ef60948185efcebee27f8b on rjw57:api-refactor into f8aca1b85dbfda1b1dbaa67773ee5ccb3fe59c4f on cuspaceflight:master.

[adamgreig]

In my mind the Python module inside tawhiri/ provides the prediction core and its API, and in that scope manager.py exists to help develop the API and app.py exists to expose it to the test or production server. Having the debug server host the UI is a developer convenience. The API thus contains the web app. I agree that it's a 'web app', but I don't think that makes it greater than the API. The API is the important part, so that's what it gets named after.

In this view I think it makes sense for manager and app to exist inside tawhiri.api because they are part of the API and not otherwise related to Tawhiri.

I think you could go further and say both, being API utilities, could be in api/__init__.py itself, and further on that path and in the spirit of "flat is better than nested" you could just put them in api.py with the current v1 and plan to split it later if we make a v2 API (I don't think we have any actual plans to do this yet, right?). Since the manager and app would all be inside api.py already, turning it into api.v1 would be easy to do with no external effects (the wsgi server can still host tawhiri.api.app).

This feels like maybe the opposite direction to where we've gone with manager.py and this PR's app.py though so what do you two think about it?

[rjw57]

This feels like maybe the opposite direction to where we've gone with manager.py and this PR's app.py though so what do you two think about it?

It's probably an issue with Python's deep connection between file and module. I take your point about flat vs. nested. Perhaps having {app, manager, api}.py exist as separate files within tawhiri.api (prefixed with '_') but accessed via stuff explicitly import-ed in api/init.py?

I'll open a separate PR with that scheme (rather than mess up the history in this one further) so you can see if it looks fugly or sensible.

[adamgreig]

I was thinking of flat in terms of files as well as the module namespace, especially since (at the moment) there's not very much code (by lines) in manager.py and app.py, so they could all go into api.py. If we did put e.g. api/_app.py and set it up such that tawhiri.api.app is tawhiri.api._app.app then that could be OK too (certainly nicer than tawhiri.api.app.app!) but perhaps for the forseeable future we'd be OK with just api.py?

[rjw57]

I'm a fan of smaller files personally but I'm happy to accept that's just my own taste. Coming back to the start of this PR, If I were to keep a long-lived "experimental API" branch, where do I put the experimentalapi.py file and how do I add it cleanly to the webapp? If the answers are "in the top-level" and "register the blueprint in app.py", then that's a fine answer and we can leave things much as they already are.

[rjw57]

Withdrawing this PR since it seems like things are OK where they stand for the moment. Feel free to re-open if that's not the case.

[adamgreig]

Aah. I forgot about the experimental API file. Really that invalidates the argument for having a singlular api.py and makes a good case for api/ with api/v1.py and api/experimental.py (or whatever). I guess in that case put api/manager.py and api/app.py and import app in api/__init__.py. Could probably go on top of this PR quite neatly.

Really hinges on whether you think it's likely we'll want multiple API Blueprints concurrently (e.g. the experimental one) vs having an experimental branch where you muck with "the" API.

[rjw57]

I think it's best to leave as-is for the moment and come back to it if/when we actually have two "real" APIs. :)

On Mon Nov 17 2014 at 13:15:43 Adam Greig notifications@github.com wrote:

Aah. I forgot about the experimental API file. Really that invalidates the argument for having a singlular api.py and makes a good case for api/ with api/v1.py and api/experimental.py (or whatever). I guess in that case put api/manager.py and api/app.py and import app in api/init.py. Could probably go on top of this PR quite neatly.

Really hinges on whether you think it's likely we'll want multiple API Blueprints concurrently (e.g. the experimental one) vs having an experimental branch where you muck with "the" API.

— Reply to this email directly or view it on GitHub https://github.com/cuspaceflight/tawhiri/pull/94#issuecomment-63302655.