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

[Core] Don't do platform detection at import time #12933

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

russellb
Copy link
Member

@russellb russellb commented Feb 8, 2025

Running just vllm or vllm --version or something similar resulted
in several lines of log noise about platform detection. This is because
several places were triggering this to occur at import time by doing the
following:

from vllm.platforms import current_platform

Platform detection runs the first time the current_platform attribute is
requested, which includes this import. Importing vllm.platforms and delaying
the reference to current_platform until code actually needs it avoids running
it in cases where it was never needed.

I only changed the places that caused a problem in my environment when running
vllm, though I'd say importing it like this at the top of a module is an
anti-pattern in general.

Signed-off-by: Russell Bryant [email protected]

Copy link

github-actions bot commented Feb 8, 2025

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

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.

LGTM, I also want to make the import lazy, but unfortunately we have lots of from vllm.platforms import current_platform in the top level, because people don't like the long vllm.platforms.current_platform. usage :(

@DarkLight1337
Copy link
Member

Can you fix the failure in Helm Chart CI?

@russellb
Copy link
Member Author

russellb commented Feb 8, 2025

Can you fix the failure in Helm Chart CI?

thanks for pointing it out. I'll take a look.

@russellb
Copy link
Member Author

russellb commented Feb 8, 2025

LGTM, I also want to make the import lazy, but unfortunately we have lots of from vllm.platforms import current_platform in the top level, because people don't like the long vllm.platforms.current_platform. usage :(

Yeah ... it's not obvious that importing that way is a problem and it'll be hard to remember to look for it during code reviews. I was thinking about changing it to be a function instead of an attribute, then it could be imported this way without a problem. That was just a much bigger change and I wanted to fix this immediate impact to the CLI first.

@russellb
Copy link
Member Author

russellb commented Feb 8, 2025

Can you fix the failure in Helm Chart CI?

thanks for pointing it out. I'll take a look.

I think this was just a huggingface error. I'll get it to run again.

Running just `vllm` or `vllm --version` or something similar resulted
in several lines of log noise about platform detection. This is because
several places were triggering this to occur at import time by doing the
following:

```python
from vllm.platforms import current_platform
```

Platform detection runs the first time the `current_platform` attribute is requested, which includes this import. Importing `vllm.platforms` and delaying the reference to `current_platform` until code actually needs it avoids running it in cases where it was never needed.

I only changed the places that caused a problem in my environment when running `vllm`, though I'd say importing it like this at the top of a module is an anti-pattern in general.

Signed-off-by: Russell Bryant <[email protected]>
@russellb russellb force-pushed the delay-platform-detection branch from cc4124d to a9cb538 Compare February 8, 2025 21:33
When putting in newlines, it ends up being split across several log events.

Signed-off-by: Russell Bryant <[email protected]>
@DarkLight1337 DarkLight1337 enabled auto-merge (squash) February 11, 2025 05:57
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Feb 11, 2025
@DarkLight1337 DarkLight1337 merged commit c320ca8 into vllm-project:main Feb 11, 2025
41 of 47 checks passed
SzymonOzog pushed a commit to SzymonOzog/vllm that referenced this pull request Feb 12, 2025
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