Skip to content

Conversation

huydhn
Copy link
Contributor

@huydhn huydhn commented Jul 27, 2023

At the moment, we only record the list of pending and failed check on Rockset merge records. This is enough to compute the force merge KPI(s), but isn't enough for more in-depth analysis on what happened at the time of the merge:

  • If the number of ok_failed_checks is less than ok_failed_checks_threshold, the list of failed_checks would be empty (expectedly). So Rockset would only record an empty list.
  • We support retry in PR, so the classifications on Dr.CI could be different than what dev observed at the time of the merge if retry completed successfully

Testing

python .github/scripts/trymerge.py --comment-id 1654010315 106095 --dry-run (need to comment out some of the code to actually write a test record to Rockset), then manually verify it with

SELECT
    *
FROM
    commons.merges
WHERE
    pr_num = 106095

to see that ignore_current_checks, broken_trunk_checks, flaky_checks, and unstable_checks shows up correctly

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 27, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/106162

Note: Links to docs will display an error until the docs builds have been completed.

✅ 1 Unrelated Failure

As of commit 5bb86ce:

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@huydhn huydhn requested a review from clee2000 July 27, 2023 23:32
@huydhn huydhn marked this pull request as ready for review July 27, 2023 23:33
@huydhn huydhn requested a review from a team as a code owner July 27, 2023 23:33
@facebook-github-bot
Copy link
Contributor

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

@huydhn
Copy link
Contributor Author

huydhn commented Jul 27, 2023

As this touches find_matching_merge_rule, I have imported this to run internal tests (if any) just in case.

Copy link
Contributor

@clee2000 clee2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be easier to put a tuple of [job id, classification] for each job into rockset instead of separate lists for each classification?

@huydhn huydhn changed the title Record details about ic, broken, flaky, and unstable checks to merge records Add details about ic, broken, flaky, and unstable checks to merge records Jul 28, 2023
@huydhn
Copy link
Contributor Author

huydhn commented Jul 28, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 28, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@huydhn huydhn added the suppress-bc-linter Suppresses the failures of API backward-compatibility linter (Lint/bc_linter) label Jul 28, 2023
@malfet
Copy link
Contributor

malfet commented Jul 28, 2023

Forward fix in #106203, but I wonder how it escaped testing in the first place

@huydhn
Copy link
Contributor Author

huydhn commented Jul 28, 2023

Forward fix in #106203, but I wonder how it escaped testing in the first place

I should have known. It was a landrace with my own other change #106095 (ouch)

bobby-palmer pushed a commit to bobby-palmer/pytorch that referenced this pull request Jul 29, 2023
…ords (pytorch#106162)

At the moment, we only record the list of pending and failed check on Rockset merge records. This is enough to compute the force merge KPI(s), but isn't enough for more in-depth analysis on what happened at the time of the merge:

* If the number of `ok_failed_checks` is less than `ok_failed_checks_threshold`, the list of `failed_checks` would be empty (expectedly).  So Rockset would only record an empty list.
* We support retry in PR, so the classifications on Dr.CI could be different than what dev observed at the time of the merge if retry completed successfully

### Testing

`python .github/scripts/trymerge.py --comment-id 1654010315 106095 --dry-run` (need to comment out some of the code to actually write a test record to Rockset), then manually verify it with

```
SELECT
    *
FROM
    commons.merges
WHERE
    pr_num = 106095
```

to see that `ignore_current_checks`, `broken_trunk_checks`, `flaky_checks`, and `unstable_checks` shows up correctly
Pull Request resolved: pytorch#106162
Approved by: https://github.com/clee2000
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged suppress-bc-linter Suppresses the failures of API backward-compatibility linter (Lint/bc_linter) test-config/default topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants