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

Use ruff instead of flake8 and isort #3213

Merged

Conversation

Lingepumpe
Copy link
Contributor

@Lingepumpe Lingepumpe commented Apr 21, 2023

/!\ This PR contains automatically generated code changes (the run of ruff --fix). Please review the commits individually, to be able to pay more attention to the parts I did manually /!\

Ruff (https://github.com/charliermarsh/ruff) is a very active and very fast tool that can replace flake8 and isort. It includes a lot of advanced rules that flake8 only has if you install additional packages. In addition to formatting includes, ruff can also auto-fix some linter errors automatically. In this PR we remove flake8 and isort, and replace the functionality (and more) with ruff.
Also, a full run of ruff --fix . is run on the code base, which fixes-up a lot of small things throughout. For a impressive example for the ruff automatic fixes, check out this automatic change in embeddings/base.py:

# Before ruff --fix
def _everything_embedded(self, data_points: Sequence[DT]) -> bool:
    for data_point in data_points:
        if self.name not in data_point._embeddings.keys():
            return False
    return True

# After ruff --fix
def _everything_embedded(self, data_points: Sequence[DT]) -> bool:
    return all(self.name in data_point._embeddings for data_point in data_points)

In addition to the automatic fixes, I enabled all rules that seemed sensible and where I was able to manually fix any issues it found - so this PR also contains quite a few manual code quality fixes that ruff suggested.

@Lingepumpe Lingepumpe force-pushed the use_ruff_instead_of_flake8_and_isort branch 8 times, most recently from 2255755 to 9e19531 Compare April 25, 2023 08:33
@Lingepumpe
Copy link
Contributor Author

Rebased on current master.

Copy link
Collaborator

@helpmefindaname helpmefindaname left a comment

Choose a reason for hiding this comment

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

´Thank you for this PR, it looks very exciting to see so many small improvements on the code :-)
There a just a few comments.

requirements-dev.txt Outdated Show resolved Hide resolved
tests/embedding_test_utils.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@helpmefindaname helpmefindaname left a comment

Choose a reason for hiding this comment

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

The changes look good to me, there is still a rebase on the main required, but then I would consider this MR mergeable

@Lingepumpe Lingepumpe force-pushed the use_ruff_instead_of_flake8_and_isort branch from 27d08b6 to 7a57952 Compare April 27, 2023 18:58
@Lingepumpe
Copy link
Contributor Author

Rebased on updated master

@alanakbik alanakbik merged commit 4435a31 into flairNLP:master Apr 28, 2023
@alanakbik
Copy link
Collaborator

@Lingepumpe thanks a lot for adding this! Looking forward to trying this out!

Comment on lines +1796 to +1801
if bioes_tag[:2] in {"B-", "S-"} or (
in_span and previous_tag[2:] != bioes_tag[2:] and (bioes_tag[:2] == "I-" or previous_tag[2:] == "S-")
):
# B- and S- always start new spans
# if the predicted class changes, I- starts a new span
# if the predicted class changes and S- was previous tag, start a new span
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debatable if this improves clarity ;) but many of the other changes are really good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this one is debatable, I considered putting a "#noqa: XYZ" there and keeping the condition as is, but on the other hand this version also has its merits (not duplicate the body in the if - so I just did it the way that ruff was also happy with it :D

@@ -77,7 +74,7 @@ def __init__(
augment_train=augment_train,
)

super(RE_ENGLISH_SEMEVAL2010, self).__init__(
super().__init__(
Copy link
Collaborator

Choose a reason for hiding this comment

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

These automatic super() call changes are somewhat surprising. I wonder what is the motivation for this?

Copy link
Contributor Author

@Lingepumpe Lingepumpe Apr 28, 2023

Choose a reason for hiding this comment

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

super() is sequivalent to super(TheNameOfTheCurrentClass, self) - so if you really are passing the current class name and self, then the shorter way to write it is chosen by ruff - it also makes it clear at first view that nothing extra ordinary with regards to super is going on. If you really want to e.g. skip one hierarchy on inheritance by doing super(TheNameOfASuperClass, self) [which would skip the TheNameOfASuperClass and search "above" it in the class hiarchy], then you would still pass the parameters to super [ruff would not remove them], and a reviewer would see that you are really doing something special with super here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok, thanks for the info!

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.

3 participants