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

Run regression tests on nightlies (PyPI CPU, GPU. & Conda CPU, GPU) #2562

Merged
merged 51 commits into from
Sep 11, 2023

Conversation

agunapal
Copy link
Collaborator

@agunapal agunapal commented Aug 31, 2023

Description

This PR adds the following

  • Workflow to test TorchServe CPU pypi & conda binaries for Python 3.8, 3.9, 3.10
  • Workflow to test TorchServe GPU pypi & conda binaries for Python 3.8, 3.9, 3.10

Thanks to @udaij12 for helping me get conda working with the GPU runner
Fixes #(issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Feature/Issue validation/testing

CPU run : https://github.com/pytorch/serve/actions/runs/6132696896/job/16643849251?pr=2562
GPU run: https://github.com/pytorch/serve/actions/runs/6132974703/job/16645443959?pr=2562

CPU Conda

Mac Py 3.10 fails because we didn't have any binaries before.
6_regression-cpu-conda-nightly (macOS-latest, 3.10).txt
4_regression-cpu-conda-nightly (macOS-latest, 3.8).txt
1_regression-cpu-conda-nightly.txt
2_regression-cpu-conda-nightly (ubuntu-20.04, 3.9).txt
1_regression-cpu-conda-nightly (ubuntu-20.04, 3.8).txt
3_regression-cpu-conda-nightly (ubuntu-20.04, 3.10).txt
5_regression-cpu-conda-nightly (macOS-latest, 3.9).txt

CPU PyPI

4_regression-cpu-pypi-nightly (macOS-latest, 3.8).txt
3_regression-cpu-pypi-nightly (ubuntu-20.04, 3.10).txt
6_regression-cpu-pypi-nightly (macOS-latest, 3.10).txt
2_regression-cpu-pypi-nightly (ubuntu-20.04, 3.9).txt
1_regression-cpu-pypi-nightly (ubuntu-20.04, 3.8).txt
5_regression-cpu-pypi-nightly (macOS-latest, 3.9).txt

GPU PyPI

1_regression-gpu-pypi-nightly (3.8).txt
2_regression-gpu-pypi-nightly (3.9).txt
1_regression-gpu-pypi-nightly.txt
3_regression-gpu-pypi-nightly (3.10).txt

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #2562 (627f2d7) into master (5e30a7d) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 627f2d7 differs from pull request most recent head 20ce65e. Consider uploading reports for the commit 20ce65e to get more accurate results

@@            Coverage Diff             @@
##           master    #2562      +/-   ##
==========================================
- Coverage   70.87%   70.79%   -0.08%     
==========================================
  Files          83       83              
  Lines        3839     3839              
  Branches       58       58              
==========================================
- Hits         2721     2718       -3     
- Misses       1114     1117       +3     
  Partials        4        4              

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@agunapal agunapal changed the title (WIP) Run regression tests on nightlies Run regression tests on nightlies (PyPI CPU, GPU. & Conda CPU) Sep 1, 2023

# Run newman api tests
Copy link
Member

Choose a reason for hiding this comment

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

could you plz elaborate on the needed changes for the newman tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure i follow. There is no change to the tests. I am only changing how we install torchserve

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Git diff makes it seem like I have changed the tests

@agunapal agunapal changed the title Run regression tests on nightlies (PyPI CPU, GPU. & Conda CPU) Run regression tests on nightlies (PyPI CPU, GPU. & Conda CPU, GPU) Sep 9, 2023
@msaroufim
Copy link
Member

Hrmm the new code owner policy isnt' great considering i can't merge this unless Matthias checks the tests

@msaroufim msaroufim mentioned this pull request Sep 10, 2023
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.

Just a few nits, other than that LGTM


# Generate mar file
mg.generate_mars()
if binaries:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Can we not derive thats its binary if pypi or conda argument is given?

cmd = f"conda install -c pytorch-nightly torchserve torch-model-archiver torch-workflow-archiver"
else:
cmd = f"conda install -c pytorch torchserve torch-model-archiver torch-workflow-archiver"
try_and_handle(cmd, False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any value in running the command with try_and_handle here? The only exception it handles, its handling by throwing it again, and we do not use dry_run here, so why not call the command directly?

@agunapal agunapal added this pull request to the merge queue Sep 11, 2023
Merged via the queue into pytorch:master with commit 821f9bc Sep 11, 2023
11 of 12 checks passed
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