fix: add max_job_memory_usage for worker #1136
Conversation
🦋 Changeset detectedLatest commit: 37e3ed1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -158,6 +158,11 @@ class WorkerOptions: | |||
|
|
|||
| Defaults to 0.75 on "production" mode, and is disabled in "development" mode. | |||
| """ | |||
| max_job_memory_usage: float = 0 | |||
There was a problem hiding this comment.
Let's put something like 300MB by default?
There was a problem hiding this comment.
I would prefer to leave it empty and let the users to decide.
This might be a breaking change if there is a default limit. I have tested that the minimal assistant agent the RAM usage is around 150M, but it could be higher if users load some other data or publishing/receiving videos. Those agents will be killed unexpectedly after upgrading.
There was a problem hiding this comment.
it makes sense to not break existing apps. Is it possible to have a "warn" mode where we initially let folks know about the high memory usage without killing the requests? Since most users don't pay attention to memory, making memory issues clearly visible would be beneficial.
maybe we should have two separate options?
job_memory_warnjob_memory_limit
There was a problem hiding this comment.
job_memory_warn_mb (defaults to 300) and job_memory_limit_mb are added.
| if ( | ||
| opts.max_job_memory_usage > 0 | ||
| and opts.job_executor_type != JobExecutorType.PROCESS | ||
| ): | ||
| logger.warning( | ||
| "max_job_memory_usage is only supported for process-based job executors, " | ||
| "ignoring max_job_memory_usage" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Make sure to remove this comment if we put a max memory usage by default (Windows is using JobExecutorType.PROCESS by default)
|
Lgtm just a few nits! |
Co-authored-by: Théo Monnom <[email protected]>
No description provided.