-
Notifications
You must be signed in to change notification settings - Fork 24
Pull VllmConfig construction up to Policy #154
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, but looks clean!
(Sorry about the merge conflicts :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did something like this in #151, wondering what you think about that approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i think i like yours better
@allenwang28 Ah I missed this change in #151, part of reason for this mini PR was actually your temp work for testing vllm I considered doing it as a classmethod like you have it at first, but |
Thanks for spotting and fixing the issue with |
Move
VllmConfig
construction intoEngineConfig
at thePolicy
level instead of deferring to thePolicyWorker
.This simplifies the PolicyWorker construction logic from:
to just
Misc:
vllm_args
from Policytemp
andtop_p
weren't being passed down from yaml cc: @DNXie