Skip to content

Conversation

@hmellor
Copy link
Member

@hmellor hmellor commented Oct 5, 2025

Closes #17657

This is a massive change and would be impossible to merge during the week. Any PR's merged while this was waiting for CI would cause merge conflicts which would be time consuming to solve in this PR.

Therefore, the plan to getting this PR merged quickly is as follows:

  • First commit:
    • Make the changes to the pre-commit
    • Run pre-commit run -a and note all the things ruff couldn't automatically fix
    • Add these as pre-file-ignores to pyproject.toml
  • Second commit:
    • Run pre-commit run -a
    • ruff and ruff format should pass immediately

Then, these ignores will be systematically removed in smaller more manageable PRs afterwards.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) October 5, 2025 12:05
@vllm-bot vllm-bot merged commit d6953be into vllm-project:main Oct 5, 2025
83 of 85 checks passed
@hmellor hmellor deleted the ruff branch October 5, 2025 14:08
hmellor added a commit to hmellor/vllm that referenced this pull request Oct 5, 2025
Forward fixes some of the issues skipped by vllm-project#26247

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
hmellor added a commit to hmellor/vllm that referenced this pull request Oct 5, 2025
Forward fixes the last of the issues skipped by vllm-project#26247

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
tomeras91 pushed a commit to tomeras91/vllm that referenced this pull request Oct 6, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
karan pushed a commit to karan/vllm that referenced this pull request Oct 6, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Karan Goel <3261985+karan@users.noreply.github.com>
@kfirwolfson
Copy link

kfirwolfson commented Oct 6, 2025

Hi @hmellor, has anyone run into an issue with the new ruff definitions?
On one hand it straightens out if-else clauses to one liners, but on the other hand it can make these lines too long (>=80)?
This causes pre-commit to constantly fail.

Edit: seems this is a known issue. Appending " #noqa E501" seems to overcome it temporarily. Would love to know how to solve it going forward

@hmellor
Copy link
Member Author

hmellor commented Oct 7, 2025

I think you may have a faulty configuration. This is not a known issue and I don't like using #noqa E501 as a workaorund.

In your Slack message you mentioned that the SIM rule was making lines to long (>80), but the new line length limit is 88.

@hmellor
Copy link
Member Author

hmellor commented Oct 7, 2025

Having just found your PR I think what you have done is reasonable https://github.com/vllm-project/vllm/pull/24520/files#diff-9eeca590fd99f15621897e559dba39b3ec4e7c2c65ec3c3229711689e008b5f4R435

Sorry for the false alarm!

southfreebird pushed a commit to southfreebird/vllm that referenced this pull request Oct 7, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@kfirwolfson
Copy link

kfirwolfson commented Oct 7, 2025

Thanks @hmellor. Yes, sorry, the row is actually 89 chars long. I also prefer not using #noqa E501. Is there another way around the problem?

@hmellor
Copy link
Member Author

hmellor commented Oct 7, 2025

This would do it:

                    scheduler_config = self.vllm_config.scheduler_config
                    cache_hit_threshold = (
                        request.cache_hit_threshold
                        if request.cache_hit_threshold is not None
                        else scheduler_config.global_cache_hit_threshold
                    )

@kfirwolfson
Copy link

kfirwolfson commented Oct 7, 2025

hmm, nice idea.

If we're adding another assignment, one can argue that we can might as well assign the global value and override if request has it.

                    cache_hit_threshold = \
                        self.vllm_config.scheduler_config.global_cache_hit_threshold
                    # Cache hit threshold in request overrides global setting
                    if request.cache_hit_threshold is not None:
                        cache_hit_threshold = request.cache_hit_threshold

I guess it's a matter of preference. The way you suggested looks more Pythonic to me, I'll go with that. Thanks

@ExtReMLapin
Copy link
Contributor

Fucking hell, how to create conflicts in one step on opened PRs

@hmellor
Copy link
Member Author

hmellor commented Oct 8, 2025

Some disruption was inevitable. However:

  • We were going to do this eventually
  • It's better to do it all at once rather than creating continued disruption over a longer period
  • We did it at the weekend when vLLM development is less active
  • We provided detailed instructions on how to resolve the conflicts here

xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
sducouedic pushed a commit to sducouedic/vllm that referenced this pull request Oct 16, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
lywa1998 pushed a commit to lywa1998/vllm that referenced this pull request Oct 20, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
alhridoy pushed a commit to alhridoy/vllm that referenced this pull request Oct 24, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
…oject#26247)

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/build deepseek Related to DeepSeek models documentation Improvements or additions to documentation frontend gpt-oss Related to GPT-OSS models kv-connector llama Related to Llama models multi-modality Related to multi-modality (#4194) new-model Requests to new models performance Performance-related issues qwen Related to Qwen models ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm speculative-decoding structured-output tool-calling tpu Related to Google TPUs v1

Projects

Status: Done
Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

Migrating from yapf to ruff format

6 participants