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.

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

[Style]: flake8 violation E721 #48084

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

CheyuWu
Copy link

@CheyuWu CheyuWu commented Oct 17, 2024

Why are these changes needed?

While running the pre-commit hook of flake8, the following error occurs if Python version is 3.12. It's because the version of flake8 is too old.
This commit fixes code that violates the E721 rules.

Related issue number

Closes #48058

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 CheyuWu changed the title style: flake8 violation E721 [Style]: flake8 violation E721 Oct 17, 2024
Copy link
Member

@MortalHappiness MortalHappiness left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but we should not use isinstance when we need to check two instances are strictly type equal.

cc @rynewang

python/ray/tests/modin/modin_test_utils.py Outdated Show resolved Hide resolved
python/ray/data/tests/test_numpy_support.py Outdated Show resolved Hide resolved
Comment on lines 25 to 26
assert type(a) is type(b), (type(a), type(b))
assert type(a[0]) is type(b[0]), (type(a[0]), type(b[0]))
Copy link
Member

@MortalHappiness MortalHappiness Oct 18, 2024

Choose a reason for hiding this comment

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

I found that for rule E721, the behavior of old and new version of flake8 are different. So maybe add back the # noqa: E721 comment, and then add a TODO comment to say that this # noqa should be removed after flake8 is upgraded. For example: TODO: Remove noqa after flake8 being upgraded to 7.1.1

Failed CI: https://buildkite.com/ray-project/microcheck/builds/6190#0192a009-91f2-402b-b318-d7a5fdc04a24

Old version of flake8:

image

New version (7.1.1) of flake8:

image

@@ -105,7 +105,7 @@ def df_equals(df1, df2):
if isinstance(df1, pandas.DataFrame) and isinstance(df2, pandas.DataFrame):
if (df1.empty and not df2.empty) or (df2.empty and not df1.empty):
assert False, "One of the passed frames is empty, when other isn't"
elif df1.empty and df2.empty and type(df1) != type(df2):
elif df1.empty and df2.empty and type(df1) is not type(df2):
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix flake8 rule E721: Do not compare types, use 'isinstance()'
2 participants