Skip to content

Fastapi backend #65

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

Merged
merged 9 commits into from
Mar 26, 2021
Merged

Fastapi backend #65

merged 9 commits into from
Mar 26, 2021

Conversation

chr1st1ank
Copy link
Contributor

This PR adds a backend for Fastapi including tests and example.

In the first commits there are also fixes for

  • fixes a few smaller issues in the flask tests which only became apparent to me when I tried to add a second test suite.
  • correction of a few dead links in the README.md
  • some more tests for flask (request log was untested)

@bobbui
Copy link
Owner

bobbui commented Mar 22, 2021

wow, will take a deep look soon!

app = fastapi.FastAPI()

# init the logger as usual
logger = logging.getLogger(__name__)
Copy link
Owner

Choose a reason for hiding this comment

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

you're missing the logger.setLevel call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually not necessary. All what's needed is configured further down with a config dict.
I'm doing it that way, so that I can pass this configuration to uvicorn which configures logging itself. When I tried it locally in the script as for the other backends it didn't work out.

Copy link
Owner

Choose a reason for hiding this comment

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

a bit weird, when i run the sample directly from terminal the logging statement didnt emit anything, only works with setLevel call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, when you copy the code directly into an IPython shell, you mean? I also tested it and indeed that didn't work without logger.setLevel(). I only tested to run the script as dedicated python process (python fastapi_sample_app.py).

But right, with an additional logger.SetLevel() it works under all circumstances, so I just added that.

return False


if is_fastapi_present():
Copy link
Owner

Choose a reason for hiding this comment

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

this method is declared twice

Copy link
Contributor Author

@chr1st1ank chr1st1ank Mar 23, 2021

Choose a reason for hiding this comment

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

No, the second one is an if statement using the function. This pattern I adopted from other backends.
But your comment reminds me of this mechanism and that I did not stick to the lazy importing it was for. I have to adjust the way I do the imports. Otherwise json-logging can only be used if fastapi is installed.
Will take me the one or other day until I find the time.
Thanks for pointing me at it!

Copy link
Owner

Choose a reason for hiding this comment

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

its total fine, somehow i misunderstood it LOL

This runs every API example in a minimal environment where the dependencies for the other API frameworks are missing. This test guarantees that json-logging works for each individual framework.
(cherry picked from commit 4d9d596)
@chr1st1ank
Copy link
Contributor Author

@bobbui, as I wrote above your comment made me aware that the fastapi implementation always imported fastapi and its basis starlette. That's probably not what we want for non-web users or for users of different frameworks.
So I changed this by putting the implementation in a separate module which is only imported if fastapi is available.

In order to guarantee that such things are caught by the tests in the future I added another test suite which runs all the examples from the README file in a minimal environment for the respective backend. E.g. the flask backend is only run in an environment with "flask" and the test tooling in it. That's the reason why I had to make it a separte job in the github action workflow. A more elegant solution did not come to my mind.
Additional benefit of these tests is that they guarantee that the README examples at least not crash immediately.
Please take a look if you like this setup.

@bobbui
Copy link
Owner

bobbui commented Mar 25, 2021

@bobbui, as I wrote above your comment made me aware that the fastapi implementation always imported fastapi and its basis starlette. That's probably not what we want for non-web users or for users of different frameworks.
So I changed this by putting the implementation in a separate module which is only imported if fastapi is available.

In order to guarantee that such things are caught by the tests in the future I added another test suite which runs all the examples from the README file in a minimal environment for the respective backend. E.g. the flask backend is only run in an environment with "flask" and the test tooling in it. That's the reason why I had to make it a separte job in the github action workflow. A more elegant solution did not come to my mind.
Additional benefit of these tests is that they guarantee that the README examples at least not crash immediately.
Please take a look if you like this setup.

Its should be ok that way even though a bit undesirable. The integration test approach is sound, if we can somehow run each sample in a minimal isolated environment, e.g: Flask sample only run with Python virtualenv with only Flask installed. That would be really great.

Only then it works in the IPython shell
@chr1st1ank
Copy link
Contributor Author

Its should be ok that way even though a bit undesirable. The integration test approach is sound, if we can somehow run each sample in a minimal isolated environment, e.g: Flask sample only run with Python virtualenv with only Flask installed. That would be really great.

That's just what I implemented in .github/workflows/code_quality.yml. It installs these minimal environments for each of these smoketests.

Copy link
Owner

@bobbui bobbui left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for great contribution

@bobbui bobbui merged commit 7315cec into bobbui:master Mar 26, 2021
@bobbui
Copy link
Owner

bobbui commented Mar 26, 2021

released 1.3.0

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.

2 participants