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

Add FastAPI framework as next step for API framework #10823

Merged
merged 44 commits into from
Dec 22, 2020

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Nov 30, 2020

FastAPI does a lot of things (websockets, graphql, Oauth2 + JWT tokens, backend tasks, celery integration, sync + async routes, openapi + typing, good performance, file serving, active community and nice documentation) and can be integrated nicely route-by-route without having to toss out everything at once.

This is a quick experiment where I replaced /api/jobs, IFF you start galaxy with python scripts/fapi.py -c config/galaxy.yml (need to activate the virtualenv and install uvicorn). The rest of routes continues to work as normal, so Galaxy is fully functional.

Here's the openAPI doc for the new route:

Screenshot 2020-11-30 at 10 20 24

In terms of challenges:

  • I don't think uwsgi can serve ASGI apps
  • We'd have to replace the interactive tool routing
  • We'd have to get buy-in from admins to abandon uwsgi

TODO for this PR:

@hexylena
Copy link
Member

  1. that docs picture is gorgeous, it would make life so much better.
  2. No one who deploys ITs in prod uses that routing anyway. And we should really switch to path based proxying (i.e. GIE style) which would address the local routing issues, and then we could toss out this uwsgi routing completely.
  3. (as an admin) I've accrued a lot of knowledge and scripting/ansibling to handle uwsgi/deployment. It would be sad to throw out completely, but, sounds like some positive improvements for devs?

@mvdbeek
Copy link
Member Author

mvdbeek commented Nov 30, 2020

3. (as an admin) I've accrued a lot of knowledge and scripting/ansibling to handle uwsgi/deployment. It would be sad to throw out completely, but, sounds like some positive improvements for devs?

I'd also be curious about what you think of gunicorn as a process manager, which seems like it's the recommended route for production. https://docs.gunicorn.org/en/latest/signals.html#master-process lists some signals it can handle. Reading this naively it sounds like it can also do zero-downtime restarts, as a sort of replacement for zerglings ?

@hexylena
Copy link
Member

That potentially looks OK, that there's a master/worker split. I'm not as big of a fan of signals as Nate is, but it looks potentially nice?

scripts/galaxy_main.py Outdated Show resolved Hide resolved
@nsoranzo nsoranzo changed the title Add FastAPI framework as next step for API framwork Add FastAPI framework as next step for API framework Nov 30, 2020
@Nerdinacan
Copy link
Contributor

+1 for websocket support. I know there's a lot more stuff we'd have to do server side to actually generate a subscription channel with changes that we could consume, but I love that it would be possible.

@mvdbeek mvdbeek force-pushed the fast_api_quick_exp branch 2 times, most recently from 7fc087a to 62ed9b4 Compare December 1, 2020 13:27
@mvdbeek mvdbeek force-pushed the fast_api_quick_exp branch from 62ed9b4 to 8352d8a Compare December 1, 2020 19:44
@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 10, 2020

This is by no means complete, but I think it is mergeable. That would make distributing work on various parts easier (xref: https://github.com/galaxyproject/galaxy/projects/20)

@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 10, 2020

@nsoranzo do you know why make update-dependencies didn't respect pygithub pinning requests to <2.25 ?

@nsoranzo
Copy link
Member

@nsoranzo do you know why make update-dependencies didn't respect pygithub pinning requests to <2.25 ?

I don't have time to investigate in deep at the moment, you can try to regenerate the Docker image with: make -C lib/galaxy/dependencies/pipfiles/docker/ clean and re-run make update-dependencies

@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 10, 2020

Thanks, no luck there, but I think the manual pin shouldn't be a big deal for now.

@jmchilton
Copy link
Member

Can you integrate some of the idea from:

  • jmchilton/galaxy@0f9279f (add option to integrate testing using uvicorn)
  • jmchilton/galaxy@e094706 (clearer transition path for older code, less duplication, and pushing more logic out of controllers that are tied to particular frameworks)

I know it is a little more work to rework the old APIs as we migrate the new ones - but I think there is value. It pushes more logic out of the controller layer if nothing else, and then it puts us in a better position if we're not able to fully make the transition within one or two releases.

@jmchilton
Copy link
Member

I didn't mean to insist that all the tests pass under uvicorn, just wanted to have the option to run them so I could play with the branch.

@mvdbeek mvdbeek force-pushed the fast_api_quick_exp branch 3 times, most recently from 8547422 to 5529f6b Compare December 13, 2020 19:25
Bare routes (without trailing /) don't quite work, so we need to fully
specify them. Also fixes job lock route to be at `/api/job_lock` instead
of /api/jobs/job_lock.
@jmchilton
Copy link
Member

Brilliant work!

@jmchilton jmchilton merged commit 46a6893 into galaxyproject:dev Dec 22, 2020
@mvdbeek mvdbeek mentioned this pull request Jan 6, 2021
23 tasks
@jmchilton jmchilton added this to the 21.01 milestone Feb 16, 2021
@mvdbeek mvdbeek deleted the fast_api_quick_exp branch March 1, 2021 08:43
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.

6 participants