Skip to content

[Refactor] Remove duplicate ceil_div #20023

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
Merged
merged 2 commits into from
Jun 25, 2025
Merged

[Refactor] Remove duplicate ceil_div #20023

merged 2 commits into from
Jun 25, 2025

Conversation

yewentao256
Copy link
Collaborator

@yewentao256 yewentao256 commented Jun 24, 2025 •
edited by github-actions bot
Loading

Purpose

Remove duplicate ceil_div

@yewentao256
remove duplicate ceil_div
05eaa6e 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
@github-actions GitHub Actions
Copy link

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

🚀

gemini-code-assist[bot]
gemini-code-assist bot reviewed Jun 24, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @yewentao256, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request aims to improve code maintainability and reduce redundancy by centralizing the ceil_div utility function. Previously, this function was defined in multiple locations, leading to duplicated code. This change consolidates ceil_div into a single, shared utility module, ensuring consistency and easier future modifications.

Highlights

  • Code Refactoring: Consolidated the ceil_div utility function into a single, canonical location (vllm/model_executor/layers/quantization/utils/fp8_utils.py).
  • Duplicate Removal: Eliminated redundant local definitions of ceil_div across various files, including benchmarks, tests, and attention/fused_moe modules, by importing the centralized version.
  • Improved Maintainability: Ensures that all parts of the codebase requiring ceiling division use the same, centrally managed implementation, reducing code duplication and improving consistency.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@mergify mergify bot added the performance Performance-related issues label Jun 24, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jun 24, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request removes duplicate ceil_div functions and imports it from vllm.model_executor.layers.quantization.utils.fp8_utils where it is defined. This refactoring improves code maintainability by centralizing the definition of this utility function.

@njhill
Copy link
Member

njhill commented Jun 24, 2025

Thanks @yewentao256!

You missed

vllm/vllm/utils.py

Lines 725 to 727 in 26d34eb

def cdiv(a: int, b: int) -> int:
"""Ceiling division."""
return -(a // -b)
:)

@yewentao256
Copy link
Collaborator Author

Thanks @yewentao256!

You missed

vllm/vllm/utils.py

Lines 725 to 727 in 26d34eb

def cdiv(a: int, b: int) -> int:
"""Ceiling division."""
return -(a // -b)

:)

Thanks! I think this version is better (negative support) , I will replace all other ceil_div using this function

@yewentao256
using cdiv
1adcde5 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
@yewentao256 yewentao256 requested a review from WoosukKwon as a code owner June 24, 2025 15:16
njhill
njhill approved these changes Jun 25, 2025
View reviewed changes
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @yewentao256

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Jun 25, 2025
@WoosukKwon WoosukKwon enabled auto-merge (squash) June 25, 2025 00:54
@WoosukKwon WoosukKwon merged commit 879f69b into vllm-project:main Jun 25, 2025
88 checks passed
@yewentao256 yewentao256 deleted the wye-remove-duplicate-ceil_div branch June 25, 2025 14:25
gmarinho2 pushed a commit to gmarinho2/vllm that referenced this pull request Jun 26, 2025
@yewentao256 @gmarinho2
[Refactor] Remove duplicate ceil_div (vllm-project#20023)
301268b 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
xjpang pushed a commit to xjpang/vllm that referenced this pull request Jun 30, 2025
@yewentao256
[Refactor] Remove duplicate ceil_div (vllm-project#20023)
3cee3f5 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
wseaton pushed a commit to wseaton/vllm that referenced this pull request Jun 30, 2025
@yewentao256
[Refactor] Remove duplicate ceil_div (vllm-project#20023)
c38dac5 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
wseaton pushed a commit to wseaton/vllm that referenced this pull request Jun 30, 2025
@yewentao256
[Refactor] Remove duplicate ceil_div (vllm-project#20023)
56c9430 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
wwl2755-google pushed a commit to wwl2755-google/vllm that referenced this pull request Jul 1, 2025
@yewentao256 @wwl2755-google
[Refactor] Remove duplicate ceil_div (vllm-project#20023)
cb6c625 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
avigny pushed a commit to avigny/vllm that referenced this pull request Jul 31, 2025
@yewentao256 @avigny
[Refactor] Remove duplicate ceil_div (vllm-project#20023)
b67c379 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
Signed-off-by: avigny <47987522+avigny@users.noreply.github.com>
googlercolin pushed a commit to googlercolin/vllm that referenced this pull request Aug 29, 2025
@yewentao256 @googlercolin
[Refactor] Remove duplicate ceil_div (vllm-project#20023)
faa7d83 
Signed-off-by: yewentao256 <zhyanwentao@126.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njhill njhill njhill approved these changes

@mgoin mgoin Awaiting requested review from mgoin mgoin is a code owner

@robertgshaw2-redhat robertgshaw2-redhat Awaiting requested review from robertgshaw2-redhat robertgshaw2-redhat is a code owner

@tlrmchlsmth tlrmchlsmth Awaiting requested review from tlrmchlsmth tlrmchlsmth is a code owner

@WoosukKwon WoosukKwon Awaiting requested review from WoosukKwon WoosukKwon is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
performance Performance-related issues 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
@yewentao256 @njhill @WoosukKwon