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

[CI] generate separate report files as artifacts #7995

Merged
merged 13 commits into from
Oct 27, 2020

Conversation

stas00
Copy link
Contributor

@stas00 stas00 commented Oct 23, 2020

This PR solves #7887 to produce easier to use reports on CIs.

  • adds an optional --make_reports=id to pytest- e.g. --make_reports=examples. It then uses that id to generate report_{id}_{reports}.txt - this was needed since some jobs like on scheduled jobs have multiple pytest runs, so a unique string is required. W/o this new flag everything remains as is - i.e. no reports get generated

  • the generated reports are all saved under reports to simplify the upload and are at the moment (assuming id was tests):

    • report_tests_durations.txt
    • report_tests_errors.txt
    • report_tests_failures.txt
    • report_tests_passes.txt
    • report_tests_short_summary.txt
    • report_tests_stats.txt
    • report_tests_warnings.txt

    We no longer need any pytests flags to generate these - e.g. no need for -rA or -durations= - they are all done internally.

    The code itself is a bit of a hack, that borrows a lot of pytest internals - but that's a start - I will see if I can find a public API to accomplish the same later if this new functionality catches on. Actually, it's pretty safe since it calls the same report functions pytest uses, so it's unlikely to break.

  • added the reporting to:

    • CirlcleCI run_examples_torch and run_tests_torch jobs
    • GitHub workflow run_all_tests_torch_and_tf_gpu job. (this one generates 3 (!) groups of reports)

Once these are tested on master and the results are satisfactory, I will add this new functionality to the rest of the jobs.

This is what you want to review:

Fixes: #7887

@sshleifer, @sgugger, @LysandreJik

@sgugger
Copy link
Collaborator

sgugger commented Oct 23, 2020

This looks good but I only see test_output.txt in the artifacts, for some reason?

@stas00
Copy link
Contributor Author

stas00 commented Oct 23, 2020

you must be looking at the wrong job? As I said I only did it for one job at the moment - this one:
https://app.circleci.com/pipelines/github/huggingface/transformers/14359/workflows/b38c8e8c-2867-4366-a907-a202da9bc9ee/jobs/104410/steps

    ~/transformers/output.txt
    ~/transformers/tests_durations.txt
    ~/transformers/tests_failures.txt
    ~/transformers/tests_passes.txt
    ~/transformers/tests_short_summary.txt
    ~/transformers/tests_stats.txt

@sgugger
Copy link
Collaborator

sgugger commented Oct 23, 2020

Ah my bad, I miscklicked!

@stas00
Copy link
Contributor Author

stas00 commented Oct 25, 2020

@sshleifer or @sgugger - I configured github artifacts in self-push.yaml of this PR - would one of you be able to start that job for me as I have no perms to do so. Thank you very much!

I hope I did it right, I added:

    - name: test suite reports artifacts
      uses: actions/upload-artifact@v2
      with:
        name: tests_results
        path: tests_*

I'm not sure whether this should be path: ~/transformers/tests_* like it was on circle_ci config - it should pick it up from the cwd.

I currently added it only to run_tests_torch_and_tf_gpu - so in theory it should upload the reports to the workflow results.

For reference, the information on this setup is at this 2 pages:

@sshleifer
Copy link
Contributor

sshleifer commented Oct 25, 2020

I can't figure out how to run a github actions workflow against a branch. It looks good enough that we I'm happy to just acknowledge that this could break on merge, in which case we'd send a follow up PR.

@stas00
Copy link
Contributor Author

stas00 commented Oct 25, 2020

Thank you for trying, @sshleifer

Ah, it's not finished yet, merge-wise - it's very rough on edges.

  • I just want to figure out how to make the results available on github actions in parallel with
  • waiting on you guys to hear what reports do you want and which not before finalizing this.

Can you suggest a different way of testing this? This was your recommendation in first place - to test it on a PR branch - except I can't test it since I don't have permissions to access the runners. Surely there must be a way of testing this?

Alternatively, we could go as simple as creating a new github workflow job that simply runs a job of echo test > tests_1.txt; echo test2 > tests_2.txt and then uploads tests_* as an artifact and checking that it is what you want. It should just work, since the docs suggest that as an example. Once we know it's working then the rest is easy.

Earlier you were talking about some possible problems with this - something about the job being always successful, I can't find that comment - but I am pretty sure there is no such issue with the approach I implemented - where pytest generates all the report files and we don't need to do anything about its log parsing.

@sshleifer
Copy link
Contributor

sshleifer commented Oct 25, 2020

waiting on you guys to hear what reports do you want and which not before finalizing this.

Don't wait, just make a sensible choice that's easy to change. Lean towards fewer reports.

Can you suggest a different way of testing this?

I don't know a good way of testing github actions. act looks promising, but I've never used it. The issue is not permissions it is that github workflows, afaict, cannot be run against arbitrary branches. There is a "rerun all jobs" button, but it will just rerun on master. Would be incredibly valuable if you figured out how to test github actions locally.

Here is everything I can see for self-push at https://github.com/huggingface/transformers/actions/runs/326336555/workflow

image

@sgugger
Copy link
Collaborator

sgugger commented Oct 26, 2020

I agree with Sam that we can merge to test and iterate if the reports look wrong (as soon as we're sure that the circleCI part is good to go, which we can test on this PR). From what I understand, the PR adds a new job, so it does not break the existing ones/reports.

@stas00
Copy link
Contributor Author

stas00 commented Oct 26, 2020

I will work on completing this and we can put it in for one circle-ci and one github workflow and see how it goes - thank you for your feedback, @sshleifer and @sgugger

@stas00 stas00 changed the title [wip] [CI] better reports [CI] better reports Oct 27, 2020
@stas00 stas00 changed the title [CI] better reports [CI] generate separate report files as artifacts Oct 27, 2020
@stas00
Copy link
Contributor Author

stas00 commented Oct 27, 2020

This is good to merge.

@sgugger sgugger requested a review from LysandreJik October 27, 2020 11:32
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

This is incredibly cool! Thanks a lot @stas00!

@LysandreJik LysandreJik merged commit bfd5e37 into huggingface:master Oct 27, 2020
@stas00 stas00 deleted the better-reports branch October 27, 2020 16:32
fabiocapsouza pushed a commit to fabiocapsouza/transformers that referenced this pull request Nov 15, 2020
* better reports

* a whole bunch of reports in their own files

* clean up

* improvements

* github artifacts experiment

* style

* complete the report generator with multiple improvements/fixes

* fix

* save all reports under one dir to easy upload

* can remove temp failing tests

* doc fix

* some cleanup
fabiocapsouza added a commit to fabiocapsouza/transformers that referenced this pull request Nov 15, 2020
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.

Github actions: more readable/informative output
4 participants