Skip to content
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.

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

add ability to view multiple focused log lines at once #4637

Merged
merged 1 commit into from
Oct 17, 2023
Merged

add ability to view multiple focused log lines at once #4637

merged 1 commit into from
Oct 17, 2023

Conversation

PaliC
Copy link
Contributor

@PaliC PaliC commented Oct 13, 2023

add ability to view multiple focused log lines at once

Add view of multiple lines using rockset. Currently we just show the same error line (because there is only one in rockset). Currently, this is also hidden under a feature flag.

Screenshot 2023-10-13 at 2 28 46 PM

🤖 Generated by Copilot at a80550d

This pull request adds the log annotation feature to the test-infra web app, which allows users to rate the quality of the log output for a job and to see multiple failure lines for a job. It also updates the backend SQL queries and the frontend components to handle the new failureLines and failureLineNumbers properties for jobs, and fixes some minor issues in the code. The affected files include JobLinks.tsx, LogViewer.tsx, LogAnnotationToggle.tsx, types.ts, drciUtils.ts, searchUtils.ts, log_annotation/[repoOwner]/[repoName]/[annotation].ts, metrics.tsx, and several files in the rockset folder.


Stack created with Sapling. Best reviewed with ReviewStack.

@PaliC PaliC mentioned this pull request Oct 13, 2023
@vercel
Copy link

vercel bot commented Oct 13, 2023

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 Oct 17, 2023 8:57pm

@facebook-github-bot 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 Oct 13, 2023
@PaliC PaliC requested a review from huydhn October 13, 2023 21:46
PaliC added a commit that referenced this pull request Oct 13, 2023
add buttons to rate logs

Adds buttons hidden under a feature flag which gives us the ability to
rate logs
<img width="1444" alt="Screenshot 2023-10-11 at 4 55 55 PM"
src="https://github.com/pytorch/test-infra/assets/13758638/76748be5-44f7-4e51-8e72-d781a96e9ba3">

<!--
copilot:summary
-->
### <samp>🤖 Generated by Copilot at e97f54f</samp>

This pull request adds the feature to annotate the log rating of a job
in the `LogViewer` component. It introduces a new component
`LogAnnotationToggle` that allows users to select an annotation and
sends it to a new API handler `/api/log_annotation`. It also defines a
new type `LogAnnotation` and stores the annotation data in a DynamoDB
table.

---
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with
[ReviewStack](https://reviewstack.dev/pytorch/test-infra/pull/4629).
* #4637
* __->__ #4629

Co-authored-by: PaliC <>
@huydhn
Copy link
Contributor

huydhn commented Oct 13, 2023

If the lines look exactly the same (same line number) like in the examples, should we just dedup them and show only one?

@huydhn
Copy link
Contributor

huydhn commented Oct 14, 2023

Ouch, I got Application error: a client-side exception has occurred (see the browser console for more information). The error is something like TypeError: Cannot read properties of null (reading 'match'). Something is null or undefined.

@@ -21,8 +21,8 @@ export interface JobData extends BasicJobData {
logUrl?: string;
durationS?: number;
queueTimeS?: number;
failureLine?: string;
failureLineNumber?: number;
failureLines?: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a correct understanding that this is only a cosmetic change atm because the current log classifier will always return only one failure line? So this would be an array one just one string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep it's just cosmetic atm

failureLine?: string;
failureLineNumber?: number;
failureLines?: string[];
failureLineNumbers?: number[];
Copy link
Contributor

@huydhn huydhn Oct 16, 2023

Choose a reason for hiding this comment

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

I think we might want to consider updating RecentWorkflowsData as well for consistency. It also has a single failure_line string field.

For the context, this failure line field is pretty important because it now influences how Dr.CI and mergebot detects flaky failures (I search for similar failures using this field as the input)

A request I have for later is to make sure that the failures are sorted so that the last failure is at index 0 and the first failure is the last one in the list, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

May be it's better to do this in a separate PR, as this one is pretty big already.

AND (
failed_checks_count > 0
OR pending_checks_count > 0
-- gets percentage of total force merges, force merges with failures, and force merges without failures (impatient)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change expected? As I see lots of differences between them and the before version has more comments :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this just got updated, when I was updating everything in rockset. This should be fine.

Add view of multiple lines using rockset. Currently we just show the same error line (because there is only one in rockset). Currently, this is also hidden under a feature flag.

<img width="915" alt="Screenshot 2023-10-13 at 2 28 46 PM" src="https://github.com/pytorch/test-infra/assets/13758638/4652e132-6886-4c21-9ac4-f208cc4d1edd">

<!--
copilot:summary
-->
### <samp>🤖 Generated by Copilot at a80550d</samp>

This pull request adds the log annotation feature to the test-infra web app, which allows users to rate the quality of the log output for a job and to see multiple failure lines for a job. It also updates the backend SQL queries and the frontend components to handle the new failureLines and failureLineNumbers properties for jobs, and fixes some minor issues in the code. The affected files include `JobLinks.tsx`, `LogViewer.tsx`, `LogAnnotationToggle.tsx`, `types.ts`, `drciUtils.ts`, `searchUtils.ts`, `log_annotation/[repoOwner]/[repoName]/[annotation].ts`, `metrics.tsx`, and several files in the `rockset` folder.
@PaliC PaliC merged commit 9c4c9b7 into main Oct 17, 2023
4 of 6 checks passed
malfet added a commit that referenced this pull request Oct 17, 2023
@PaliC PaliC mentioned this pull request Oct 17, 2023
malfet added a commit that referenced this pull request Oct 17, 2023
@malfet malfet deleted the pr4637 branch October 18, 2023 04:37
huydhn added a commit that referenced this pull request Nov 14, 2023
This field is now called `failure_lines` after
#4637, which allows it to
capture multiple failures on the logs (from a new to-be-built log
classifier). Here is the struct definition
https://github.com/pytorch/test-infra/blob/main/torchci/lib/types.ts#L45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants