Skip to content

feat: faster linting with ruff #3956

New issue
New issue
Closed
#4043
Closed
feat: faster linting with ruff#3956
#4043
Labels
Community Contribution NeededWe want community to contributekind/housekeeping

Description

@sudohainguyen
sudohainguyen
opened on Feb 18, 2024

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

Activity

  1. sudohainguyen
    added
    kind/featureNew feature or request
    on Feb 18, 2024

  2. shuchu commented on Feb 18, 2024

    @shuchu
    shuchu
    on Feb 18, 2024
    Collaborator

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

  3. sudohainguyen
    added
    Community Contribution NeededWe want community to contribute
    on Feb 19, 2024

  4. dmartinol commented on Mar 22, 2024

    @dmartinol
    dmartinol
    on Mar 22, 2024
    Contributor

    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?

  5. shuchu commented on Mar 22, 2024

    @shuchu
    shuchu
    on Mar 22, 2024
    Collaborator

    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.

  6. sudohainguyen commented on Mar 23, 2024

    @sudohainguyen
    sudohainguyen
    on Mar 23, 2024
    CollaboratorAuthor

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

  7. sudohainguyen
    added
    kind/housekeeping
    and removed
    kind/featureNew feature or request
    on Mar 25, 2024

  8. dmartinol commented on Mar 25, 2024

    @dmartinol
    dmartinol
    on Mar 25, 2024
    Contributor

    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.

  9. dmartinol commented on Mar 25, 2024

    @dmartinol
    dmartinol
    on Mar 25, 2024
    Contributor

    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.

  10. sudohainguyen commented on Mar 25, 2024

    @sudohainguyen
    sudohainguyen
    on Mar 25, 2024
    CollaboratorAuthor

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

  11. dmartinol commented on Mar 25, 2024

    @dmartinol
    dmartinol
    on Mar 25, 2024
    Contributor

    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.

  12. dmartinol commented on Mar 26, 2024

    @dmartinol
    dmartinol
    on Mar 26, 2024
    Contributor

    As agreed with @jeremyary , sent PR #4043 to clarify what I tried to explain in previous comments.

  13. jeremyary
    closed this as completedin #4043on Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Community Contribution NeededWe want community to contributekind/housekeeping

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions