Skip to content

Conversation

@Akshat-Tripathi
Copy link
Contributor

@Akshat-Tripathi Akshat-Tripathi commented Jan 31, 2025

This PR adds a Multi-LoRA implementation that works on the TPU backend, extending the work done in #11100.

Currently this uses pytorch operations for the Punica kernels, but I am going to put up a PR with Pallas kernels soon.

@github-actions
Copy link

👋 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.

🚀

Comment on lines 1081 to 1087
if current_platform.is_tpu():
# Because nan_to_num_ doesn't work with actual -inf values on TPU
neg_inf = torch.finfo(lora_logits.dtype).min
pos_inf = torch.finfo(lora_logits.dtype).max
else:
neg_inf = float("-inf")
pos_inf = float("inf")
Copy link
Contributor

@liangfu liangfu Jan 31, 2025

Choose a reason for hiding this comment

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

these if-else conditions will make vLLM hard to maintain.

file an issue with torch-xla ? or abstract this as part of an utility function ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

abstract this as part of an utility function sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that sounds good I can abstract it away, it was only a problem for that nan_to_num() function though, -inf works properly elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

abstract it away as a short-term solution is fine.

it would better if we can create an issue in torch-xla repo, as a longer-term solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've made the issue here: pytorch/xla#8674

@@ -0,0 +1,58 @@
import torch

from ..torch_ops import bgmv_expand, bgmv_expand_slice, bgmv_shrink
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems the TPU ops are still using PyTorch operators, is it necessary to add the below ops?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sgmv ops are slightly different here because I'm using repeat_interleave with a static size rather than a dynamic tensor, which reduces the compile time quite a bit because torch_xla can't lower the dynamic version properly.


# The platforms that are compatible with the PyTorch-native implementation can
# inherit this class
class PunicaWrapperTPU(PunicaWrapperBase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not directly inherit from PunicaWrapperCPU ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but this code is going to change very soon as I add in the Pallas kernels

@Akshat-Tripathi
Copy link
Contributor Author

It looks like the Async Engine, Inputs, Utils, Worker Test is failing on multimodal inputs, which is WIP right now.
The TPU test seems to be failing on non lora code. Do these tests pass on main? I'm wondering if they're linked to this PR or something else

@miladm
Copy link
Collaborator

miladm commented Feb 7, 2025

cc @lsy323 to take a pass

@miladm miladm requested review from lsy323 and removed request for liangfu February 7, 2025 19:02
@Akshat-Tripathi Akshat-Tripathi marked this pull request as draft February 18, 2025 10:41
@Akshat-Tripathi
Copy link
Contributor Author

Switched to draft while I get refactor for the v1 implementation

@mergify
Copy link

mergify bot commented Feb 18, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @Akshat-Tripathi.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

jeejeelee and others added 19 commits March 3, 2025 18:07
Signed-off-by: Jee Jee Li <pandaleefree@gmail.com>
…llm-project#13841)

Signed-off-by: KuntaiDu <kuntai@uchicago.edu>
Signed-off-by: Kuntai Du <kuntai@uchicago.edu>
Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
…kend class (vllm-project#14065)

Signed-off-by: Sage Moore <sage@neuralmagic.com>
Signed-off-by: Jee Jee Li <pandaleefree@gmail.com>
Signed-off-by: qux-bbb <1147635419@qq.com>
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
@mergify
Copy link

mergify bot commented Mar 3, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @Akshat-Tripathi.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@Akshat-Tripathi
Copy link
Contributor Author

Closing in favour of #14238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.