-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Upload code coverage reports from different jobs, other CI improvements #5257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a couple of comments/suggestions.
.github/workflows/ci.yml
Outdated
mv coverage.xml coverage/ | ||
|
||
- name: Save coverage report | ||
if: matrix.python == '3.7' && github.repository == 'allenai/allennlp' && (github.event_name == 'push' || github.event_name == 'pull_request') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we fix 3.7 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, this was needed when these tests ran on multiple versions of Python, because we only needed one coverage report from this job.
.github/workflows/ci.yml
Outdated
pip freeze | ||
|
||
- name: Run black | ||
if: '! cancelled()' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this check necessary? Doesn't it always cancel all the checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed, it was only need when this check was in the other workflow that had a lot of different steps. And then it survived the copy-paste.
.github/workflows/ci.yml
Outdated
# Could run into issues with the cache if we don't uninstall the editable. | ||
# See https://github.com/pypa/pip/issues/4537. | ||
pip uninstall --yes allennlp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads like some hard-won wisdom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
.github/workflows/ci.yml
Outdated
- name: Debug info | ||
run: | | ||
ls -lh coverage | ||
ls -lh coverage/cpu_tests | ||
ls -lh coverage/gpu_tests | ||
ls -lh coverage/model_tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, I'll take out.
Co-authored-by: Akshita Bhagia <akshita23bhagia@gmail.com>
Closes #5253. This gives us a more accurate code coverage report now, showing 91% covered instead of 88%.
This also makes a few other improvements to CI. Altogether, the changes proposed are:
setup-python
action to work on the self-hosted runner. But I was able to get it to work now by following this comment.flake8
andmypy
checks to a separate job calledLint
black --check .
to a separate job calledStyle
..bulldozer.yml
config which we don't use anymore.