[ch] Dr. CI to use CH Part 1 #5634

clee2000 · 2024-09-09T21:51:16Z

Moves most queries used by Dr. CI to CH. There are some one off queries that I still need to track down

types changed: the CH query doesn't have nulls, so I changed null checks to check for the corresponding default value ("" for string, 0 for int, time 0 for timestamps) and removed a lot of !. This causes a mild conflict with the JobData struct, so there's some casting that I don't think actually does anything. I'm also not sure how accurate the typing was originally, since I think some ids were int before too
- is there a runtime type checker for typescript?
- I tried and failed to add a linter, like https://typescript-eslint.io/rules/strict-boolean-expressions/ to make sure the null/existence checks were correct. It would be nice to figure this out
CH returns timestamps without the "Z" timezone, but they are all in UTC, so use dayjs.utc to make sure it is correct (dayjs uses local timezone by default)

I checked the outputs by cherry picking csl/comp_orig (4db3fa5) onto this branch and origin/main (origin/main also needs 055a919 to fix a bug), running curl curl "http://localhost:3000/api/drci/drci" --data 'repo=pytorch' > ../_logs/<new, orig>.json, running a script



import json
from library import REPO_ROOT
import re

def failures(thing):
    thing = thing["failures"]
    for pr_num in thing:
        for classification in thing[pr_num]:
            thing[pr_num][classification] = sorted(thing[pr_num][classification], key=lambda x: x["id"])
            for failure in thing[pr_num][classification]:
                if failure["failure_captures"] == None:
                    failure["failure_captures"] = []
                if failure["failure_context"] == None:
                    failure["failure_context"] = []
                if failure["failure_lines"] == None:
                    failure["failure_lines"] = []
                if failure["runnerName"] == None:
                    failure["runnerName"] = ""
                for key, value in failure.items():
                    date_match = re.match(r"(.*) (.*)\.000000000", str(value))
                    if date_match:
                        failure[key] = f"date"
                    date_match_2 = re.match(r"(\d\d\d\d-\d\d-\d\d)T(\d\d:\d\d:\d\d).*", str(value))
                    if date_match_2:
                        failure[key] = f"date"
    return thing


def comments(thing):
    thing = thing["comments"]
    s = "\n"
    for pr_num in sorted(thing.keys()):
        s += f"{pr_num}\n"
        s += f"{thing[pr_num]['new']}\n"
        s += f"{thing[pr_num]['body']}\n"
    return s

if __name__ == "__main__":
    orig_file = REPO_ROOT / "../test-infra/_logs/orig.json"
    new_file = REPO_ROOT / "../test-infra/_logs/new.json"

    with open(orig_file, "r") as f:
        orig = json.load(f)

    with open(new_file, "r") as f:
        new = json.load(f)

    o_thing = failures(orig)
    n_thing = failures(new)

    with open(orig_file, "w") as f:
        json.dump(o_thing, f, indent=2, sort_keys=True)
        print(comments(orig), file=f)

    with open(new_file, "w") as f:
        json.dump(n_thing, f, indent=2, sort_keys=True)
        print(comments(new), file=f)

and the diffing the files

There were small differences which I then checked individually, and they were due to the databases not being exactly the same when I did the curls. There were also some small differences in which disable issues were used, but based on the code for that I think it's still technically correct

vercel · 2024-09-09T21:51:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
torchci	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 11, 2024 8:21pm

clee2000 · 2024-09-10T16:42:28Z

torchci/lib/drciUtils.ts

+    acc[row.pr_num].push(row.merge_commit_sha);
+    return acc;
+  }, new Map<number, string[]>());
+}


Moved this function here from jobUtils.ts because it imports from clickhouse.ts file

clee2000 · 2024-09-10T16:42:51Z

torchci/lib/fetchIssuesByLabel.ts

+    label,
+  });
+}
+


split up functions since I want Dr. CI to use CH, but hud to still use rockset for now, and the gating by environment variable doesn't work for that

clee2000 · 2024-09-10T16:45:21Z

torchci/pages/api/drci/drci.ts

+    OWNER,
+    repo,
+    Array.from(workflowsByPR.keys())
+  );


Move this query to be out of the loop so its one big query instead of one query for each PR

huydhn · 2024-09-11T16:39:48Z

torchci/clickhouse_queries/recent_pr_workflows_query/query.sql

@@ -81,3 +89,5 @@ WHERE
  w.id in (select id from materialized_views.workflow_run_by_head_sha where head_sha in (select sha from recent_prs))
 ORDER BY
  time DESC
+-- Non experimental analyzer has problems with final...


Just a thought, the need to add final into all the different places seems easy to miss. Maybe we should adopt what @kit1980 has been doing in https://github.com/pytorch/test-infra/blob/main/torchci/clickhouse_queries/queued_jobs_by_label/query.sql#L48 by setting final to true for the whole query?

Maybe when we get a linter working in CI

Yeah, a linter for this would be good

huydhn · 2024-09-11T16:43:37Z

torchci/lib/clickhouse.ts

@@ -1,5 +1,12 @@
+// This file can't be imported by .tsx files due to client/server stuff (ex not


This can only be used on the server side, i.e. API. All HUD pages should query the API https://github.com/pytorch/test-infra/tree/main/torchci/pages/api/clickhouse to get the data instead. This is the same reason I need to add the toggle for CH/Rockset on HUD metric page

Maybe something that can be done by a linter?

huydhn · 2024-09-11T16:44:51Z

torchci/lib/clickhouse.ts

+// Import itself to ensure that mocks can be applied, see
+// https://stackoverflow.com/questions/51900413/jest-mock-function-doesnt-work-while-it-was-called-in-the-other-function
+// https://stackoverflow.com/questions/45111198/how-to-mock-functions-in-the-same-module-using-jest
+import * as thisModule from "./clickhouse";


TIL! Previously, I need to refactor the code to move the functions I want to mock to another module instead

torchci/lib/drciUtils.ts

torchci/lib/fetchRecentWorkflows.ts

huydhn

There are some minors comments but overall LGTM!

Follow up to #5634. These are the one off queries that I needed to track down. Surprisingly, none of the current PRs go through this path, so I used some older PRs, like 135397, 114772, as tests

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 9, 2024

clee2000 force-pushed the csl/drci_ch branch from c114332 to 47d6a09 Compare September 9, 2024 21:52

vercel bot had a problem deploying to Preview September 9, 2024 21:52 Failure

tc

f2326f2

clee2000 force-pushed the csl/drci_ch branch from 47d6a09 to b21443c Compare September 9, 2024 22:11

vercel bot had a problem deploying to Preview September 9, 2024 22:12 Failure

tc

d74df70

clee2000 force-pushed the csl/drci_ch branch from b21443c to d74df70 Compare September 9, 2024 22:21

vercel bot deployed to Preview September 9, 2024 22:22 View deployment

tests

beef0a4

vercel bot deployed to Preview September 10, 2024 16:26 View deployment

more test

6efab8b

vercel bot deployed to Preview September 10, 2024 16:36 View deployment

clee2000 commented Sep 10, 2024

View reviewed changes

clee2000 marked this pull request as ready for review September 10, 2024 16:59

clee2000 requested a review from a team September 10, 2024 17:00

clee2000 changed the title ~~[ch] Dr. CI to use CH~~ [ch] Dr. CI to use CH Part 1 Sep 10, 2024

clee2000 mentioned this pull request Sep 10, 2024

[ch] Dr. CI to use CH Part 2 #5637

Merged

huydhn reviewed Sep 11, 2024

View reviewed changes

torchci/lib/drciUtils.ts Outdated Show resolved Hide resolved

huydhn reviewed Sep 11, 2024

View reviewed changes

torchci/lib/drciUtils.ts Outdated Show resolved Hide resolved

huydhn reviewed Sep 11, 2024

View reviewed changes

torchci/lib/fetchRecentWorkflows.ts Outdated Show resolved Hide resolved

huydhn approved these changes Sep 11, 2024

View reviewed changes

clee2000 added 2 commits September 11, 2024 13:05

re comments

fb30a57

random comment

d8ba61f

vercel bot deployed to Preview September 11, 2024 20:21 View deployment

clee2000 merged commit eb5fda4 into main Sep 11, 2024
7 checks passed

This was referenced Sep 11, 2024

[HUD] add percentage for linux rollover #5643

Merged

Run yarn format #5657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ch] Dr. CI to use CH Part 1 #5634

[ch] Dr. CI to use CH Part 1 #5634

clee2000 commented Sep 9, 2024 •

edited

Loading

vercel bot commented Sep 9, 2024 •

edited

Loading

clee2000 Sep 10, 2024

clee2000 Sep 10, 2024

clee2000 Sep 10, 2024

huydhn Sep 11, 2024

clee2000 Sep 11, 2024

huydhn Sep 11, 2024

huydhn Sep 11, 2024

clee2000 Sep 11, 2024

huydhn Sep 11, 2024 •

edited

Loading

huydhn left a comment

		@@ -1,5 +1,12 @@
		// This file can't be imported by .tsx files due to client/server stuff (ex not

[ch] Dr. CI to use CH Part 1 #5634

[ch] Dr. CI to use CH Part 1 #5634

Conversation

clee2000 commented Sep 9, 2024 • edited Loading

vercel bot commented Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huydhn Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

huydhn left a comment

Choose a reason for hiding this comment

clee2000 commented Sep 9, 2024 •

edited

Loading

vercel bot commented Sep 9, 2024 •

edited

Loading

huydhn Sep 11, 2024 •

edited

Loading