GeezerVison is showing github.com — feat: faster linting with ruff · Issue #3956 · feast-dev/feast
feat: faster linting with ruff #3956DescriptionIs your feature request related to a problem? Please describe.
Since we're adopting flake8 and isort which are conventional formatters, however apparently they usually take some time to finish the job.
Describe the solution you'd like
Adopting ruff would improve significantly lint check due to its rust-backed mechanism.
Moreover, ruff offers unified settings and simplifies packages installation, all we need to focus is ruff configuration.
So that we could remove fragmented config files in our repo
Describe alternatives you've considered
Stick with current settings Activityshuchu commented on Feb 18, 2024 I support this. Reducing the number of dependents is also an advantage (as using Ruff instead of insort, flake8 and pylint). dmartinol commented on Mar 22, 2024 Hi all, I can take a look at this issue, as agreed with @jeremyary
FYI, I've just started playing with the latest ruff(*), which uses flake8 6.1.0 instead of Feast default of flake8>=6.0.0,<6.1.0, and I see that new errors are raised on the current codebase, like:
E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
That translates to ruff's:
E721 Do not compare types, use `isinstance()`
But, despite the claim of Drop-in parity with Flake8, I see that the list of E721 errors is different, so I guess that their implementation is not equivalent.
During this initial investigation, I've found also a few discrepancies, even using the same settings, so I'm wondering if we can just add some extra ignore options to lead to the equivalent linting output with no code changes or we want to go further and also fix the new issues identified by ruff. I'd personally postpone the code changes to avoid any risks, but I'd like to hear your opinion.
(*) do you see any reasons for not using the latest version? shuchu commented on Mar 22, 2024 I believe we don't have to config the Ruff exact same as before. I prefer the "ignore" option and let's put Ruff to work. Once it is done. We can have new PRs to fix those "ignored" code. sudohainguyen commented on Mar 23, 2024 CollaboratorAuthor More actions dmartinol commented on Mar 25, 2024 One more point: ATM Feast uses mypy, isort, flake8 and black in the lint-python step of the CI, but mypy is not covered by ruff:
It's recommended that you use Ruff in conjunction with a type checker, like Mypy, Pyright, or Pyre, with Ruff providing faster feedback on lint violations and the type checker providing more detailed feedback on type errors.
Can we state that the request for this issue is to move from mypy, isort, flake8 and black to mypy and ruff linting stack?
Also, the format-python step is currently using isort and black to format code using the pre-commit hook: do we also want to replace these two with ruff and try to replicate the same configuration, despite some known deviations? Sorry for asking the trivial question, but I was not sure if the issue was referring to both the linting and formatting steps or just to one of them. dmartinol commented on Mar 25, 2024 Hi, another thing I've just discovered and then I'll stop bothering until you provide any guidance.
The current configuration enables error code C, but also ignores error C901, e.g. the only error detected by the McCabe plugin. Basically, the following configuration means that category "C" can be completely omitted from the select setting:
[flake8]
ignore = E203, E266, E501, W503, C901
select = B,C,E,F,W,T4
The reason for saying that is that:
ruff seems to compute the code complexity in a different way, look at the results on the same file, where the complexity of the function get_app is 28 according to mccabe and 23 using ruff:
(feast) dmartino@dmartino-mac python % pip list | grep mccabe
mccabe 0.7.0
(feast) dmartino@dmartino-mac python % python -m mccabe --min 5 feast/feature_server.py
48:0: 'get_app' 28
(feast) dmartino@dmartino-mac python % python -m ruff --version
ruff 0.3.3
(feast) dmartino@dmartino-mac python % python -m ruff check feast/feature_server.py
feast/feature_server.py:48:5: C901 `get_app` is too complex (23 > 20)
ruff has many other error codes in the C category, like the C4-flake8-comprehensions section.
In short: the current configuration is misleading and categoryC should be omitted from the results. sudohainguyen commented on Mar 25, 2024 CollaboratorAuthor More actions Yes, should take additional efforts to pick up the right setting dmartinol commented on Mar 25, 2024 A possible configuration to replicate the current linter behavior (note: it must be in pyproject.toml because setyp.cfg is not supported):
[tool.ruff]
exclude = [".git","__pycache__","docs/conf.py","dist","feast/protos","feast/embedded_go/lib","feast/infra/utils/snowflake/snowpark/snowflake_udfs.py"]
[tool.ruff.lint]
select = ["E","F","W","I"]
ignore = ["E203", "E266", "E501", "E721"]
[tool.ruff.lint.isort]
known-first-party = ["feast", "feast", "feast_serving_server", "feast_core_server"]
default-section = "third-party"
No further setting is needed for isort as:
Ruff's import sorting is intended to be near-equivalent to isort's when using isort's profile = "black".
and this profile is almost equivalent to our current isort setting.
Note 1: Few files (4) must be fixed to adapt to the known differences betweenruff and isort, see here, but these should be minor changes.
Note 2: Be aware that, if this issue was originated because of it usually take some time to finish the job, these are the times I experimented on my Mac Intel
linting: from ~11/12" to ~7/8" (*)
formatting: from 2" to <0.2"
(*) in both cases, ~7" are consumed by mypy
Pls let me know if it's worth going forward in any way and how we want to deal with the file formatting step. dmartinol commented on Mar 26, 2024 As agreed with @jeremyary , sent PR #4043 to clarify what I tried to explain in previous comments.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|