ENH: Lint checks for PR diffs #18423
Conversation
|
An example of a linting error being caught: https://dev.azure.com/ganesh3597/ganesh3597/_build/results?buildId=5&view=logs&j=a2bdd735-2cd0-5874-344f-2950fb45c949&t=43bcde9c-2634-50fc-b6ce-b6802de90ad4 |
|
Is there a reason to prefer github actions over azure pipelines for this? I find the summary presentation on github actions a little nicer and it is more tightly integrated with github. |
|
It was a very random choice from my side Matti :). Yeah, I'll see GitHub actions, looks cleaner 👍 . |
|
+1 Azure is really annoying, it takes a lot of clicking to get from the "Details" link in the PR view to the actual logs. |
My experience is the opposite, scrolling down the log to find the actual error is a lengthy process in GitHub actions. Azure does a nice job organizing the results. I'm hoping GitHub improves things going forward. |
This might be because I used a single composite sub-action |
|
I've added Github actions as well(in the process, will fix errors), we can choose one and discard other |
.github/workflows/build_test.yml
Outdated
| @@ -32,6 +32,21 @@ jobs: | |||
| python-version: ${{ env.PYTHON_VERSION }} | |||
| - uses: ./.github/actions | |||
|
|
|||
| lint: | |||
| needs: smoke_test | |||
There was a problem hiding this comment.
I think we can run lint irrespective of smoke_test?
There was a problem hiding this comment.
Well, do we want a lint failure to fail the whole build or just provide one more data point about the PR? On PyTorch they choose to end the CI early since anyway the submitter will have to fix the PR, so the whole CI will have to run again. That perspective would make me think it should run first, and everything else be conditional on it. On the otherhand, the CI might fail for other reasons so the linter is just icing on the cake, in which case it should not fail the whole CI run. I think I have a slight preference for the former: make it the reverse of this:
smoke_test:
needs: lint
What do others think?
There was a problem hiding this comment.
To have an opinion, how about we run both and only continue if both succeed? That way, if you look at the PR you know whether there are clearly issues beyond style. And the smoke-test run doesn't take a huge amount of resources for NumPy.
The argument of rerunning CI is good though, even if it might be mildly annoying sometimes to get blocked by.
There was a problem hiding this comment.
I added lint and smoke as mandatory, making the summary look like this.
There was a problem hiding this comment.
What would be nice is to group jobs and call it say preflight, and add preflight as needs instead of adding lint+smoke every time.
There was a problem hiding this comment.
I like this. I don't have an opinion on actions vs. azure, but either version seems like a good addition in any case.
There was a problem hiding this comment.
I like the idea of running it on both Azure and GH Actions, as the PR does now.
The preflight thing is a nice cleanup, but it's not essential and can be done separately.
9f5871a to
898ee74
Compare
a2df2d5 to
5eb7c8e
Compare
|
Sorry for bombarding with force pushes, not able to check errors in my fork. |
|
Shall I add this check as an option in |
That sounds like a good idea to me. |
|
Forgot to add |
ee21636 to
fb43f15
Compare
|
This accumulated a merge conflict now, would you mind rebasing @ganesh-k13? Do you think it's good to merge? |
|
I have fixed it Ralf. I hope the merge-base logic is fine, other than that I think we are good to ship it. |
|
Sorry for the strange commit meta data, on a different PC :( , will fix by tonight when I get back. |
There was a problem hiding this comment.
LGTM. Tested locally, and all works as expected. In it goes. Thanks @ganesh-k13! And thanks everyone else for reviews.
|
Testing in gh-18566, and I think fixing a leftover |
|
I also think we have to handle this error: https://github.com/numpy/numpy/runs/2051142496#step:5:1. This happens due to numpy/.github/workflows/build_test.yml Line 38 in b88ab10 this line where: ${{ github.base_ref }} is empty
[EDIT] same is true for Azure: https://dev.azure.com/numpy/numpy/_build/results?buildId=16027&view=logs&j=a2bdd735-2cd0-5874-344f-2950fb45c949&t=43bcde9c-2634-50fc-b6ce-b6802de90ad4&l=11 |
|
Ah yes, the CI changes in this PR work for other PRs, but not for merge commits on master. |
DOC: Added documentation for linter (#18423)
Lint checks for PR diffs
This PR aims to integrate the linting checks on diffs to the CI pipeline.
Changes
Usage:
Just script:
With runtest:
Other todo items
Based on scipy/scipy#11423
cc: @rgommers @seberg