-
Notifications
You must be signed in to change notification settings - Fork 44
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
Rename autoscaler parameters and add .update_autoscaler method #2885
base: main
Are you sure you want to change the base?
Conversation
@@ -586,17 +592,14 @@ def function( | |||
# Or, pass (request, limit) to additionally specify a hard limit in MiB. | |||
memory: Optional[Union[int, tuple[int, int]]] = None, | |||
ephemeral_disk: Optional[int] = None, # Specify, in MiB, the ephemeral disk size for the Function. | |||
min_containers: Optional[int] = None, # Minimum number of containers to keep warm, even when Function is idle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a way it could be nice if these shared a common prefix (instead of suffix) for in-editor discoverability when typing, but containers_min
sounds so much worse that I don't know what's best...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah container_floor
/ container_ceiling
is one way you could spell that but it would be sort of bizarre.
Most users won't be typing though!
min_containers: Optional[int] = None, # Minimum number of containers to keep warm, even when Function is idle. | ||
max_containers: Optional[int] = None, # Limit on the number of containers that can be spun up for this Function | ||
buffer_containers: Optional[int] = None, # Number of additional idle containers to maintain under active load | ||
scaledown_window: Optional[int] = None, # Max amount of time a container can remain idle before scaling down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this name, but the might have already been a discussion on that elsewhere?
"scaledown" feels like something more global imo, something that affects the whole pool of containers, which is only indirectly/partially true in this case.
Related - what happens if you adjust the scaledown window using update_autoscaler
on a function with live containers - will the existing running containers have their scaledown_window updated or will they still respect the value from when they started up? (don't remember how this is implemented)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we discussed extensively, it's probably my least favorite of the names here, but at this point we've made all the backend changes and I think we should value consistent frontend / backend terminology in the absence of a clear alternative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the primary motivation here was to avoid confusion arising from the fact that we don't always honor the full window as a "timeout"; containers aren't guaranteed to stay up for x seconds after becoming idle, rather, they are guaranteed not to be idle for more than x seconds. It's right that the pool of containers is what's scaling up / scaling down, and individual containers can be affected by that at any point within their scaledown_window
.
scaledown_window: Optional[int] = None, | ||
buffer_containers: Optional[int] = None, | ||
) -> None: | ||
"""Override the current autoscaler behavior for this Cls instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we start calling a "Cls instance" a Service soon? So much easier to pronounce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? I still don't think I understand what the plan is for changes to the client object model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on this on Slack :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments/questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Describe your changes
Introduces the new autoscaler parameters and the new
.update_autoscaler
methods and updates all internal usage. Adds a deprecation warning on usage of the old names (currentlypending=True
so that we can update docs and integration tests).Changelog
App.function
andApp.cls
parameters that configure the behavior of Modal's autoscaler:concurrency_limit
is nowmax_containers
keep_warm
is nowmin_containers
container_idle_timeout
is nowscaledown_window
buffer_containers
(previously available as_experimental_buffer_containers
). When your Function is actively handling inputs, the autoscaler will spin up additionalbuffer_containers
so that subsequent inputs will not be blocked on cold starts. When the Function is idle, it will still scale down to the value given bymin_containers
..keep_warm
method and replacing it with a new more general method,.update_autoscaler
, which can be used to override any of these parameters without redeploying your App.