-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Show Flake8 errors in GitHub CI again #46990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
💊 CI failures summary and remediationsAs of commit a299030 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
flake8 should fail the PR IMO. doesn't make sense to check in code that doesn't pass linter. |
Agreed; so, this PR would make flake8 fail the PR (as it should), but it seems like it would also prevent the following "Add annotations" step from running, if flake8 fails. Is that what we want? |
Summary: In #46990 I asked whether the "Run flake8" step was supposed to always succeed (so that the "Add annotations" step would be sure to run). The reviewers and I weren't sure of the answer to that question, so we merged it anyway, but that turned out to be wrong: https://github.com/pytorch/pytorch/runs/1327599980 So this PR fixes that issue introduced by #46990. Pull Request resolved: #47236 Reviewed By: janeyx99 Differential Revision: D24692359 Pulled By: samestep fbshipit-source-id: c12382de6945245d6251ce792896e5e688f480af
Fixes #46985.
Question: Can someone comment on whether the "Run flake8" step should fail if
flake8
produces errors? This PR makes sure the errors are still shown, but the job linked from the issue also shows that the failure of that step seems to have caused the "Add annotations" step not to run.Is this what we want, or should I instead revert back to the
--exit-zero
behavior (in this case by just removing the-o pipefail
from this PR) that we had before #46740? And if the latter, then (how) should I modify thisflake8-py3
job to make sure it fails whenflake8
fails (assuming it didn't already do that?)