Skip to content

Conversation

@samestep
Copy link
Contributor

@samestep samestep commented Apr 22, 2021

This PR adds if: always() to all the steps in our "Lint / quick-checks" job that don't depend on any preceding steps. Technically means that the title of this PR isn't entirely correct, since the "Ensure correct trailing newlines" step depends on the "Setup Python" step and thus we don't always() run it even though it's grep-based.

Test plan:

On master, it looks like the only failing part of quick-checks is the trailing whitespace check:

However, the CI on this PR shows that the unqualified type ignore check is also failing:

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 22, 2021

💊 CI failures summary and remediations

As of commit 654b204 (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

1 failure not recognized by patterns:

Job Step Action
GitHub Actions quick-checks Ensure no trailing spaces 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

@samestep samestep requested a review from a team April 22, 2021 15:06
@samestep samestep marked this pull request as ready for review April 22, 2021 15:06
@facebook-github-bot
Copy link
Contributor

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@walterddr
Copy link
Contributor

bravo. explosed some more quick check errors. plz fix them in the same PR?

@samestep
Copy link
Contributor Author

plz fix them in the same PR?

@walterddr I'm not fixing them in this PR because they're already being fixed by #56692, which is landing right now.

@walterddr
Copy link
Contributor

walterddr commented Apr 22, 2021

plz fix them in the same PR?

@walterddr I'm not fixing them in this PR because they're already being fixed by #56692, which is landing right now.

sounds good! maybe rebase after #56692 landed to retrigger quick check ?

@samestep
Copy link
Contributor Author

maybe rebase after #56692 landed to retrigger quick check ?

I'm not sure what you mean; could you clarify? I'm landing this right now since it doesn't conflict with #56692.

@walterddr
Copy link
Contributor

maybe rebase after #56692 landed to retrigger quick check ?

I'm not sure what you mean; could you clarify? I'm landing this right now since it doesn't conflict with #56692.

But if you land this now. Wouldn't it broke linter on master?

@samestep
Copy link
Contributor Author

samestep commented Apr 22, 2021

But if you land this now. Wouldn't it broke linter on master?

Behold: https://github.com/pytorch/pytorch/runs/2411806242

@facebook-github-bot
Copy link
Contributor

@samestep merged this pull request in 3355c30.

@samestep samestep deleted the quick-checks-grep-always branch April 22, 2021 15:40
facebook-github-bot pushed a commit that referenced this pull request May 5, 2021
Summary:
This is essentially a continuation of #56700. Currently, some of the steps in **Lint / quick-checks** (such as the trailing newlines check) still don't always run if an earlier steps fail; example: https://github.com/pytorch/pytorch/runs/2504623867

This PR adds some more `if`s to remaining steps, so that they, too, can still run even when earlier steps fail.

Pull Request resolved: #57572

Test Plan:
- https://github.com/pytorch/pytorch/runs/2504706736 before this PR, many steps get skipped if an early step fails
- https://github.com/pytorch/pytorch/runs/2504778437 using this PR's technique, those steps still run
- https://github.com/pytorch/pytorch/runs/2504787234 if the requirements step doesn't run, steps still get skipped
- https://github.com/pytorch/pytorch/runs/2504796695 after this PR, `quick-checks` still succeeds

Reviewed By: driazati

Differential Revision: D28205900

Pulled By: samestep

fbshipit-source-id: bea856e15bdd17ee66e9ebba019ce91133b17bcd
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
Summary: Pull Request resolved: pytorch#56700

Reviewed By: walterddr

Differential Revision: D27940638

Pulled By: samestep

fbshipit-source-id: 54311ef45ec051ee29d934d501e83b3542bbb439
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
Summary:
This is essentially a continuation of pytorch#56700. Currently, some of the steps in **Lint / quick-checks** (such as the trailing newlines check) still don't always run if an earlier steps fail; example: https://github.com/pytorch/pytorch/runs/2504623867

This PR adds some more `if`s to remaining steps, so that they, too, can still run even when earlier steps fail.

Pull Request resolved: pytorch#57572

Test Plan:
- https://github.com/pytorch/pytorch/runs/2504706736 before this PR, many steps get skipped if an early step fails
- https://github.com/pytorch/pytorch/runs/2504778437 using this PR's technique, those steps still run
- https://github.com/pytorch/pytorch/runs/2504787234 if the requirements step doesn't run, steps still get skipped
- https://github.com/pytorch/pytorch/runs/2504796695 after this PR, `quick-checks` still succeeds

Reviewed By: driazati

Differential Revision: D28205900

Pulled By: samestep

fbshipit-source-id: bea856e15bdd17ee66e9ebba019ce91133b17bcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants