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.

Sign up for GitHub

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

Jump to bottom

[Misc] LoRA - Refactor Punica ops tests #12970

Merged
merged 3 commits into from
Feb 11, 2025
Merged

[Misc] LoRA - Refactor Punica ops tests #12970

merged 3 commits into from
Feb 11, 2025

Conversation

varun-sundar-rabindranath
Copy link
Contributor

@varun-sundar-rabindranath varun-sundar-rabindranath commented Feb 8, 2025 •
edited by github-actions bot
Loading

Cleanup/Refactor punica kernel tests - No functional change.

Specifically, merge tests/lora/test_punica_ops_sizes.py and tests/lora/test_punica_ops_variation.py into a single file.

refactor punica ops tests
3133c75 
Signed-off-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2025

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

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 either: Add ready label to the PR or enable auto-merge.

🚀

jeejeelee
jeejeelee reviewed Feb 9, 2025
View reviewed changes
tests/lora/test_punica_ops.py Outdated
hidden_sizes included in the LoRA models currently supported by vLLM. It tests
whether the corresponding Triton kernel can run normally when tensor parallelism
is set to [1, 2, 4, 8, 16, 32, 64].
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this comment is unused now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad ! Thanks for catching this 👍

jeejeelee
jeejeelee reviewed Feb 9, 2025
View reviewed changes
tests/lora/test_punica_ops.py
DTYPES = [torch.float16, torch.bfloat16]
DEVICES = [f"cuda:{0}"]
SEED = [0]

Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: Can we move these configurations before the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean before the utility functions (before line 27) ? I thought this was good as it is just before the pytest functions where they are used - what do you think ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, let's keep this

remove unused comment
cfb41ee 
Signed-off-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
jeejeelee
jeejeelee approved these changes Feb 10, 2025
View reviewed changes
Copy link
Collaborator

@jeejeelee jeejeelee 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 clean up, LGTM

@jeejeelee jeejeelee added the ready ONLY add when PR is ready to merge/full CI is needed label Feb 10, 2025
@DarkLight1337
Copy link
Member

It appears that LoRA Test 3 is failing now.

@varun-sundar-rabindranath
Copy link
Contributor Author

The tests are running out-of-memory. Ill look into it 👍

revert additional tests
0d019e4 
Signed-off-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
@varun-sundar-rabindranath
Copy link
Contributor Author

@DarkLight1337 - I expanded the tests to test bigger batch sizes. These tests triggered the OOM. I have reverted it now. I think it should be good now 🤞

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) February 10, 2025 17:17
@DarkLight1337 DarkLight1337 merged commit 78a141d into vllm-project:main Feb 11, 2025
21 checks passed
SzymonOzog pushed a commit to SzymonOzog/vllm that referenced this pull request Feb 12, 2025
@varun-sundar-rabindranath @SzymonOzog
[Misc] LoRA - Refactor Punica ops tests (vllm-project#12970)
4e6569e 
Signed-off-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
Co-authored-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
Signed-off-by: SzymonOzog <szymon.ozog@aleph-alpha.com>
kwang1012 pushed a commit to kwang1012/vllm that referenced this pull request Feb 12, 2025
@varun-sundar-rabindranath @kwang1012
[Misc] LoRA - Refactor Punica ops tests (vllm-project#12970)
7c291fc 
Signed-off-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
Co-authored-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
panf2333 pushed a commit to yottalabsai/vllm that referenced this pull request Feb 18, 2025
@varun-sundar-rabindranath @panf2333
[Misc] LoRA - Refactor Punica ops tests (vllm-project#12970)
8d91874 
Signed-off-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
Co-authored-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
kerthcet pushed a commit to kerthcet/vllm that referenced this pull request Feb 21, 2025
@varun-sundar-rabindranath @kerthcet
[Misc] LoRA - Refactor Punica ops tests (vllm-project#12970)
354d458 
Signed-off-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
Co-authored-by: Varun Sundar Rabindranath <varun@neuralmagic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeejeelee jeejeelee jeejeelee approved these changes

Assignees
No one assigned
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@varun-sundar-rabindranath @DarkLight1337 @jeejeelee