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

[Bugfix][V1] Fix molmo text-only inputs #11676

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jeejeelee
Copy link
Collaborator

@jeejeelee jeejeelee commented Jan 2, 2025

Reproduce Code

import os
from vllm import LLM, SamplingParams


os.environ["VLLM_USE_V1"] = "1"


MODEL_NAME = "allenai/Molmo-7B-D-0924"
llm = LLM(
    model=MODEL_NAME,
    trust_remote_code=True,
    tensor_parallel_size=1,
    gpu_memory_utilization=0.7,
    enforce_eager=True,
)

stop_token_ids = None
sampling_params = SamplingParams(
    stop_token_ids=stop_token_ids,
    temperature=0,
    max_tokens=128,
)
prompts = [
    "Hello, my name is",
]
# text--only inputs
outputs = llm.generate(
    prompts=prompts,
    sampling_params=sampling_params,
)
print(outputs[0].outputs[0].text)

Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Jee Jee Li <[email protected]>
Copy link

github-actions bot commented Jan 2, 2025

👋 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 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 do one of these:

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

🚀

Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Jee Jee Li <[email protected]>
@DarkLight1337
Copy link
Member

Hmm, maybe we should expand our tests to cover this case...

Copy link
Member

@ywang96 ywang96 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! Can you verify if this yields the same result as main branch on V0? I was mainly curious if the padded dummy image input ids will matter at all for text-only input.

Comment on lines -1093 to -1114
else:
base_image_input_size = image_processor.base_image_input_size
image_patch_size = image_processor.image_patch_size
image_num_patch = (
base_image_input_size[0] // image_patch_size,
base_image_input_size[1] // image_patch_size,
)
n_pixels = image_patch_size * image_patch_size * 3
n_patches = image_num_patch[0] * image_num_patch[1]

image_length_w = image_processor.image_token_length_w
image_length_h = image_processor.image_token_length_h
tokens_per_image = image_length_w * image_length_h
images = torch.full(
(max_total_crops, n_patches, n_pixels),
-1,
dtype=torch.float32,
)
image_input_idx = torch.full(
(max_total_crops, tokens_per_image),
-1,
dtype=torch.int32,
Copy link
Member

@ywang96 ywang96 Jan 2, 2025

Choose a reason for hiding this comment

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

I wasn't sure why this was in the code originally when the AI2 team made the PR to support Molmo on vLLM, but I guess it wasn't an issue back then because it didn't matter on V0 since we didn't use the placeholder ranges for these "dummy" image input indices padded to the prompt token ids.

@jeejeelee
Copy link
Collaborator Author

Thanks for the fix! Can you verify if this yields the same result as main branch on V0? I was mainly curious if the padded dummy image input ids will matter at all for text-only input.

I have verified using the reproduce code above, and the generated results completely align with the main V0

@jeejeelee
Copy link
Collaborator Author

Hmm, maybe we should expand our tests to cover this case...

Where should tests be added for this PR?

@DarkLight1337
Copy link
Member

DarkLight1337 commented Jan 2, 2025

You can edit the image_size_factors in test_models.py.

Hmm, actually it seems that empty image is already included there... why does the test still pass? Never mind, molmo tests aren't being included in test_models.py yet.

@jeejeelee
Copy link
Collaborator Author

You can edit the image_size_factors in test_models.py.

Hmm, actually it seems that empty image is already included there... why does the test still pass? Never mind, molmo tests aren't being included in test_models.py yet.

Alright, then I won't add tests.

@DarkLight1337
Copy link
Member

It seems that we don't have any tests for Molmo at all.

@ywang96
Copy link
Member

ywang96 commented Jan 2, 2025

It seems that we don't have any tests for Molmo at all.

yea I think we decided that if the model support came from the vendor then the test is not required.

@DarkLight1337
Copy link
Member

DarkLight1337 commented Jan 2, 2025

I think to avoid breaking the code in future PRs (especially with the V1 refactoring that's going on), we should add tests for it.

@jeejeelee
Copy link
Collaborator Author

I think to avoid breaking the code in future PRs (especially with the V1 refactoring that's going on), we should add tests for it.

Okay, l will handle this asap

@DarkLight1337
Copy link
Member

Sorry didn't realize you added tests.

@DarkLight1337
Copy link
Member

Please fix the lint errors

Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Jee Jee Li <[email protected]>
@ywang96 ywang96 added the ready ONLY add when PR is ready to merge/full CI is needed label Jan 6, 2025
@ywang96 ywang96 enabled auto-merge (squash) January 6, 2025 11:05
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