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

Refactor to use FastAPI #252

Merged
merged 31 commits into from
Oct 15, 2024
Merged

Refactor to use FastAPI #252

merged 31 commits into from
Oct 15, 2024

Conversation

petechd
Copy link
Contributor

@petechd petechd commented Sep 9, 2024

What is the context of this PR?

To improve validator performance we want to use a framework that supports concurrency. Mainly changes to Flask libraries specific code were made, they were replaced with FastAPI equivalents. I've reduced the number of modules and put the endpoints logic in the main file, since the codebase for the initial app is minimal in size. Some new logic added since FastAPI modules behave slightly different, mainly around data received in requests sent.

How to review

Run it with Runner locally, ideally by pulling validator docker images and using make commands like make run-validator etc. using docker images and relevant branch of Runner.

Checklist

  • eq-translations updated to support any new schema keys which need translation

@petechd petechd changed the title Test Fastapi Refactor to use FastAPI Sep 11, 2024
Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

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

I think the run_app.sh might need to be updated with the new command as well?

Copy link
Contributor

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

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

Works as expected through Runner.
This part of the Make Run Command does not work : poetry run ./scripts/run_app.sh.
Edit: Did not see liam already make a comment on this sorry!

Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

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

Great work! Tested locally

Dockerfile Outdated Show resolved Hide resolved
api.py Show resolved Hide resolved
api.py Show resolved Hide resolved
api.py Outdated Show resolved Hide resolved
api.py Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
api.py Show resolved Hide resolved
api.py Outdated Show resolved Hide resolved
api.py Outdated Show resolved Hide resolved
@petechd petechd merged commit 2bba332 into main Oct 15, 2024
3 checks passed
@petechd petechd deleted the fast-api-conversion branch October 15, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants