Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-939

Change how we run python linters on pipelines code

    XMLWordPrintable

Details

    • RFC
    • Status: Implemented
    • Resolution: Done
    • DM
    • None

    Description

      The current way we check python code is:

      • Specify a flake8 configuration matching the style guide.
      • Run flake8 in a GitHub action to ensure compliance before merging is allowed.
      • Run pytest-flake8 when we run the tests (generally via sconsUtils but also using a pytest configuration that adds the "--flake8" option to the pytest call).

      The pytest-flake8 dependency is causing real problems because the package is moribund and it does not support anything newer than flake8 v4. This forces us to pin flake8 to an older version in the rubin-env. There is no hope that pytest-flake8 is going to be resurrected. Additionally, the pytest-flake8 calls are slow since flake8 is run separately for every file and there is no multi-processing internal to flake8. For local development this is not much of a problem because on the second call to pytest the cache is used, but it does add load to Jenkins. When pytest reports durations during builds you will see that at times a FLAKE8 test appears in the top 5 (maybe taking 3 seconds for a single file). As a concrete example, the daf_butler tests on my computer take 408s including the flake8 (no multi-processing) and 300s (no multi-processing) after that when flake8 results have been cached. This overhead is less noticeable when tests run in parallel but it is still there.

      If flake8 is run standalone and allowed to use multiple cores (flake8 v4 does not allow this on macOS), something like daf_butler can complete the test in 2 or 3 seconds. There is no caching so every time it takes the same time.

      There is though an alternative to flake8 called ruff that has a number of advantages:

      • It is compatible with flake8 (uses the same error codes) and can be a drop-in replacement with a suitable configuration.
      • It is incredibly fast and includes caching to make it even faster.
      • It comes by default with additional checkers so new checks can be added without requiring a new support package to be installed into the environment. For example, enabling pydocstyle requires solely adding a "D" to the ruff configuration. Enabling "UP" will show you where you can modernize your python code.
      • In some instances it can automatically fix problems (most "UP" errors and some "D" errors).
      • Like flake8 it can be run by pre-commit.

      The timing improvements are stark. 0.25s for daf_butler compared to 3 seconds (but using much less CPU) and 0.02s subsequently. Indeed, the entirety of lsst_distrib takes only 3 seconds the first time it runs.

      This RFC proposes the following:

      • Drop pytest-flake8 from all packages.
      • Change sconsUtils to always run a linter if a suitable configuration is available.
      • Run ruff if a ruff configuration is available, run flake8 otherwise (do not run flake8 if no .flake8 or setup.cfg configuration exists). This allows each package to define when it should switch to using ruff.
      • Add ruff to rubin-env (it must be available in the main build)
      • Modify the style guide to indicate that ruff MAY be used for linting (it currently says that flake8 MAY be used).

      Attachments

        Issue Links

          Activity

            tjenness Tim Jenness added a comment -

            It sounds like people either want scons to run flake8/ruff or don't really mind if it does or not. I think this leads to:

            • Remove the pytest flake8 option from every setup.cfg file. This has to be done as soon as possible since it blocks the update to flake8 6 (which supports multiprocessin on macOS).
            • Update the dev guide to indicate that ruff MAY be used in place of flake8.
            • Add ruff and flake8 v6 to rubin-env 7.0.0
            • When rubin-env 7 is released, change sconsUtils to run flake8 or ruff depending on the situation (or not run it at all if there is no flake8 configuration).

            ie explicitly link this change to 7.0 and require that we remove the flake8 pytest option before we go to 7.0.

            tjenness Tim Jenness added a comment - It sounds like people either want scons to run flake8/ruff or don't really mind if it does or not. I think this leads to: Remove the pytest flake8 option from every setup.cfg file. This has to be done as soon as possible since it blocks the update to flake8 6 (which supports multiprocessin on macOS). Update the dev guide to indicate that ruff MAY be used in place of flake8. Add ruff and flake8 v6 to rubin-env 7.0.0 When rubin-env 7 is released, change sconsUtils to run flake8 or ruff depending on the situation (or not run it at all if there is no flake8 configuration). ie explicitly link this change to 7.0 and require that we remove the flake8 pytest option before we go to 7.0.

            I think this all sounds good, I just wanted to note that I do think that scons should, in general, still run the linting one way or another though. I think that's roughly what you're saying, but just wanted to be clear that I'd really like to avoid a situation where you only get the linting run automatically on GitHub or via pre-commit. I don't think that's in conflict with what you're saying, but just wanted to be clear that I'm definitely not on team "don't mind if it does not" - I think that would be a regression and cause problems/frustration (and if there's ever an RFC about mandatory pre-commit everywhere enforcing linting on all branches then that's a big "please no" from me - it has to remain possible to push unlinted code somehow, though obviously I'm totally against merging it).

            mfisherlevine Merlin Fisher-Levine added a comment - I think this all sounds good, I just wanted to note that I do think that scons should, in general, still run the linting one way or another though. I think that's roughly what you're saying, but just wanted to be clear that I'd really like to avoid a situation where you only get the linting run automatically on GitHub or via pre-commit. I don't think that's in conflict with what you're saying, but just wanted to be clear that I'm definitely not on team "don't mind if it does not" - I think that would be a regression and cause problems/frustration (and if there's ever an RFC about mandatory pre-commit everywhere enforcing linting on all branches then that's a big "please no" from me - it has to remain possible to push unlinted code somehow, though obviously I'm totally against merging it).
            rowen Russell Owen added a comment -

            Sounds good. I assume this is understood, but if scons is going to run flake8/ruff, this should almost certainly be done by running pre-commit. Then we can drop pytest-flake8 and the other pytest linting plugins. (And we definitely need some kind of CI to run the linters, since developers may forget to init pre-commit).

            Also to speak to Merlin's concern about pre-commit blocking commits. For short-term work you can `git commit -n` to avoid running pre-commit. I find I need to do this occasionally. It is safe as long as CI also runs pre-commit, since you can't forget to clean things up before merging a PR.

            rowen Russell Owen added a comment - Sounds good. I assume this is understood, but if scons is going to run flake8/ruff, this should almost certainly be done by running pre-commit. Then we can drop pytest-flake8 and the other pytest linting plugins. (And we definitely need some kind of CI to run the linters, since developers may forget to init pre-commit). Also to speak to Merlin's concern about pre-commit blocking commits. For short-term work you can `git commit -n` to avoid running pre-commit. I find I need to do this occasionally. It is safe as long as CI also runs pre-commit, since you can't forget to clean things up before merging a PR.
            tjenness Tim Jenness added a comment -

            This RFC is not requiring pre-commit. It removes pytest-flake8.

            The implementation ticket on DM-38499 has scons run either ruff or flake8. It doesn't try to get pre-commit to run those linters. It runs them natively after pytest has completed.

            tjenness Tim Jenness added a comment - This RFC is not requiring pre-commit. It removes pytest-flake8. The implementation ticket on DM-38499 has scons run either ruff or flake8. It doesn't try to get pre-commit to run those linters. It runs them natively after pytest has completed.

            Adding to Russell's comments about pre-commit (sorry for dragging on about that): TSSW makes sure that the Jienkins jobs install pre-commit. Then there is a TSSW conda package that installs common pre-commit hook config files, which then get used by Jenkins in a pre-commit run --all call. That way, if a developer forgets to install and run pre-commit, any code issues will still be caught.

            wvreeven Wouter van Reeven added a comment - Adding to Russell's comments about pre-commit (sorry for dragging on about that): TSSW makes sure that the Jienkins jobs install pre-commit. Then there is a TSSW conda package that installs common pre-commit hook config files, which then get used by Jenkins in a pre-commit run --all call. That way, if a developer forgets to install and run pre-commit, any code issues will still be caught.

            People

              tjenness Tim Jenness
              tjenness Tim Jenness
              Arun Kannawadi, Eli Rykoff, Eric Bellm, Jim Bosch, John Parejko, Kian-Tat Lim, Merlin Fisher-Levine, Russell Owen, Tim Jenness, Wouter van Reeven
              Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                Jenkins

                  No builds found.