DM-43967: Add a realistic demo app with Astroplan domain #3
Conversation
jonathansick
force-pushed
the
tickets/DM-43967
branch
3 times, most recently
from
11b89bf to
cb95484
Compare
jonathansick
force-pushed
the
tickets/DM-43967
branch
from
cdcedd8 to
fbbc1db
Compare
There was a problem hiding this comment.
This also looks great. I love having a real astronomy example to show people! This was a ton of work; thank you so much. I only had a few minor comments and questions, none blocking.
It's pretty minor, but it might be worth importing the changes to the template to use requirements/tox.in so that people at the bootcamp will get the same layout as this app when they create a new one.
| def get_request_username(self) -> str | None: | ||
| """Get the username who made the request. | ||
|
|
||
| Uses the X-Auth-Request-Username header passed by Gafaelfawr. | ||
| """ | ||
| return self.request.headers.get("X-Auth-Request-User") |
There was a problem hiding this comment.
I usually use auth_dependency provided by Safir instead, since it fails if there is no user. Maybe the problem there is that this makes it annoying to use the app locally where there's no Gafaelfawr? (It also requires passing in a user when running tests, but that's pretty easy to handle in a fixture.)
There was a problem hiding this comment.
I must have copied this from somewhere. The auth_dependency would be a good idea for a real app. I do worry it'd make it harder to demo the API interactively locally if it fails if there are no Gafaelfawr headers.
| # Make any modifications to the response headers or status code. | ||
| context.response.headers["Location"] = response_data.self_url |
There was a problem hiding this comment.
I thought Location was only defined for 201 and 3xx responses, but this seems to be returning 200. I think I'm missing something? Maybe it's just an example of something that you could do if you were returning a redirect?
There was a problem hiding this comment.
I was trying to include a clever example of setting headers and other things on the response but I'll find a better place for that. Good point!
| local_timezone: str = Field( | ||
| ..., | ||
| description="Local timezone of the observation site.", | ||
| examples=["Chile/Continental"], |
There was a problem hiding this comment.
Very minor nit: Chile/Continental is a deprecated time zone identifier and not available on many systems (such as the laptop where I'm writing this). It predates the switch to Olson time zone identifiers, which are always of the form Continent/City where the city is the most populous city in the time zone. The current corresponding time zone identifier is America/Santiago.
There was a problem hiding this comment.
Interesting. I'm going by the Astropy data here. I'll see if I can update it and the examples as well.
| current IERS data. Requests that trigger an IERS download will take longer | ||
| and will block the app since Astropy downloads IERS data synchronously. |
There was a problem hiding this comment.
Technically, another approach would be to write all the handlers and underlying methods that might trigger an IERS download as non-async functions and methods, which will tell FastAPI to run them in a thread pool and avoid blocking the app. I'm not sure if you want to get into that in this comment, though.
We'll use astroplan as the domain in this app example. https://astroplan.readthedocs.io/en/stable/
This sets up a more realistic application within the bootcamp app that's built around Astroplan and its Observer as the domain model. This implements the full set of architectural layers we use in our real API apps: - Storage layer - Service layer - HTTP path operations and API models - A Factory pattern for creating services and storage clients - The RequestContext pattern for creating a factory and providing other dependencies to endpoints. - Error handling for user request arguments Note on vendoring the data for the sites store: Because the Astropy astropy.coordinates.EarthLocation includes duplicates of sites under aliases, we're including the original site.json file in the package itself so we can use our own constructor for the observer object. Potentially we could to an httpx request to the Astropy data server at http://www.astropy.org/astropy-data/coordinates/sites.json but vendoring makes things easier for this demo.
This demonstrates an endpoint for listing all resources, including querying/filtering with query parameters.
This endpoint exposes a bit more complexity. It shows how the "work" of the application resides in the domain layer, and how the service and HTTP layers can cleanly use that functionality.
This storage class provides control over the Astropy IERS data cache so that we can pre-download the data on start up.
jonathansick
force-pushed
the
tickets/DM-43967
branch
from
12dcae3 to
cd4ddc6
Compare
Ideally we'd use the auth_dependency from Safir here instead, but that *requires* that the app run behind gafaelfawr, and can interfere with running the app locally since every request needs to send the X-Auth-Request-User header to avoid errors. Since we're not using this feature in this app, we'll drop it but perhaps mention this dependency elsewhere in the demo.
This allows us to pin the version of tox we use locally and in CI.
This sets up a more realistic application within the bootcamp app that's built around Astroplan and its Observer as the domain model (along with a JSON file of observatories as the data storage). This implements the full set of architectural layers we use in our real API apps:
The endpoints we implement are:
GET /fastapi-bootcamp/astroplan/observers(?name={name})to list observatoriesGET /fastapi-bootcamp/astroplan/observers/{id}to get a specific observatoryPOST /fastapi-bootcamp/astroplan/observers/{id}/observabilityto compute the observability of a target at a specific time.Merge after #2, which provides the initial commits.