#30404 closed enhancement (fixed)

Add pyright and pycodestyle checks as Github actions

Reported by: gh-tobiasdiez Owned by:
Priority: major Milestone: sage-9.3
Component: build Keywords: code style, lint
Cc: saraedum, mkoeppe, chapoton, gh-mjungmath, embray, slelievre, dimpase Merged in:
Authors: Tobias Diez Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 51828da (Commits, GitHub, GitLab) Commit: 51828da28a18b1c0d161f09a91417abd9f652088
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

Currently, the linters (pycodestyle, pyflakes, ...) run as plugins of patchbot. This has a few disadvantages: a) the lint results depend on the version of the tools installed by the owner of the patchbot b) it takes quite some time until the results are available and c) you need people to run the patchbot.

This ticket adds pyright, relint and pycodestyle checks as Github actions. They run really fast (about 2min), have reliable results as the version of the linters can be controlled and uses Github server instead of people's machines.

Although only those rules are activated that are also run by the patchbot, the pycodestyle and relint checks currently fail with a lot of errors. See https://github.com/tobiasdiez/sage/actions/runs/352364668. Thus, making these tests pass requires more work by the sage community.

What is left to do is to add the integration of the github action output to trac - #30877

Change History (24)

comment:1 Changed 13 months ago by saraedum

Option A: I think this is politically impossible. I know that some of the contributors here have (or at least had) strong feelings about github. That likely did not improve with Microsoft buying them.

Option B*: Every branch here is already mirrored on GitLab. I believe what's missing here is better docs: https://gitlab.com/sagemath/dev/trac

Option C*: You could add the linter to GitLab CI. If I remember embray's comments correctly it's really easy to write a trac plugin that shows the gitlab CI status here on trac.

comment:2 Changed 13 months ago by mkoeppe

Note there's also https://github.com/sagemath/sagetrac-mirror - but I don't know at what intervals it is mirrored.

comment:3 Changed 13 months ago by gh-tobiasdiez

Perfect @mkoeppe! In this case, the integration should be fairly straightforward and amounts to adding the adding a link to the badge, by

<a href="https://github.com/sagemath/sagetrac-mirror/actions?query=workflow%3ALint+branch%3BRANCH"><img src="https://github.com/sagemath/sagetrac-mirror/workflows/Lint/badge.svg?branch=BRANCH&event=push"/></a>

Giving something similar to

https://github.com/tobiasdiez/sage/workflows/Lint/badge.svg

with a link to https://github.com/tobiasdiez/sage/actions?query=workflow%3ALint+branch%3Apatch-2 (its sadly not possible to directly link to the workflow output)

This leaves the following todo list:

  • Restrict current github actions to master and develop branch
  • Activate gitub actions in sagetrac-mirror
  • Add the link to the badge to trac

I can do the first task. Could somebody else then do point 2 and 3?

comment:4 Changed 13 months ago by gh-tobiasdiez

  • Cc embray added

comment:5 Changed 13 months ago by mkoeppe

My suggestion is to first focus on local operation. For example, finding a place for the pycodestyle configuration in the Sage source tree (this should not be configured in .github/workflows/, nor in the patchbot sources!) -- perhaps this should go to src/tox.ini, and then provide corresponding documentation on how to run it and why. Next, change the patchbot so that it uses the configuration from that file. This will make sure that things don't fall out of sync.

comment:6 Changed 13 months ago by slelievre

  • Cc slelievre added
  • Keywords code style lint added
  • Type changed from PLEASE CHANGE to enhancement

comment:7 follow-up: Changed 13 months ago by gh-tobiasdiez

Moving the config to src/tox.ini is done in #30408.

Should I continue with the above plan and restrict the current github workflows to the master & develop branch?

comment:8 Changed 13 months ago by mkoeppe

Take a look at the other workflows from my portability tests. They are configured to run on every push to a tag and on every pull request. That's I think also the right granularity for the new workflow

comment:9 in reply to: ↑ 7 Changed 13 months ago by mkoeppe

Replying to gh-tobiasdiez:

Moving the config to src/tox.ini is done in #30408.

You could make #30408 a dependency of this ticket & merge in the branch

comment:10 Changed 13 months ago by gh-tobiasdiez

  • Branch set to public/build/lintGithub
  • Commit set to 2e88f614ec248e40ef76d76b61231f3c5f76af74
  • Dependencies set to #30408, #30361
  • Status changed from new to needs_review

I've now changed the workflow to use tox, which is enabled by #30408. Moreover, I realized based on the above comment by mkoeppe that the other Github actions are already restricted to only run on pushes with tags. Thus, they don't run on normal pushes to sagetrac-mirror, so no further action needed there.

They are configured to run on every push to a tag and on every pull request. That's I think also the right granularity for the new workflow

I think the lint workflow should run on every push and pull request. In this way, the developer gets immediate feedback, and this is similar to the patchbot that also runs on every "push". Moreover, they run very quickly so there is no problem in running them on every push.

So this leaves the following todo's.

  • Peer-review this ticket
  • Activate gitub actions in sagetrac-mirror
  • Merge the dependencies #30408, #30361
  • Double check that the workflow runs successful on sagetrac-mirror
  • Merge
  • Add the link to the badge to trac as described above

Afterwards, the workflow can be extended to include further linters. It might also be good to later add a basic build & doctest workflow that runs on every push.


New commits:

2e88f61Add lint workflow

comment:11 Changed 13 months ago by mkoeppe

  • Authors set to Tobias Diez
  • Status changed from needs_review to needs_work
  • Work issues set to merge dependencies

comment:12 Changed 13 months ago by gh-tobiasdiez

@mkoeppe, what should I do here? The dependencies are rather indirect (it works without #30361, it just tests more code). Moreover, this github action is currently nowhere invoked and hence not checked / tested....

comment:13 Changed 13 months ago by mkoeppe

Ticket description: "The pyright check currently fails, as there is no configuration file"

comment:14 Changed 13 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Yes, but even with the configuration files added the github workflow fails as there is no typing information yet(and the one added in #29775 is not complete) and pycodestyle reports over 600 errors as well.

Fixing these points requires more effort from the whole sage community.

The aim of this ticket is to add the github actions so that one has a consistent check, which at some point in the future hopefully turns green.

Last edited 13 months ago by gh-tobiasdiez (previous) (diff)

comment:15 Changed 12 months ago by gh-tobiasdiez

  • Status changed from needs_review to needs_work

Should invoke relint as well (added in #30467).

comment:16 Changed 12 months ago by mkoeppe

Could just run all tox environments...

comment:17 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:18 Changed 11 months ago by git

  • Commit changed from 2e88f614ec248e40ef76d76b61231f3c5f76af74 to 51828da28a18b1c0d161f09a91417abd9f652088

Branch pushed to git repo; I updated commit sha1. New commits:

e3751deMerge branch 'develop' of git://github.com/sagemath/sage into public/build/lintGithub
51828daAdd relint

comment:19 Changed 11 months ago by gh-tobiasdiez

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I've now added relint. This is now ready for review.

comment:20 Changed 11 months ago by mkoeppe

  • Cc dimpase added
  • Dependencies #30408, #30361 deleted
  • Description modified (diff)
  • Status changed from needs_review to positive_review
  • Work issues merge dependencies deleted

This seems to work well.

I have moved the part of the ticket description that describes Trac integration to the new ticket #30877. But actually, given that the new push mirror for Trac ended up using GitLab instead of GitHub (see #30736), you should probably look into creating GitLab pipelines for the same checks.

comment:21 Changed 11 months ago by gh-tobiasdiez

Thanks for the review!

I think you can make a good argument to have both Gitlab as well as Github as push mirrors.

comment:22 Changed 11 months ago by chapoton

reviewer name missing

comment:23 Changed 11 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe

comment:24 Changed 10 months ago by vbraun

  • Branch changed from public/build/lintGithub to 51828da28a18b1c0d161f09a91417abd9f652088
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.