-
Notifications
You must be signed in to change notification settings - Fork 692
feat: Enable intel gaudi on dynamo #4209
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
base: main
Are you sure you want to change the base?
Conversation
|
👋 Hi Spycsh! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
1d97695 to
82fe0ea
Compare
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.
@tmonty12 @julienmancuso can you help review?
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.
Thanks, the k8s related path are not validated currently from my side, and I think it would be good and easier to add gaudi-related resource type/plugin after this PR #3548 is merged.
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.
@alec-flowers @ziqifan617 can you review vllm changes and make sure there's no issues for either disagg or kvbm connector logic?
|
Related: #3548 |
|
@Spycsh thanks for contributing this! Can you |
| # if a specific --kv_transfer_config is passed, ignore the --connector handling | ||
| if has_connector_flag and not has_kv_transfer_config: |
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 think it worth keeping ValueError. Without it, we might have a silent fail, which is frustrating from a user's perspective
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, could you please not change this in this PR. This one: #4317 has a nice fix, which will benefit this PR as well
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.
Sure. I will revert this fix in this PR after #4317 is merged.
Overview:
Draft on enabling intel gaudi on dynamo. Also fixed issues mentioned in 4208.
Details:
Validate running a vLLM PD disaggregation example in Dynamo on Intel Gaudi. vLLM with NIXLConnector is enabled with the support on vLLM-Gaudi through on-host buffer via UCX.
Here are the steps:
Where should the reviewer start?
Regarding to 4208, components/src/dynamo/vllm/args.py and components/src/dynamo/vllm/handlers.py should be the fix.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)