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

Reduces the memory requirement of a standalone Prefect server again #16651

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

chrisguidry
Copy link
Collaborator

@chrisguidry chrisguidry commented Jan 8, 2025

In #16644, we were able to get the standalone Prefect server we run in
prefect server start from 400MB to 343MB. By running uvicorn
directly in the parent process, we further reduce that to 266MB. I
measured this using the same process as before, using a cgroup with
no swap to constrain the memory until I found the minimum.

There are two main changes here:

  1. Making prefect server start a synchronous command in both the
    background and the foreground cases. This helps to simplify the
    handling of signals and removes the need for anyio or piping the
    streams of a child process manually.

  2. Splitting the paths between background and foreground servers. In
    the case of the background server, we run the subprocess as usual and
    then exit leaving it running. In the case of the foreground server,
    we run it with uvicorn.run as the final call, letting uvicorn
    handle signals. uvicorn already handles SIGINT and SIGTERM to
    cleanly shutdown, so we don't need to do anything additional here.

I'm removing the two tests that were testing what happens when the
signals are sent in escalating succession, as they aren't materially
different now. In both cases, uvicorn will attempt to shutdown
cleanly. I also updated the tests to reflect that SIGINT will lead to
a successful exit, while SIGTERM will lead to a -15 exit (pretty
standard).

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

In #16644, we were able to get the standalone Prefect server we run in
`prefect server start` from 400MB to 343MB.  By running `uvicorn`
directly in the parent process, we further reduce that to *266MB*.  I
measured this using the same process as before, using a `cgroup` with
no swap to constrain the memory until I found the minimum.

There are two main changes here:

1. Making `prefect server start` a synchronous command in both the
   background and the foreground cases.  This helps to simplify the
   handling of signals and removes the need for `anyio` or piping the
   streams of a child process manually.

2. Splitting the paths between background and foreground servers.  In
   the case of the background server, we run the subprocess as usual and
   then exit leaving it running.  In the case of the foreground server,
   we run it with `uvicorn.run` as the final call, letting `uvicorn`
   handle signals.  `uvicorn` already handles `SIGINT` and `SIGTERM` to
   cleanly shutdown, so we don't need to do anything additional here.

I'm removing the two tests that were testing what happens when the
signals are sent in escalating succession, as they aren't materially
different now.  In both cases, `uvicorn` will attempt to shutdown
cleanly.  I also updated the tests to reflect that `SIGINT` will lead to
a successful exit, while `SIGTERM` will lead to a -15 exit (pretty
standard).
Copy link

codspeed-hq bot commented Jan 8, 2025

CodSpeed Performance Report

Merging #16651 will not alter performance

Comparing run-standalone-server-in-process (dabb1ea) with main (0fc38cf)

Summary

✅ 3 untouched benchmarks

from prefect.server.api.server import create_app

with temporary_settings(
{getattr(prefect.settings, k): v for k, v in server_settings.items()}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this cheeseball? Should I just explicitly set the Prefect settings here? I couldn't decide...

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable to me, considering the current state of settings. We need to update temporary_settings to be more ergonomic now we're using pydantic settings, but this is the right way to do it now.

server_env["PREFECT_API_SERVICES_SCHEDULER_ENABLED"] = str(scheduler)
server_env["PREFECT_SERVER_ANALYTICS_ENABLED"] = str(analytics)
server_env["PREFECT_API_SERVICES_LATE_RUNS_ENABLED"] = str(late_runs)
server_env["PREFECT_API_SERVICES_UI"] = str(ui)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While doing this, I discovered that PREFECT_API_SERVICES_UI isn't a thing, so I removed it. Not sure if was supposed to be another setting or just an old variable?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, PREFECT_UI_ENABLED is the correct one, which also there, so this is good to remove.

Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

Awesome!

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

rockstar!

@chrisguidry chrisguidry merged commit fbffa5c into main Jan 8, 2025
50 checks passed
@chrisguidry chrisguidry deleted the run-standalone-server-in-process branch January 8, 2025 20:40
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.

3 participants