Skip to content

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented May 6, 2022

What does this PR do?

self-push.yml has lines like

run: cat reports/tests_torch_gpu_failures_short.txt

which should be

run: cat reports/tests_torch_gpu/failures_short.txt

Currently, we get errors like

cat: reports/tests_torch_multi_gpu_failures_short.txt: No such file or directory
Error: Process completed with exit code 1.

(see for example this job).

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 6, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Ccing @stas00 since he introduced this.

If this change is needed, it should also be done in the other GitHub action files, and also in the report part of the job? Not sure if they will push properly nested files in subdirectories.

@ydshieh
Copy link
Collaborator Author

ydshieh commented May 6, 2022

If this change is needed, it should also be done in the other GitHub action files,

The other occurrences are in self-nightly-scheduled.yml". I can update it too. Done

and also in the report part of the job? Not sure if they will push properly nested files in subdirectories.

I guess you are talking about files like notification_service_deprecated.py and notification_service.py. They are not using failures_short.txt.

Note that:

  • this PR doesn't change any artifact directory location
  • notification_service_deprecated.py does have issues with the report.
    • For example, "run_all_tests_tf_gpu_test_reports/[X].txt" should be run_all_tests_tf_gpu_test_reports/tests_torch_gpu/[X].txt
      • (if we don't change the artifact directory in self-push.yml)
    • I am still on the task of the self-push report format, which might still take some time.
      • But think I can already fix a few small things in the meantime.

Copy link
Contributor

@stas00 stas00 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing, @ydshieh

Clearly nobody was looking at the reports ;)

The original was correct, but when this new workflow was added it had this bug

@stas00 stas00 merged commit 351cdbd into huggingface:main May 6, 2022
@ydshieh ydshieh deleted the fix_push_ci_report_path branch May 6, 2022 14:53
elusenji pushed a commit to elusenji/transformers that referenced this pull request Jun 12, 2022
* fix report cat path

* fix report cat path

Co-authored-by: ydshieh <[email protected]>
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