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

[Bug fix][Core] fixup ngram not setup correctly #4551

Merged
merged 7 commits into from
May 7, 2024

Conversation

leiwen83
Copy link
Contributor

@leiwen83 leiwen83 commented May 2, 2024

ngram_prompt_lookup_max/ngram_prompt_lookup_min need to be past through SpecDecodeWorker.create_worker's draft_worker_kwargs.

If those two doesn't get past, now there will be exception as dict cannot pop those two keys.

wenlei03 added 2 commits May 2, 2024 17:43
ngram_prompt_lookup_max/ngram_prompt_lookup_min need to be past
through SpecDecodeWorker.create_worker's draft_worker_kwargs.

If those two doesn't get past, now there will be exception as
dict cannot pop those two keys.
@rkooo567
Copy link
Collaborator

rkooo567 commented May 2, 2024

cc @comaniac

@leiwen83 leiwen83 marked this pull request as ready for review May 2, 2024 14:14
Copy link
Collaborator

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Oops. We can merge first but it should be better to add a unit test to cover this case.

@cadedaniel
Copy link
Collaborator

+1. Let's get a test covering this path.

@cadedaniel
Copy link
Collaborator

Why was it not covered by existing tests?

@comaniac
Copy link
Collaborator

comaniac commented May 2, 2024

Why was it not covered by existing tests?

I guess existing tests directly initiated the worker, but this is more like an end-to-end path starting from a higher level?

@leiwen83
Copy link
Contributor Author

leiwen83 commented May 3, 2024

Why was it not covered by existing tests?

It is for current ngram still use draft model set as target model to get some info like vocab size. In this failure, ngram testcase is actually turned into multistep case with draft model same as target model...

I add a check assert in conftest to ensure we current in ngram running path, when corresponding param is set.

tests/spec_decode/e2e/conftest.py Outdated Show resolved Hide resolved
@cadedaniel
Copy link
Collaborator

Retrying test infra failure

@comaniac
Copy link
Collaborator

comaniac commented May 7, 2024

@cadedaniel this should be able to merge.

@cadedaniel cadedaniel enabled auto-merge (squash) May 7, 2024 18:40
@cadedaniel cadedaniel merged commit 8344f77 into vllm-project:main May 7, 2024
49 checks passed
z103cb pushed a commit to z103cb/opendatahub_vllm that referenced this pull request May 8, 2024
@simon-mo
Copy link
Collaborator

simon-mo commented May 8, 2024

Spec decode tests start failing in main branch after this PR https://buildkite.com/vllm/ci/builds/6784#018f551e-d727-491c-be34-9d9fa29f4ea4

@comaniac
Copy link
Collaborator

comaniac commented May 8, 2024

The fix PR is here: #4672
Meanwhile, @cadedaniel adjusted the test config to workaround this issue in #4592, so we should be good after merging this one.

robertgshaw2-redhat pushed a commit to neuralmagic/nm-vllm that referenced this pull request May 19, 2024
dtrifiro pushed a commit to dtrifiro/vllm that referenced this pull request May 21, 2024
Temirulan pushed a commit to Temirulan/vllm-whisper that referenced this pull request Sep 6, 2024
Co-authored-by: Lei Wen <[email protected]>
Co-authored-by: Cade Daniel <[email protected]>
Co-authored-by: Cody Yu <[email protected]>
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.

6 participants