-
-
Notifications
You must be signed in to change notification settings - Fork 753
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 run_server(server) convenience func #1012
base: master
Are you sure you want to change the base?
Conversation
the run() function contains valuable logic for kicking of the StatReloader and the Multiprocess workers, however it does not give you access to the server instance before starting and subsequently blocking. thus splitting out the server start logic into a function accepting an already intialized server instance solves this problem.
@rmorshea Why would you want to have access to the |
In fact this comment suggests a relatively similar change. In that case the |
@Kludex @florimondmanca @euri10 I would personally recommend this PR over #1248... simple & elegant, plus it was written first! Just needs to resolve conflicts from master and add |
I'd be happy to make the requisite changes once the choice is made to go with this PR over the other. |
@Kludex IMO this PR is better than the other one which has a bunch of issues (hence the multitude of comments)... why not close that one instead? See, e.g., #1248 (comment) |
I'm not happy about having multiple ways to run uvicorn in code e.g. I'm also not that convinced about #1248 either fwiw. 😅 In any case, if you want, feel free to open a discussion, and propose the alternatives. I don't think a discussion presenting the possibilities, and why we should do this was created in a good way to convince others about this (or similar). |
@Kludex both PRs introduce (Food for thought/alternative: rather than adding yet another function, could just stuff the reload logic in |
Oh... True. I didn't see that, sorry. But after checking it again, the PRs are pretty similar anyway. |
@Kludex yep, quite similar, hence why I'd suggest re-opening this one and closing the other:
|
the
run()
function contains valuable logic for kicking off theStatReloader
and theMultiprocess
workers, however it does not give you access to the server instance before starting and subsequently blocking. thus splitting out the server start logic into a function accepting an already initialized server instance solves this problem.I'll add tests/docs for this once it's generally accepted as a good idea