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

[Refactor] fix ruff rule C416: unnecessary-comprehension #49852

Merged
merged 5 commits into from
Jan 24, 2025
Merged

[Refactor] fix ruff rule C416: unnecessary-comprehension #49852

merged 5 commits into from
Jan 24, 2025

Conversation

kenchung285
Copy link
Contributor

Why are these changes needed?

It's unnecessary to use a dict/list/set comprehension to build a data structure if the elements are unchanged. Wrap the iterable with dict(), list(), or set() instead.

Ref: https://docs.astral.sh/ruff/rules/unnecessary-comprehension/#unnecessary-comprehension-c416

Related issue number

#47991

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@jjyao jjyao added the go add ONLY when ready to merge, run all tests label Jan 15, 2025
@kenchung285
Copy link
Contributor Author

@MortalHappiness PTAL

@jcotant1 jcotant1 added the core Issues that should be addressed in Ray Core label Jan 15, 2025
@MortalHappiness MortalHappiness mentioned this pull request Jan 15, 2025
Open
34 tasks
@kenchung285 kenchung285 force-pushed the #47991-C416 branch 5 times, most recently from 0635492 to a79f6f5 Compare January 17, 2025 17:29
@kenchung285
Copy link
Contributor Author

@MortalHappiness PTAL

@MortalHappiness
Copy link
Member

Please resolve conflicting files.

aslonnie
aslonnie reviewed Jan 18, 2025
View reviewed changes
python/ray/serve/grpc_util.py
@@ -18,13 +18,13 @@ def __init__(self, grpc_context: grpc._cython.cygrpc._ServicerContext):
self._auth_context = grpc_context.auth_context()
self._code = grpc_context.code()
self._details = grpc_context.details()
self._invocation_metadata = [
self._invocation_metadata = [ # noqa: C416
Copy link
Collaborator

Choose a reason for hiding this comment

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

why? invocation_metadata() is not an iterator of tuples?

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 failed in CI of previous commit using list()
But I pass CI in the following commit after reset to explicit list comprehension
I think elements in invocation_metadata() is not always tuple

Copy link
Collaborator

Choose a reason for hiding this comment

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

@zcin or @edoakes , could you provide an opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can just do: list(grpc_context.invocation_metadata)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's my new commit that use list(grpc_context.invocation_metadata()), but its CI failed.
0764a0b

Copy link
Contributor

Choose a reason for hiding this comment

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

here's the error: "Can't pickle <class 'importlib._bootstrap._Metadatum'>: attribute lookup _Metadatum on importlib._bootstrap failed"

Copy link
Contributor

Choose a reason for hiding this comment

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

so somehow these two lines don't produce the same resulting list... amazing

i'm fine to just add the noqa

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll reset to the version of using noqa and rebase the master branch after work

@kenchung285 kenchung285 force-pushed the #47991-C416 branch 2 times, most recently from 6b565db to 914eb64 Compare January 21, 2025 13:21
@MortalHappiness MortalHappiness added go add ONLY when ready to merge, run all tests and removed go add ONLY when ready to merge, run all tests labels Jan 22, 2025
@aslonnie aslonnie self-requested a review January 24, 2025 03:01
@aslonnie aslonnie merged commit 28715d3 into ray-project:master Jan 24, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MortalHappiness MortalHappiness MortalHappiness left review comments

@edoakes edoakes edoakes approved these changes

@hongpeng-guo hongpeng-guo Awaiting requested review from hongpeng-guo hongpeng-guo is a code owner

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

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

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

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

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

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

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

@aslonnie aslonnie Awaiting requested review from aslonnie

Assignees

@aslonnie aslonnie

Labels
core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kenchung285 @MortalHappiness @edoakes @aslonnie @jjyao @jcotant1