Add research about migration to FastAPI #222
Conversation
|
@Venefilyn could you also have a look please? |
|
|
||
| ### Advantages: | ||
|
|
||
| - automatic documentation: OpenAPI, Swagger |
There was a problem hiding this comment.
Technically this is possible in Flask as well but requires the same amount of work really as everything needs to be migrated from View to Model
| - data validation, type hints: Python’s type annotations and [Pydantic](https://docs.pydantic.dev/latest/) for data validation | ||
| - built-in support for dependency injection, which is useful for managing database | ||
| sessions, authentication, and configuration | ||
| - websocket and background task support - can be helpful for real-time updates or long-running processes (for usages stats?) |
There was a problem hiding this comment.
I'm imagining this being useful for workflow details etc. possibly notifications too
There was a problem hiding this comment.
I agree that this sounds useful for collecting stats outside of our celery workers.
| - benefits: | ||
| - we could get rid of non-used endpoints | ||
| - we could refactor/improve the functionality | ||
| - documentation of endpoints => easier dashboard development |
There was a problem hiding this comment.
With intended ViewModel usage it also increases debugging capability - especially with getting queries and seeing its cost/bottleneck
| ### Rewriting endpoints | ||
|
|
||
| - convert Flask routes to FastAPI - replace Flask routes (`@app.route`) with FastAPI routes (`@app.get()`). | ||
| - update request parsing, query params |
There was a problem hiding this comment.
Most of the filtering and searching can be done without the boilerplate code like codebase currently has around this. For pagination there are good package dependencies from what I can see
research/fast-api/index.md
Outdated
| ### Implementing models | ||
|
|
||
| This is not strictly required (for the GET requests) but significantly improves the migration's value and is highly recommended. | ||
|
|
||
| Previous attempt to document the endpoints with Flask: | ||
|
|
||
| - https://github.com/packit/packit-service/pull/2089/files | ||
| - the biggest issues: lot of duplication and additional code, change in serialization |
There was a problem hiding this comment.
IMO this is the primary reason to migrate. I think migrating to FastAPI seems good overall but without this the benefit of all the migration work doesn't seem worth the investment. The models could also be implemented and fixed in Flask but the work would be very similar to the work required to migrate to FastAPI, hence why I think migration can tackle both the model issue and FastAPI improvements in one go
There was a problem hiding this comment.
agreed, I will mention this explicitly there
research/fast-api/index.md
Outdated
| - run Flask and FastAPI together | ||
| - pick a few endpoints (e.g. file by file) and migrate | ||
| - redirect traffic to FastAPI for migrated endpoints | ||
| - repeat until everything is in FastAPI | ||
| - remove Flask completely |
There was a problem hiding this comment.
IMO I think keeping the endpoints for Flask is better and then introducing a documented FastAPI under something like /api/v1/ to make the API documented. Then we wouldn't need to redirect traffic but simply mark it as deprecated until it breaks (or we remove it)
There was a problem hiding this comment.
I like the idea of a gradual rewrite and as @Venefilyn says we could also keep the old deprecated API for a while (but I would remove the code at some point and not wait for it to break 🙂)
| for build in KojiBuildTargetModel.get_range(first, last, scratch): | ||
| build_data = KojiBuildModel( | ||
| packit_id=build.id, | ||
| task_id=build.task_id, | ||
| scratch=build.scratch, | ||
| status=build.status, | ||
| build_submitted_time=optional_timestamp(build.build_submitted_time), | ||
| chroot=build.target, | ||
| web_url=build.web_url, | ||
| build_logs_urls=build.build_logs_urls, | ||
| pr_id=build.get_pr_id(), | ||
| branch_name=build.get_branch_name(), | ||
| release=build.get_release_tag(), | ||
| project=( | ||
| ProjectModel( | ||
| project_url=build.get_project().project_url, | ||
| repo_namespace=build.get_project().namespace, | ||
| repo_name=build.get_project().repo_name, | ||
| ) | ||
| if build.get_project() | ||
| else None | ||
| ), | ||
| ) |
There was a problem hiding this comment.
This would also be moved or migrated to expected FastAPI way of doing things to make the endpoint self-documenting. Otherwise getting this documented over time is difficult without manual declaration. I think it should be possible in some sort of function wrapper to ensure the document is documented, or by using a pagination library together with this
There was a problem hiding this comment.
Am I wrong or are we missing the disadvantages that @dcermak suggested to us on arch meeting?
These are really just two things:
However, I cannot judge whether either of them are relevant to you |
|
@majamassarini no you are right, I haven't pushed my updates here yet. But as @dcermak wrote, while acknowledging those, I don't think they (the 2.point mainly) will be affecting our setup too much. |
lbarcziova
requested review from
Venefilyn and
majamassarini
and removed request for
Venefilyn
Fixes packit/packit-service#2699