Skip to content

Conversation

@Kilo59
Copy link
Collaborator

@Kilo59 Kilo59 commented Mar 22, 2020

I thought I should open up my in-progress PR so that the maintainers can see what a transition to
#130 FastAPI would look like.

I didn't want to heavily redesign the application without input or discussion from the original creators.
But for example, some of the existing code could be refactored to take advantage of FastAPI's dependency injection system.

I've added 2 pipenv scripts, that can be used to run a local development server.
To run it just type.
pipenv run dev_app or pipenv run app

TODO:

  • fix timelines for GET /locations
  • refactor according to guidance from creators
  • work with csbs source
  • verify working /v1 endpoints
  • setup gunicorn configuration
  • Test Heroku deployment
  • add GET /sources
  • fix datetime format
  • add source param to all endpoints?
  • make timelines query param a boolean (in swagger doc)
  • replace flask v2 with FastAPI v2
  • FastAPI app tests
  • Make all properties on a location queryable

@Kilo59
Copy link
Collaborator Author

Kilo59 commented Mar 22, 2020

Swagger/OpenAPI docs

image

ReDocs

image

@ExpDev07
Copy link
Owner

Looks good. I don't like that everything is in main.py. Hopefully we can for example put models under /models, etc.

@ExpDev07 ExpDev07 linked an issue Mar 22, 2020 that may be closed by this pull request
@Kilo59
Copy link
Collaborator Author

Kilo59 commented Mar 22, 2020

Looks good. I don't like that everything is in main.py. Hopefully we can for example put models under /models, etc.

Yes absolutely, I planned on refactoring and moving those into a models.py, just thought I'd talk to you first about exactly how you'd like that done.

@ExpDev07
Copy link
Owner

ExpDev07 commented Mar 22, 2020

just thought I'd talk to you first about exactly how you'd like that done.

That's considerate of you, thank you. I really like dividing it up as much as possible, so you have the /models directory, then /models/location.py, /models/timeline.py, etc, each containing max 1-2 classes each. You can really just look at how much I've divided it with the current version running :).

It's really just some simple refactoring that can be done at the end though.

@Kilo59 Kilo59 force-pushed the FastAPI-conversion branch from 988a375 to 9ae7ff5 Compare March 22, 2020 22:03
app/main.py Outdated


# mount the existing Flask app to /v2
APP.mount("/v2", WSGIMiddleware(create_app()))
Copy link
Collaborator Author

@Kilo59 Kilo59 Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ExpDev07
I'm not sure how this behaves with the /v1 endpoints. But (at least locally), the /v2 endpoints appear to be functional, including the redirect to the github page when hitting /v2.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So FastAPI works with flask or? The v1 endpoints can be found at:

  1. https://coronavirus-tracker-api.herokuapp.com/all
  2. https://coronavirus-tracker-api.herokuapp.com/confirmed
  3. https://coronavirus-tracker-api.herokuapp.com/deaths
  4. https://coronavirus-tracker-api.herokuapp.com/recovered

I have checked the logs, and they are still in use by a lot of apps, so they need to work.

Copy link
Collaborator Author

@Kilo59 Kilo59 Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ExpDev07
It's mashing v1 and v2 together 😦
So because /v1 and /v2 are defined into the same app, this appears to put both /v2 and /v1 behind the /v2 prefix.

In order to separate them, I believe I need to create 2 separate flask apps and mount them separately.
This should be relatively simple.

For example /confirmed is part of /v1, but I can access it locally at /v2/confirmed when it should be /v1/confirmed.

Copy link
Collaborator Author

@Kilo59 Kilo59 Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably need to do something like...

APP.mount("/v1", WSGIMiddleware(create_app("v1")))
APP.mount("/v2", WSGIMiddleware(create_app("v2")))

Copy link
Owner

@ExpDev07 ExpDev07 Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be /confirmed without any prefix :). Hmm, two different flask apps? Not really the biggest fan of that. FastAPI doesn't support blueprinting?

Copy link
Owner

@ExpDev07 ExpDev07 Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if the old v1 endpoints work then, then I guess it's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to get on a screen share, discord etc. to talk about this stuff (and deployment configuration, etc.).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My Discord # is: Marius Big Ounce#2997.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just sent you an invite.
Kilo59#8819

Copy link
Owner

@ExpDev07 ExpDev07 Mar 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I accidentally declined it (lol), sent you one now.

@Kilo59
Copy link
Collaborator Author

Kilo59 commented Mar 22, 2020

@ExpDev07
Here's a response comparison between the currently deployed application and this one.
The only difference is the format of last_updated.

Working on fixing that now.
https://fastapi.tiangolo.com/tutorial/extra-data-types/#other-data-types
https://pydantic-docs.helpmanual.io/usage/types/#datetime-types

GET /locations?country_code=IT


v2

{
  "latest": {
    "confirmed": 53578,
    "deaths": 4825,
    "recovered": 6072
  },
  "locations": Array[1][
    {
      "coordinates": {
        "latitude": "43",
        "longitude": "12"
      },
      "country": "Italy",
      "country_code": "IT",
      "id": 16,
      "last_updated": "2020-03-22T22:02:46.078243Z",
      "latest": {
        "confirmed": 53578,
        "deaths": 4825,
        "recovered": 6072
      },
      "province": ""
    }
  ]
}

v2-1

{
  "latest": {
    "confirmed": 53578,
    "deaths": 4825,
    "recovered": 6072
  },
  "locations": Array[1][
    {
      "coordinates": {
        "latitude": "43",
        "longitude": "12"
      },
      "country": "Italy",
      "country_code": "IT",
      "id": 16,
      "last_updated": "2020-03-22T22:12:42.421409+00:00",
      "latest": {
        "confirmed": 53578,
        "deaths": 4825,
        "recovered": 6072
      },
      "province": ""
    }
  ]
}

@ExpDev07
Copy link
Owner

Awesome, how about the v1 endpoints?

@Kilo59 Kilo59 force-pushed the FastAPI-conversion branch from 4561003 to dc943b1 Compare March 24, 2020 22:58
@Kilo59 Kilo59 force-pushed the FastAPI-conversion branch from dc943b1 to 129a153 Compare March 24, 2020 22:59
@Kilo59 Kilo59 requested a review from ExpDev07 March 24, 2020 23:09
Copy link
Owner

@ExpDev07 ExpDev07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

flask = "*"
python-dotenv = "*"
requests = "*"
gunicorn = "*"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed now that we're using uvicorn.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we are running uvicorn workers via gunicorn.
https://www.uvicorn.org/deployment/

Copy link
Collaborator Author

@Kilo59 Kilo59 Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems to imply that it's okay to run directly it with uvicorn.
It might be worth doing a performance test on both deployment methods.
https://fastapi.tiangolo.com/deployment/#alternatively-deploy-fastapi-without-docker

Copy link
Owner

@ExpDev07 ExpDev07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an issue where when using alternative data-sources for locations, their fields won't show up. For example, /v2/locations?source=csbs won't display the county field as defined in app/location/csbs.py - CSBSLocation.

@Kilo59 Kilo59 requested a review from ExpDev07 March 25, 2020 00:41
@Kilo59
Copy link
Collaborator Author

Kilo59 commented Mar 25, 2020

There's an issue where when using alternative data-sources for locations, their fields won't show up. For example, /v2/locations?source=csbs won't display the county field as defined in app/location/csbs.py - CSBSLocation.

Fixed it by adding the county field to the mode.
Also added some very minimal tests of the v2/location endpoint itself.
I certainly can add more.

@ExpDev07
Copy link
Owner

To start the app, now use

  • pipenv run dev for development (with live-reload), and
  • pipenv run start for starting without livereload.

Copy link
Owner

@ExpDev07 ExpDev07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work dude! Looks like it's time for merge.

@ExpDev07 ExpDev07 merged commit 1bc26b7 into ExpDev07:master Mar 25, 2020
@ExpDev07 ExpDev07 deleted the FastAPI-conversion branch March 25, 2020 03:55
@ExpDev07
Copy link
Owner

@all-contributors please add @Kilo59 for code, infrastructure, testing, and documentation.

@allcontributors
Copy link
Contributor

@ExpDev07

I've put up a pull request to add @Kilo59! 🎉

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.

Consider switching (from Flask) to ASGI framework

2 participants