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

Style: update flake8 to latest #48006

Closed
wants to merge 7 commits into from
Closed

Style: update flake8 to latest #48006

wants to merge 7 commits into from

Conversation

CheyuWu
Copy link

@CheyuWu CheyuWu commented Oct 13, 2024

Why are these changes needed?

While running the pre-commit hook of flake8, the following error occurs if Python version is 3.12.

Related issue number

Closes #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 :(

cc @MortalHappiness

@CheyuWu
style: Fix Python linter issues B023, C416, C419, C420, C418, E721, E501
666e100 
Signed-off-by: jerry <cheyu1220@gmail.com>
@CheyuWu
style: change flake8 version, update style B020,B023,B024,B027,B028,B…
9e01a55 
…026,B035

Signed-off-by: jerry <cheyu1220@gmail.com>
@CheyuWu
style: ignore B019, F401, F811
67cd9d3 
Signed-off-by: jerry <cheyu1220@gmail.com>
@jjyao
Copy link
Collaborator

jjyao commented Oct 14, 2024

@rynewang and @MortalHappiness please shepherd this PR.

rynewang
rynewang reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@rynewang rynewang left a comment

Choose a reason for hiding this comment

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

It's nice to have a chance to tidy things up! @aslonnie could you take a look on the ci change?

doc/source/custom_directives.py Outdated
@@ -751,17 +751,17 @@ def render_library_examples(config: pathlib.Path = None) -> bs4.BeautifulSoup:
# Group the examples by the ExampleConfig.groupby value:
examples = defaultdict(list)
example_config = ExampleConfig(config, app.srcdir)
for example in example_config:
for _example in example_config:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change?

Copy link
Author

Choose a reason for hiding this comment

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

Ops, I misread that. Let me make some corrections.

python/ray/_private/accelerators/accelerator.py Outdated
@@ -86,7 +86,7 @@ def validate_resource_request_quantity(
@staticmethod
@abstractmethod
def get_current_process_visible_accelerator_ids() -> Optional[List[str]]:
"""Get the ids of accelerators of this family that are visible to the current process.
"""Get the ids of accelerators of this family that are visible to the current process. # noqa:E501
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we break the line instead of ignoring the line-too-long error?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I will handle that

@rynewang
Copy link
Contributor

We got some CI failures:


[2024-10-13T19:34:00Z] Traceback (most recent call last):
--
  | [2024-10-13T19:34:00Z]   File "/root/.cache/bazel/_bazel_root/1df605deb6d24fc8068f6e25793ec703/execroot/com_github_ray_project_ray/bazel-out/k8-opt/bin/python/ray/tests/test_serialization.runfiles/com_github_ray_project_ray/python/ray/tests/test_serialization.py", line 15, in <module>
  | [2024-10-13T19:34:00Z]     import ray
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/__init__.py", line 113, in <module>
  | [2024-10-13T19:34:00Z]     from ray._private.worker import (  # noqa: E402,F401
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/_private/worker.py", line 56, in <module>
  | [2024-10-13T19:34:00Z]     import ray.actor
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/actor.py", line 39, in <module>
  | [2024-10-13T19:34:00Z]     from ray.util.tracing.tracing_helper import (
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/util/tracing/tracing_helper.py", line 29, in <module>
  | [2024-10-13T19:34:00Z]     from ray.runtime_context import get_runtime_context
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/runtime_context.py", line 8, in <module>
  | [2024-10-13T19:34:00Z]     from ray.runtime_env import RuntimeEnv
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/runtime_env/__init__.py", line 1, in <module>
  | [2024-10-13T19:34:00Z]     from ray._private.runtime_env.mpi import mpi_init  # noqa: E402,F401
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/_private/runtime_env/mpi.py", line 5, in <module>
  | [2024-10-13T19:34:00Z]     from ray._private.runtime_env.plugin import RuntimeEnvPlugin
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/_private/runtime_env/plugin.py", line 24, in <module>
  | [2024-10-13T19:34:00Z]     class RuntimeEnvPlugin(ABC):
  | [2024-10-13T19:34:00Z]   File "/rayci/python/ray/_private/runtime_env/plugin.py", line 32, in RuntimeEnvPlugin
  | [2024-10-13T19:34:00Z]     def validate(runtime_env_dict: dict) -> None:
  | [2024-10-13T19:34:00Z]   File "/opt/miniconda/lib/python3.9/abc.py", line 24, in abstractmethod
  | [2024-10-13T19:34:00Z]     funcobj.__isabstractmethod__ = True
  | [2024-10-13T19:34:00Z] AttributeError: attribute '__isabstractmethod__' of 'staticmethod' objects is not writable

@CheyuWu
style: Change the variables back to their original ones.
cf1e7bf 
Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
@CheyuWu
style: break the line instead of ignoring the line-too-long error and…
4682bf5 
… fix not required change

Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
@CheyuWu
fix: fix ci __isabstractmethod__ error
1bb9420 
Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
@CheyuWu
fix: update mccabe==0.7.0 to handle conflicting dependencies
5d72a74 
Signed-off-by: Cheyu Wu <cheyu1220@gmail.com>
jjyao
jjyao reviewed Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

It should be nice to break this into several smaller PRs: for example, one PR just changing type()==x to isinstance.

doc/source/custom_directives.py
@@ -776,8 +776,8 @@ def render_library_examples(config: pathlib.Path = None) -> bs4.BeautifulSoup:
soup.append(page_text)

container = soup.new_tag("div", attrs={"class": "example-index"})
for group, examples in examples.items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this change?

Copy link
Author

Choose a reason for hiding this comment

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

Because if we don't change examples to _example, it will trigger flake8's warning:

"Found for loop that reassigns the iterable it is iterating with each iterable value. Flake8 (B020)"

Copy link
Author

Choose a reason for hiding this comment

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

By the way, regarding a PR that only changes type() == x to isinstance, do I need to close the current PR and open a new one for this?"

Copy link
Collaborator

Choose a reason for hiding this comment

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

My recommendation is that you create a PR for each flake8 violation code and then this PR will just be the last PR that upgrades flake8.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I'll work on it shortly. Thanks for your suggestion!

Copy link
Member

Choose a reason for hiding this comment

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

@jjyao I agree with you. I'll help @CheyuWu divide this PR into several smaller ones offline, and possibly assign some tasks to other contributors.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid using variable names that start with an underscore. We can discuss how to name these variables after I divide this task into smaller ones.

@MortalHappiness
Copy link
Member

MortalHappiness commented Oct 15, 2024 •
edited
Loading

Closed this PR because this issue has been divided into smaller ones.

See #47991 for details.

@MortalHappiness MortalHappiness mentioned this pull request Oct 15, 2024
Open
@jjyao jjyao mentioned this pull request Oct 16, 2024
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rynewang rynewang rynewang left review comments

@MortalHappiness MortalHappiness MortalHappiness left review comments

@jjyao jjyao jjyao left review comments

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

@shrekris-anyscale shrekris-anyscale Awaiting requested review from shrekris-anyscale

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

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

@akshay-anyscale akshay-anyscale Awaiting requested review from akshay-anyscale akshay-anyscale is a code owner

@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

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang stephanie-wang is a code owner

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

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

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

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

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

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

Assignees

@MortalHappiness MortalHappiness

@rynewang rynewang

@aslonnie aslonnie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Epic][CI] Upgrade flake8 to version 7.1.1
5 participants
@CheyuWu @jjyao @rynewang @MortalHappiness @aslonnie