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

[CI/Build] Avoid downloading all HF files in RemoteOpenAIServer #7836

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

DarkLight1337
Copy link
Member

Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

  • Comment /ready on the PR
  • Add ready label to the PR
  • Enable auto-merge.

🚀

Copy link
Sponsor Collaborator

@mgoin mgoin left a comment

Choose a reason for hiding this comment

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

Nice fix

tests/utils.py Outdated
self.host = str(args.host or 'localhost')
self.port = int(args.port)

# download the model before starting the server to avoid timeout
engine_args = AsyncEngineArgs.from_cli_args(args)
engine_config = engine_args.create_engine_config()
Copy link
Member

Choose a reason for hiding this comment

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

can we directly create a load config object? it should be simple in my opinion. just load format auto.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's fix the tests first before simplifying this.

Copy link
Member Author

@DarkLight1337 DarkLight1337 Aug 25, 2024

Choose a reason for hiding this comment

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

I'm not that familiar with the model loading code so may need some help regarding fixing the tests.

tests/utils.py Outdated
@@ -60,36 +61,40 @@ class RemoteOpenAIServer:

def __init__(self,
model: str,
cli_args: List[str],
serve_args: List[str],
Copy link
Member

Choose a reason for hiding this comment

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

why rename the arg name btw?

Copy link
Member Author

@DarkLight1337 DarkLight1337 Aug 25, 2024

Choose a reason for hiding this comment

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

I meant to associate them with vllm serve specifically, not just any CLI. Perhaps it could be clearer.

Edit: Updated the name to vllm_serve_args


parser = FlexibleArgumentParser(
description="vLLM's remote OpenAI server.")
parser = make_arg_parser(parser)
args = parser.parse_args(cli_args)
Copy link
Member

Choose a reason for hiding this comment

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

I think you just need args = parser.parse_args(["--model", model, *cli_args])

Copy link
Member

@youkaichao youkaichao left a comment

Choose a reason for hiding this comment

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

thanks for the fix!

@DarkLight1337 DarkLight1337 changed the title [Bugfix][CI/Build] Fix model name being overwritten [CI/Build] Avoid downloading all HF files in RemoteOpenAIServer Aug 26, 2024
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) August 26, 2024 04:03
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 26, 2024
@DarkLight1337 DarkLight1337 merged commit 029c71d into vllm-project:main Aug 26, 2024
56 checks passed
@DarkLight1337 DarkLight1337 deleted the fix-test-openai-server branch August 26, 2024 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants