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

Aggeregate test summary files in CircleCI workflow runs #34989

Merged
merged 87 commits into from
Dec 16, 2024
Merged

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Nov 28, 2024

What does this PR do?

See the new collection_job outputs in the artifacts tab.

This could ease the searching if a test is run or not, run in which jobs, their status in each job etc.

@ydshieh ydshieh requested a review from ArthurZucker November 28, 2024 10:42
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Very nice work! A few suggestions

Comment on lines 49 to 50
{"run": """while [[ $(curl --location --request GET "https://circleci.com/api/v2/workflow/$CIRCLE_WORKFLOW_ID/job" --header "Circle-Token: $CCI_TOKEN"| jq -r '.items[]|select(.name != "collection_job")|.status' | grep -c "running") -gt 0 ]]; do sleep 5; done"""},
{"run": 'python utils/process_circleci_workflow_test_reports.py --workflow_id $CIRCLE_WORKFLOW_ID'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

In circle-ci can't we just use something like with or require? It will automatically wait for the run_test to run, and then fetch results?
(It could even be a github action no?)

Copy link
Collaborator Author

@ydshieh ydshieh Dec 2, 2024

Choose a reason for hiding this comment

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

Job 2 require Job 1: if job 1 failed, job 2 won't be triggered (sad 😢 ). when: always can't apply for jobs in a workflow (only steps in a job) for CircleCI.

That is why such wait is implemented (although I don't like it).

Copy link
Collaborator

@ArthurZucker ArthurZucker Dec 5, 2024

Choose a reason for hiding this comment

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

OK then you can merge but I would like for this test to not block merge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean if this new job fails, it shouldn't block the merge?

I will see if I can configure this 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am just doing

d14de3d

to make sure the run steps will always success.


job_test_summaries = {}
for artifact in job_artifacts:
if artifact["path"].startswith("reports/") and artifact["path"].endswith("/summary_short.txt"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using the junit.xlm will be more "foolproof" than regex parsing no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no regex in this script.

Regarding junit.xlm, yes probably more foolproof, but more post-process to be done I think:

  • xml's structure is more complex then summary_short.txt.
  • it doesn't give something like tests/generation/test_utils.py::GenerationIntegrationTests::test_generated_length_assisted_generation that we could copy-paste and run directly.

summary_short.txt is working well in a very simple way. If we find xlm is necessary in the future, happy to make the change however.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok sounds good!

@ydshieh ydshieh requested a review from ArthurZucker December 2, 2024 15:53
@ArthurZucker
Copy link
Collaborator

Let's fix the test to merge!

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks!

@ydshieh
Copy link
Collaborator Author

ydshieh commented Dec 13, 2024

will merge on Monday instead :-) Thanks for approval

@ydshieh ydshieh merged commit 66531a1 into main Dec 16, 2024
26 checks passed
@ydshieh ydshieh deleted the final_job branch December 16, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants