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

Fix missing prev_ci_results #30313

Merged
merged 1 commit into from
Apr 18, 2024
Merged

Fix missing prev_ci_results #30313

merged 1 commit into from
Apr 18, 2024

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Apr 18, 2024

What does this PR do?

#29914 added a new step in slack report workflow, but it used actions/checkout@v4 after the first part (about model CI job). This indeed removes all files in the same directory, so the actions/checkout@v4 created in the first part is moved, and the last step of uploading artifact can't find it. This makes the new mdoel failures missing in the slack report channel.

@ydshieh ydshieh requested a review from SunMarc April 18, 2024 10:07
Comment on lines +60 to +68

# Upload complete failure tables, as they might be big and only truncated versions could be sent to Slack.
- name: Failure table artifacts
# Only the model testing job is concerned for this step
if: ${{ inputs.job == 'run_tests_gpu' }}
uses: actions/upload-artifact@v4
with:
name: prev_ci_results
path: prev_ci_results
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need to be before the second actions/checkout@v4 below

uses: actions/upload-artifact@v4
with:
name: prev_ci_results
path: prev_ci_results

- uses: actions/checkout@v4
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the killer

@ydshieh ydshieh requested a review from amyeroberts April 18, 2024 10:10
@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
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks for the fix !

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

@ydshieh ydshieh merged commit df96438 into main Apr 18, 2024
8 checks passed
@ydshieh ydshieh deleted the find_missing_new_failure_report branch April 18, 2024 14:10
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Apr 18, 2024
ArthurZucker pushed a commit that referenced this pull request Apr 22, 2024
ydshieh added a commit that referenced this pull request Apr 23, 2024
itazap pushed a commit that referenced this pull request May 14, 2024
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.

4 participants