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] Factor out input preprocessing to a separate class #7329

Merged
merged 20 commits into from
Sep 13, 2024

Conversation

DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Aug 9, 2024

Following up to #7258, this PR implements the suggestion by @robertgshaw2-neuralmagic to create a new class for prompt parsing. This helps keep the sync/async versions of the preprocessing code together so they can be updated together easily, while also slimming down the existing engine classes to focus on request processing.

Copy link

github-actions bot commented Aug 9, 2024

👋 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.

🚀

@njhill njhill self-requested a review September 12, 2024 15:32
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @DarkLight1337, just some minor comments.

We may need to re-think some of this soon since we're hoping to get rid of AsyncLLMEngine altogether. I'm thinking we'll hopefully no longer need both sync and async versions of these things.

vllm/inputs/preprocess.py Outdated Show resolved Hide resolved
vllm/inputs/preprocess.py Outdated Show resolved Hide resolved
"""Async version of :meth:`_extract_prompt_components`."""
parsed = parse_singleton_prompt(inputs)

if parsed["type"] == "str":
Copy link
Member

Choose a reason for hiding this comment

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

have a prompt_type = parsed["type"] and reuse instead of repeated lookups? (same for parsed["content"])

Copy link
Member Author

@DarkLight1337 DarkLight1337 Sep 12, 2024

Choose a reason for hiding this comment

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

I tried this, and unfortunately this breaks the type checker's capability to narrow down the type of parsed. So I'm keeping the code as is.


parsed = parse_singleton_prompt(inputs)

if parsed["type"] == "str":
Copy link
Member

Choose a reason for hiding this comment

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

have a prompt_type = parsed["type"] and reuse instead of repeated lookups? (same for parsed["content"])

Copy link
Member Author

Choose a reason for hiding this comment

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

As above.

@DarkLight1337
Copy link
Member Author

DarkLight1337 commented Sep 12, 2024

Thanks for the review! I'll address the detailed comments shortly.

We may need to re-think some of this soon since we're hoping to get rid of AsyncLLMEngine altogether. I'm thinking we'll hopefully no longer need both sync and async versions of these things.

Even without AsyncLLMEngine, the preprocessing logic better fits inside vllm.inputs module, and this PR would reduce the bloat inside LLMEngine itself.

@DarkLight1337
Copy link
Member Author

I have finished addressing your other comments.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) September 12, 2024 17:24
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 12, 2024
@DarkLight1337 DarkLight1337 merged commit 5ec9c0f into vllm-project:main Sep 13, 2024
49 of 50 checks passed
@DarkLight1337 DarkLight1337 deleted the input-preprocessor branch September 13, 2024 03:18
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.

2 participants