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

Fix vLLM generation with sampling params #578

Merged
merged 6 commits into from
Feb 21, 2025
Merged

Fix vLLM generation with sampling params #578

merged 6 commits into from
Feb 21, 2025

Conversation

lewtun
Copy link
Member

@lewtun lewtun commented Feb 20, 2025

This PR fixes a bug where it was not possible to pass generation_parameters to lighteval vllm due to:

TypeError: lighteval.models.vllm.vllm_model.VLLMModelConfig() got multiple values for keyword argument 'generation_parameters'

The root cause was due to model_args containing generation_parameters after we extracted them.

@lewtun lewtun requested a review from NathanHB February 20, 2025 21:33
@HuggingFaceDocBuilderDev
Copy link
Collaborator

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@@ -138,6 +139,8 @@ def vllm(
generation_parameters = GenerationParameters.from_dict(config)
else:
generation_parameters = GenerationParameters.from_model_args(model_args)
# We slice out generation_parameters from model_args to avoid double-counting in the VLLMModelConfig
model_args = re.sub(r"generation_parameters=\{.*?\},?", "", model_args).strip(",")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit hacky, so am open to other ideas!

Copy link
Member

Choose a reason for hiding this comment

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

Works for me for now, I trust you debuggued it - but can you create an issue so we don't forget to upgrade to something more robust?

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue here! #579

Copy link
Member

@clefourrier clefourrier left a comment

Choose a reason for hiding this comment

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

LGTM - indeed a bit hackish but since it's urgent I'rm ok with it :)
Just remove the prints or pass them through the logger instead

@clefourrier
Copy link
Member

feel free to merge once tests are go again :)

@lewtun lewtun merged commit ebb7377 into main Feb 21, 2025
4 checks passed
@lewtun lewtun deleted the lewtun/fix-lcb branch February 21, 2025 10:30
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.

3 participants