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

[CI/Build] bump ruff version, fix linting issues #6546

Closed
wants to merge 1 commit into from
Closed

[CI/Build] bump ruff version, fix linting issues #6546

wants to merge 1 commit into from

Conversation

dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Jul 18, 2024 •
edited
Loading

  • bump ruff to 0.5.5
  • fix new linting issues
  • update ruff workflow for new syntax

@dtrifiro dtrifiro force-pushed the update-ruff branch 4 times, most recently from 611c243 to 583d63c Compare July 18, 2024 16:15
@dtrifiro dtrifiro marked this pull request as ready for review July 18, 2024 16:17
@dtrifiro dtrifiro force-pushed the update-ruff branch 5 times, most recently from 56f6c7e to ada1701 Compare July 24, 2024 10:56
njhill
njhill reviewed Jul 24, 2024
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 @dtrifiro, the changes look good to me

vllm/model_executor/layers/quantization/compressed_tensors/compressed_tensors.py Outdated
Comment on lines 167 to 173
if not (is_symmetric_activation # noqa: SIM103
and is_per_tensor_activation):
Copy link
Member

Choose a reason for hiding this comment

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

curious of the reason for this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.astral.sh/ruff/rules/needless-bool/

Basically ruff here tells you that you can just do

return not(is_symmetric_activation and is_per_tensor_activation)

but since we have a comment below, I think it's clearer if we just suppress this error and keep it as is

vllm/model_executor/layers/quantization/compressed_tensors/utils.py Outdated Show resolved Hide resolved
vllm/model_executor/layers/quantization/compressed_tensors/utils.py Outdated Show resolved Hide resolved
vllm/model_executor/layers/quantization/compressed_tensors/compressed_tensors.py Show resolved Hide resolved
@dtrifiro
Copy link
Contributor Author

CI failures look unrelated?

AssertionError: Error in memory profiling. This happens when the GPU memory was not properly cleaned up before initializing the vLLM instance.

[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_single_prompt_string[The capital of France is] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_single_prompt_string[The future of AI is] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids0] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids1] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids2] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids3] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_multi_prompt_string - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_v1_v2_api_consistency_multi_prompt_tokens - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_encode.py::test_multiple_pooling_params - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_single_prompt_string[The president of the United States is] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_single_prompt_string[The capital of France is] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_single_prompt_string[The future of AI is] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids0] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids1] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids2] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_single_prompt_tokens[prompt_token_ids3] - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_multi_prompt_string - AssertionError: pipeline model parallel group is not initialized
[2024-07-24T15:56:13Z] FAILED entrypoints/llm/test_generate.py::test_v1_v2_api_consistency_multi_prompt_tokens - AssertionError: pipeline model parallel group is not initialized```

@dtrifiro dtrifiro force-pushed the update-ruff branch 3 times, most recently from fb7b534 to 6be352d Compare August 6, 2024 15:34
@dtrifiro
Copy link
Contributor Author

dtrifiro commented Aug 6, 2024

/ready

@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 6, 2024
@dtrifiro dtrifiro force-pushed the update-ruff branch 2 times, most recently from 4443e7c to 6064087 Compare August 12, 2024 09:06
@dtrifiro dtrifiro force-pushed the update-ruff branch 2 times, most recently from 4018a95 to 4e14c2a Compare September 16, 2024 16:37
@dtrifiro
Copy link
Contributor Author

Closed in favor of #8469

@dtrifiro dtrifiro closed this Sep 16, 2024
@dtrifiro dtrifiro deleted the update-ruff branch September 16, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njhill njhill njhill left review comments

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.
2 participants
@dtrifiro @njhill