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

[server] Passing model_override_args to launch_server via the CLI. #1298

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

kevin85421
Copy link
Contributor

@kevin85421 kevin85421 commented Sep 2, 2024

Motivation

  1. Passing model_override_args to launch_server via the CLI.
  2. Add a new function prepare_server_args to make the logic more unit testable.

Closes #591

Modifications

Checklist

  • Format your code according to the Contributor Guide.
  • Add unit tests as outlined in the Contributor Guide.
  • Update documentation as needed, including docstrings or example tutorials.

Signed-off-by: Kai-Hsun Chen <kaihsun@anyscale.com>
Prepare the server arguments from the command line arguments.

Args:
args: The command line arguments. Typically, it should be `sys.argv[1:]`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default value of parse_args when no arguments are passed to the function is sys.argv[1:].

https://github.com/python/cpython/blob/c3ed775899eedd47d37f8f1840345b108920e400/Lib/argparse.py#L1865

Signed-off-by: Kai-Hsun Chen <kaihsun@anyscale.com>
@@ -0,0 +1,24 @@
import unittest
Copy link
Contributor Author

@kevin85421 kevin85421 Sep 2, 2024

Choose a reason for hiding this comment

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

Where should this test be added in the CI pipeline?

https://github.com/sgl-project/sglang/blob/main/.github/workflows/pr-test.yml?

This comment was marked as resolved.

Copy link
Contributor

@merrymercy merrymercy Sep 3, 2024

Choose a reason for hiding this comment

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

@kevin85421 Add it here

suites = {
"minimal": [
"models/test_embedding_models.py",
"models/test_generation_models.py",
"sampling/penaltylib",
"test_chunked_prefill.py",

This "minimal" suite will be run on CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Added 3ecf574.

@@ -12,7 +12,7 @@ def test_default(self):
"python3",
"-m",
"sglang.bench_latency",
"--model",
"--model-path",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--model could be --model-path or --model-override-args.

This comment was marked as resolved.

Copy link
Contributor

@merrymercy merrymercy Sep 3, 2024

Choose a reason for hiding this comment

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

Can you rename --model-override-args to --override-model-args?

In the meanwhile, we can change all --model in the code base to --model-path to avoid future ambiguity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you rename --model-override-args to --override-model-args?

How about --json-model-override-args, or do you prefer --override-model-args?

In the meanwhile, we can change all --model in the code base to --model-path to avoid future ambiguity

image

@merrymercy
Copy link
Contributor

Let me know when it is ready for review.

@merrymercy merrymercy marked this pull request as ready for review September 4, 2024 11:00
@kevin85421 kevin85421 changed the title [WIP][server] Passing model_override_args to launch_server via the CLI. [server] Passing model_override_args to launch_server via the CLI. Sep 4, 2024
@kevin85421
Copy link
Contributor Author

@merrymercy I have already removed "WIP" from the PR title. It's ready for another review iteration. Thanks!

@merrymercy merrymercy merged commit c9b7591 into sgl-project:main Sep 9, 2024
9 checks passed
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.

model_override_args with server
3 participants