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

Added code-coverage.yml using codecov #1665

Merged
merged 26 commits into from
Jul 21, 2022
Merged

Added code-coverage.yml using codecov #1665

merged 26 commits into from
Jul 21, 2022

Conversation

msaroufim
Copy link
Member

@msaroufim msaroufim commented Jun 2, 2022

Description

This PR introduces code coverage using codecov https://about.codecov.io/

The code coverage results will show up as a github action showing the total coverage and how much coverage was added or lost in a PR. The action will show up as failing if coverage goes down in a PR. For now we've included

  • Tests to ts/
  • Test to workflow_archiver and model_archiver

Note that postman does not do code coverage, postman is a way to test APIs not code

Also deleted install_utils shell script since it's not used anywhere and has some remnant pytest related code that may be confusing later on

(base) ➜  serve git:(msaroufim-patch-13) ✗ grep -r "install_utils" .                      
(base) ➜  serve git:(msaroufim-patch-13) ✗

Fixes #1636

Next steps in future PRs

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing

Checklist:

  • Did you have fun?

@msaroufim msaroufim changed the title Create code-coverage.yml Create code-coverage.yml Jun 2, 2022
@msaroufim msaroufim mentioned this pull request Jun 2, 2022
16 tasks
@msaroufim msaroufim changed the title Create code-coverage.yml Added code-coverage.yml using codecov Jun 2, 2022
@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@46076f1). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 1debc0a differs from pull request most recent head 5b146e7. Consider uploading reports for the commit 5b146e7 to get more accurate results

@@            Coverage Diff            @@
##             master    #1665   +/-   ##
=========================================
  Coverage          ?   45.36%           
=========================================
  Files             ?       64           
  Lines             ?     2590           
  Branches          ?       60           
=========================================
  Hits              ?     1175           
  Misses            ?     1415           
  Partials          ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us.

@msaroufim
Copy link
Member Author

For example here is the coverage for ts/metrics

Screen Shot 2022-06-02 at 11 26 35 AM

@msaroufim
Copy link
Member Author

Checks will look like this
Screen Shot 2022-06-02 at 12 37 27 PM

@msaroufim
Copy link
Member Author

This is what the UI will look like to identify specific lines that were not covered
Screen Shot 2022-06-02 at 12 44 29 PM

@msaroufim
Copy link
Member Author

Hi @maaquib @lxning @HamidShojanazeri this PR is ready to review now

Copy link
Collaborator

@lxning lxning left a comment

Choose a reason for hiding this comment

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

Could you elaborate the problems of the existing code coverage report? Where do you set up code coverage threshold in CI job?

.github/workflows/code-coverage.yml Outdated Show resolved Hide resolved
ts_scripts/backend_utils.py Show resolved Hide resolved
ts_scripts/modelarchiver_utils.py Show resolved Hide resolved
ts_scripts/workflow_archiver_utils.py Show resolved Hide resolved
ts_scripts/modelarchiver_utils.py Show resolved Hide resolved
ts_scripts/modelarchiver_utils.py Show resolved Hide resolved
ts_scripts/workflow_archiver_utils.py Show resolved Hide resolved
ts_scripts/workflow_archiver_utils.py Show resolved Hide resolved
@msaroufim
Copy link
Member Author

msaroufim commented Jun 15, 2022

Thanks for the feedback @lxning

Could you elaborate the problems of the existing code coverage report?

  1. Generates an HTML report which isn't easy to render on remote machines unless you download the files
  2. Is not integrated into CI in an easy-to-use way with visible outputs for anyone to browse
  3. Is a point in time measurement and doesn't show you trends with coverage increasing or decreasing or provide aggregations to identify which folders need the most work to expand coverage
  4. Does not show relative improvements or decreases in coverage for a PR relative to master
  5. Not integrated with a centralized dashboard solution
  6. No easy way to aggregate all coverage reports into one large report

codevov solves all of the above, codecov is the default code coverage tool all pytorch projects use because the UX is fantastic. I would highly recommend you click on https://app.codecov.io/gh/pytorch/serve and browse the images above to get a feel for what makes it so nice

Where do you set up code coverage threshold in CI job?

This is easy to do with codecov.yaml https://docs.codecov.com/docs/codecov-yaml, I'll add a 45% threshold for now which we can revisit and increase over time after #1667 and #1670 are landed

@msaroufim msaroufim added this to in review in v0.6.1 lifecycle Jun 15, 2022
@msaroufim msaroufim added the p0 high priority label Jun 15, 2022
@msaroufim msaroufim self-assigned this Jun 15, 2022
@lxning
Copy link
Collaborator

lxning commented Jun 16, 2022

@msaroufim I think both ci-gpu and ci-cpu build should have code coverage. lint was already in the existing ci build.

@msaroufim
Copy link
Member Author

@msaroufim I think both ci-gpu and ci-cpu build should have code coverage.

Done

lint was already in the existing ci build.

It's not ideal to have linting in the CI pipeline where tests run since that takes ~20-30min to run whereas our new linting github action takes about 11s. The new black linting pipeline is also tightly integrated with pre-commit where you can setup a config files for all the linting rules you want with a great local UI. https://github.com/pytorch/serve/blob/msaroufim-patch-13/.pre-commit-config.yaml - it's also what torchx and torchdata use and it's a very popular way of doing linting in open source

@msaroufim msaroufim requested a review from mreso June 23, 2022 01:04
Copy link
Collaborator

@mreso mreso left a comment

Choose a reason for hiding this comment

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

LGTM

ts_scripts/backend_utils.py Outdated Show resolved Hide resolved
@msaroufim
Copy link
Member Author

@maaquib @lxning I believe we can go ahead and merge this change. As far as integration tests don't do, postman does not do code coverage since it only tests an API. The files in test/pytest are the next obvious candidate to improve code coverage once I also integrate them into Github Action

@msaroufim msaroufim merged commit 358e538 into master Jul 21, 2022
@msaroufim msaroufim deleted the msaroufim-patch-13 branch July 21, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p0 high priority
Projects
Development

Successfully merging this pull request may close these issues.

Add code coverage
5 participants