feat: faster linting with ruff

[sudohainguyen]

Is 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

[shuchu]

I support this. Reducing the number of dependents is also an advantage (as using Ruff instead of insort, flake8 and pylint).

[dmartinol]

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]

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]

agree with @shuchu @dmartinol let me give you a hand though

[dmartinol]

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]

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:

1. 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)

1. 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]

Yes, should take additional efforts to pick up the right setting

[dmartinol]

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]

As agreed with @jeremyary , sent PR https://github.com/feast-dev/feast/pull/4043 to clarify what I tried to explain in previous comments.