Skip to content

Conversation

@squeakymouse
Copy link
Contributor

No description provided.

str(args.num_workers),
*envs,
"model_engine_server.inference.forwarding.http_forwarder:app",
*extra_args,
Copy link
Contributor

Choose a reason for hiding this comment

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

oh wait hmm I think it might be good to just hardcode in the termination grace period seconds as a default here idk

Copy link
Contributor

Choose a reason for hiding this comment

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

extra_args feels like it should be a dict to me (e.g. --key1 val1 --key2 val2 from {key1: val1, key2: val2}), is there such thing as extra_kwargs?

this way we can set a default of graceful-timeout and override it if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so default it to 600 seconds? Will do! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait didn't see the second comment when I commented ^that (GitHub is bad at refreshing lol 😅 ). From local testing, parser.parse_known_args() will turn --key1 val1 --key2 val2 into extra_args = ["--key1", "val1", "--key2", "val2"], which seems fine to directly pass? :P

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it's fine to pass; I was hoping for a dict but if we don't get that from argparse then we can just directly pass it



def start_gunicorn_server(port: int, num_workers: int, debug: bool) -> None:
def start_gunicorn_server(
Copy link
Contributor

@seanshi-scale seanshi-scale Sep 26, 2023

Choose a reason for hiding this comment

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

wait this is the gateway right? I don't think we need to change things for the gateway worker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I just searched "gunicorn" and made the change everywhere :P Will undo this! :)

@squeakymouse squeakymouse merged commit 4b3ca52 into main Sep 26, 2023
@squeakymouse squeakymouse deleted the katiewu/propagate-gunicorn-params branch September 26, 2023 01:35
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