Skip to content
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

Make SDX Local Controller a WSGI app #149

Merged
merged 27 commits into from
Jun 19, 2024
Merged

Make SDX Local Controller a WSGI app #149

merged 27 commits into from
Jun 19, 2024

Conversation

sajith
Copy link
Member

@sajith sajith commented Jun 17, 2024

Resolves #86.

@coveralls
Copy link

coveralls commented Jun 17, 2024

Pull Request Test Coverage Report for Build 9556400895

Details

  • 0 of 10 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 77.103%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_lc/app.py 0 10 0.0%
Totals Coverage Status
Change from base Build 9519814897: -0.3%
Covered Lines: 978
Relevant Lines: 1215

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9558410474

Details

  • 0 of 11 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 77.273%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_lc/app.py 0 11 0.0%
Totals Coverage Status
Change from base Build 9519814897: -0.1%
Covered Lines: 978
Relevant Lines: 1212

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9558450035

Details

  • 0 of 11 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 77.273%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_lc/app.py 0 11 0.0%
Totals Coverage Status
Change from base Build 9519814897: -0.1%
Covered Lines: 978
Relevant Lines: 1212

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9568411630

Details

  • 0 of 18 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 77.729%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_lc/app.py 0 18 0.0%
Totals Coverage Status
Change from base Build 9519814897: 0.3%
Covered Lines: 978
Relevant Lines: 1204

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9568543362

Details

  • 0 of 19 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 77.671%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_lc/app.py 0 19 0.0%
Totals Coverage Status
Change from base Build 9519814897: 0.3%
Covered Lines: 978
Relevant Lines: 1205

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9568572969

Details

  • 0 of 19 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 77.671%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_lc/app.py 0 19 0.0%
Totals Coverage Status
Change from base Build 9519814897: 0.3%
Covered Lines: 978
Relevant Lines: 1205

💛 - Coveralls

All so that uvicorn will use and print the same port that we specified
with SDXLC_PORT when running `docker compose up`.
@coveralls
Copy link

coveralls commented Jun 18, 2024

Pull Request Test Coverage Report for Build 9570971840

Details

  • 0 of 19 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 77.671%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdx_lc/app.py 0 19 0.0%
Totals Coverage Status
Change from base Build 9519814897: 0.3%
Covered Lines: 978
Relevant Lines: 1205

💛 - Coveralls

@sajith sajith self-assigned this Jun 19, 2024
@sajith sajith marked this pull request as ready for review June 19, 2024 16:23
@sajith sajith requested a review from YufengXin June 19, 2024 16:23
Copy link
Collaborator

@YufengXin YufengXin left a comment

Choose a reason for hiding this comment

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

ASGI vs WSGI;

Flask vs. uvicorn vs. gunicorn

FastAPI vs. swagger UI

Please write a blog for educational purpose...

@sajith
Copy link
Member Author

sajith commented Jun 19, 2024

ASGI vs WSGI;

Flask vs. uvicorn vs. gunicorn

FastAPI vs. swagger UI

Please write a blog for educational purpose...

@YufengXin Is that a request meant for me?

🙈 🙉 🙊

@sajith sajith merged commit a8d56e9 into main Jun 19, 2024
9 checks passed
@sajith sajith deleted the 86.wsgi branch June 19, 2024 16:51
@YufengXin
Copy link
Collaborator

@sajith Yes-:) I assume you're moving to upgrade sdx-controller too?
The justification on the choice would be nice to be included in the annual report.

@sajith
Copy link
Member Author

sajith commented Jun 19, 2024

This is already done on sdx-controller.

For production usage, using Flask's built-in web server directly is not a good idea, since it is single threaded and synchronous. It is good enough during development, but it just not meant to be used in production. It should be used in conjunction with a WSGI server (Gunicorn, uWSGI), since it is more secure, scalable, and can work with reverse proxy web servers (such as nginx or caddy, which we will need for SSL termination).

ASGI is essentially the updated version of WSGI that understands async/await. We are not quite there, but I liked uvicorn, which is an ASGI server, enough to make our app sort-of-ASGI with WsgiToAsgi() adaptor. We can always switch to gunicorn or uWSGI or something else if we need to do so.

@YufengXin
Copy link
Collaborator

The first part is clear as we always do.

I don't know much about ASGI. So the key is the wsgi_to_asgi converter. as my naive understanding is that FASTAPI is to ASGI as Flask to WSGI. But somebody on Internet said Flask2.0 supports ASGI....

Nice upgrade. I'll read more into it.

@sajith
Copy link
Member Author

sajith commented Jun 19, 2024

FastAPI is an alternative to connexion and Flask, if I understood correctly. It probably would be nicer too, but it is also very likely to come with its own set of limitations.

Since the choice has already been made, and since we're somewhat familiar with Flask and connexion, and since there is a cost (time and opportunity) to switching, I am of the opinion that we should continue working with the current setup. Unless there is a good reason to switch to something else, of course. :-)

@sajith
Copy link
Member Author

sajith commented Jun 19, 2024

Also the WsgiToAsgi() call is really optional. We can simply remove that line and use gunicorn instead of uvicorn and forget that we ever tried to use an ASGI server.

@YufengXin
Copy link
Collaborator

Totally agree. Let's keep it this way, because you made it work well-:)

@sajith
Copy link
Member Author

sajith commented Jun 20, 2024

Thank you. There's more to be done, of course. :-)

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.

Use WSGI or ASGI
3 participants