Skip to content

[file report] Lots of changes, mainly due to using new CH table #7480

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Nov 19, 2025
Merged

[file report] Lots of changes, mainly due to using new CH table #7480

merged 11 commits into from
Nov 19, 2025

Conversation

@clee2000
Copy link
Contributor

@clee2000 clee2000 commented Nov 17, 2025 •
edited
Loading

https://torchci-git-csl-filereportusech-fbopensource.vercel.app/tests/fileReport

  • use commit timeline instead of list:

old
image

new
image

  • status change api uses clickhouse instead of pre aggregated files
  • weekly report generator uses clickhouse instead of s3 files
  • diff view has a commit selector instead of 1 row per commit pair

old
image
new
image

  • remove overview table since the diff view is probably better
  • add start date and end date picker for new api

old
image

new
image

  • new api for getting file report info based on the new weekly report generator
  • remove the group by options in the graph (confusing for periodic)
  • remove the old way to fetch data

@vercel
Copy link

vercel bot commented Nov 17, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Updated (UTC)
torchci Ignored Ignored Preview Nov 19, 2025 4:33am

@meta-cla meta-cla 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 Nov 17, 2025
@clee2000 clee2000 marked this pull request as ready for review November 17, 2025 23:14
@clee2000 clee2000 force-pushed the csl/file_report_use_ch branch from 8a28466 to 28edea3 Compare November 17, 2025 23:15
@clee2000 clee2000 changed the title [file report] Lots of changes, mainly using new CH table [file report] Lots of changes, mainly due to using new CH table Nov 18, 2025
@clee2000 clee2000 requested review from huydhn and yangw-dev November 18, 2025 17:51
@clee2000 clee2000 force-pushed the csl/file_report_use_ch branch from d33d535 to 89fd3d2 Compare November 18, 2025 17:52
@clee2000 clee2000 force-pushed the csl/file_report_use_ch branch from 89fd3d2 to 6d68e1a Compare November 18, 2025 19:40
yangw-dev
yangw-dev reviewed Nov 19, 2025
View reviewed changes
tools/torchci/test_insights/daily_regression.py Outdated
self, start_date, stop_date
) -> list[dict[str, Any]]:
response = requests.get(
f"http://localhost:3000/api/flaky-tests/fileReport?startDate={start_date}&endDate={stop_date}"
Copy link
Contributor

@yangw-dev yangw-dev Nov 19, 2025

Choose a reason for hiding this comment

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

nit: http://localhost:3000

Copy link
Contributor Author

@clee2000 clee2000 Nov 19, 2025

Choose a reason for hiding this comment

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

oops, swapped to https://hud.pytorch.org

yangw-dev
yangw-dev reviewed Nov 19, 2025
View reviewed changes
torchci/pages/api/flaky-tests/statusChanges.ts
invoking_file,
classname,
name
LIMIT 200
Copy link
Contributor

@yangw-dev yangw-dev Nov 19, 2025

Choose a reason for hiding this comment

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

[optional]do you want to do pagination token for the api? see the example
the api returns next_token if there are more

the api:
list_regression_summary_reports

the ui code
https://github.com/pytorch/test-infra/blob/main/torchci/components/benchmark_v3/components/benchmarkRegressionReport/listView/RegressionReportListView.tsx#L30

Copy link
Contributor Author

@clee2000 clee2000 Nov 19, 2025

Choose a reason for hiding this comment

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

I'll try that in a different PR, thanks for the references

yangw-dev
yangw-dev approved these changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@yangw-dev yangw-dev left a comment

Choose a reason for hiding this comment

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

LGTM, left some comments

yangw-dev
yangw-dev reviewed Nov 19, 2025
View reviewed changes
torchci/clickhouse_queries/tests/test_status_counts_on_commits_by_file/query.sql
select
distinct
id,
regexp_replace(
Copy link
Contributor

@yangw-dev yangw-dev Nov 19, 2025

Choose a reason for hiding this comment

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

this might make the query slow, just in case.

Copy link
Contributor Author

@clee2000 clee2000 Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's probably worth it to do the regexp replace here instead of later due to the grouping. This might be better as a materialized column in the table though, I imagine it could be useful in other places too

huydhn
huydhn reviewed Nov 19, 2025
View reviewed changes
torchci/clickhouse_queries/tests/test_status_counts_on_commits_by_file/query.sql
replaceAll(invoking_file, '.', '/') as invoking_file,
all_test_runs.name as name,
classname,
multiIf(
Copy link
Contributor

@huydhn huydhn Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a cancelled_count here that we need to handle?

Copy link
Contributor Author

@clee2000 clee2000 Nov 19, 2025

Choose a reason for hiding this comment

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

I think cancelled is more of a job conclusion rather than a test conclusion

huydhn
huydhn approved these changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@huydhn huydhn left a comment

Choose a reason for hiding this comment

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

LGTM! Mainly review the SQL file :)

@clee2000 clee2000 force-pushed the csl/file_report_use_ch branch from 1a6e89e to 8f6683b Compare November 19, 2025 04:33
@clee2000 clee2000 merged commit 014121b into main Nov 19, 2025
7 checks passed
@clee2000 clee2000 deleted the csl/file_report_use_ch branch November 19, 2025 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yangw-dev yangw-dev yangw-dev approved these changes

@huydhn huydhn huydhn approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@clee2000 @huydhn @yangw-dev