Clarify and document commit choice for CI jobs #54967
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
💊 CI failures summary and remediationsAs of commit 19f7ed2 (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 1 ongoing upstream failure:These were probably caused by upstream breakages that are not fixed yet.
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. |
Contributor
|
@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This was referenced Mar 30, 2021
xuzhao9
reviewed
Mar 30, 2021
| - For CI jobs on GitHub Actions: | ||
| - If the PR was created using [`ghstack`](https://github.com/ezyang/ghstack), | ||
| choice **1** is used. | ||
| - Otherwise, choice **2** is used. |
Contributor
There was a problem hiding this comment.
Does this mean, for GitHub Actions CI jobs, even if I manually create a branch from 'viable/strict', the CI will still checkout and use master as the base? I guess this is because of current limitations of GHA?
Contributor
Author
There was a problem hiding this comment.
Yes, that is correct. Before this PR, our GitHub Actions jobs would kind of check out the PR tip (for the repo state), but would still get the workflow YAML files from the merge commit. This PR makes it more consistent. However, as documented, you still have the option to use ghstack if you want to base on viable/strict and not have the GitHub Actions CI merge with master.
seemethere
approved these changes
Mar 30, 2021
Member
seemethere
left a comment
seemethere
left a comment
There was a problem hiding this comment.
Love all of the documentation here, great job @samestep
| `master` is at commit `A`, and the branch for PR #42 (just a placeholder) is at | ||
| commit `B`: | ||
|
|
||
| ``` |
walterddr
approved these changes
Mar 30, 2021
Contributor
walterddr
left a comment
walterddr
left a comment
There was a problem hiding this comment.
looks good. it would be great if Dr.CI will comment on a GHA failure / Circle Failures and imply these behavior difference.
Contributor
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 30, 2021
Summary: *Context:* #53406 added a lint for trailing whitespace at the ends of lines. However, in order to pass FB-internal lints, that PR also had to normalize the trailing newlines in four of the files it touched. This PR adds an OSS lint to normalize trailing newlines. The changes to the following files (made in 54847d0) are the only manually-written parts of this PR: - `.github/workflows/lint.yml` - `mypy-strict.ini` - `tools/README.md` - `tools/test/test_trailing_newlines.py` - `tools/trailing_newlines.py` I would have liked to make this just a shell one-liner like the other three similar lints, but nothing I could find quite fit the bill. Specifically, all the answers I tried from the following Stack Overflow questions were far too slow (at least a minute and a half to run on this entire repository): - [How to detect file ends in newline?](https://stackoverflow.com/q/38746) - [How do I find files that do not end with a newline/linefeed?](https://stackoverflow.com/q/4631068) - [How to list all files in the Git index without newline at end of file](https://stackoverflow.com/q/27624800) - [Linux - check if there is an empty line at the end of a file [duplicate]](https://stackoverflow.com/q/34943632) - [git ensure newline at end of each file](https://stackoverflow.com/q/57770972) To avoid giving false positives during the few days after this PR is merged, we should probably only merge it after #54967. Pull Request resolved: #54737 Test Plan: Running the shell script from the "Ensure correct trailing newlines" step in the `quick-checks` job of `.github/workflows/lint.yml` should print no output and exit in a fraction of a second with a status of 0. That was not the case prior to this PR, as shown by this failing GHA workflow run on an earlier draft of this PR: - https://github.com/pytorch/pytorch/runs/2197446987?check_suite_focus=true In contrast, this run (after correcting the trailing newlines in this PR) succeeded: - https://github.com/pytorch/pytorch/pull/54737/checks?check_run_id=2197553241 To unit-test `tools/trailing_newlines.py` itself (this is run as part of our "Test tools" GitHub Actions workflow): ``` python tools/test/test_trailing_newlines.py ``` Reviewed By: malfet Differential Revision: D27409736 Pulled By: samestep fbshipit-source-id: 46f565227046b39f68349bbd5633105b2d2e9b19
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 6, 2021
Summary: Since #54967, our clang-tidy CI job has been giving warnings on files that PRs don't touch (see the screenshot below for an example). This PR should fix the issue by comparing against the merge-base of the `merge` commit with `master`, which is just `master` itself.  Pull Request resolved: #55318 Test Plan: CI. Reviewed By: janeyx99 Differential Revision: D27572553 Pulled By: samestep fbshipit-source-id: 9a833aaeecc2ab22462b3fa99fa3353490c3de85
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 9, 2021
Summary: This PR - adds a `tools/translate_annotations.py` script that - parses annotations into JSON using the regexes that we were previously passing to [`pytorch/add-annotations-github-action`](https://github.com/pytorch/add-annotations-github-action) and - uses `git diff-index` to translate the line numbers for those annotations from the PR `merge` onto the PR `head`, since (as of #54967) we now run CI on the former instead of the latter; - modifies the `flake8-py3` and `clang-tidy` jobs to use that script and thus upload JSON in their artifacts instead of raw text; and - modifies the "Add annotations" workflow to specify `mode: json` to allow it to use those preprocessed annotations. Depends on pytorch/add-annotations-github-action#18. Pull Request resolved: #55569 Test Plan: You can run the unit tests with this command: ``` python tools/test/test_translate_annotations.py ``` I also tested the entire system together in my personal sandbox repo. Reviewed By: malfet Differential Revision: D27662161 Pulled By: samestep fbshipit-source-id: ecca51b79b9cf00c90fd89f0d41d0c7b89d69c63
krshrimali
pushed a commit
to krshrimali/pytorch
that referenced
this pull request
May 19, 2021
Summary: This PR - adds a `tools/translate_annotations.py` script that - parses annotations into JSON using the regexes that we were previously passing to [`pytorch/add-annotations-github-action`](https://github.com/pytorch/add-annotations-github-action) and - uses `git diff-index` to translate the line numbers for those annotations from the PR `merge` onto the PR `head`, since (as of pytorch#54967) we now run CI on the former instead of the latter; - modifies the `flake8-py3` and `clang-tidy` jobs to use that script and thus upload JSON in their artifacts instead of raw text; and - modifies the "Add annotations" workflow to specify `mode: json` to allow it to use those preprocessed annotations. Depends on pytorch/add-annotations-github-action#18. Pull Request resolved: pytorch#55569 Test Plan: You can run the unit tests with this command: ``` python tools/test/test_translate_annotations.py ``` I also tested the entire system together in my personal sandbox repo. Reviewed By: malfet Differential Revision: D27662161 Pulled By: samestep fbshipit-source-id: ecca51b79b9cf00c90fd89f0d41d0c7b89d69c63
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PRs #53652 and #54693 attempted to increase the consistency of our choice of commit (head vs merge) for CI on PRs, and have so far been unsuccessful. This PR takes a less ambitious approach to the problem by clarifying the choice in one specific way (see the following paragraph) and documenting it in
CONTRIBUTING.md.In addition to documentation, this PR also removes the current behavior of our GHA jobs that checkout the PR tip instead of the merge commit. At first glance, this behavior seems to increase consistency (by eliminating the special-case for
ghstackPRs), but in reality, it actually just means that for non-ghstackPRs, the question "Which commit is used in CI?" has two answers instead of just one; see the description of #53652 for more details.Once merged, this PR will unblock other PRs that modify our GHA workflows in breaking ways, such as #54737 and #54974.
Test plan:
None.