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

Include SAM vit_h in torch.compile nightly benchmark workflow and change model artifact name for SAM vit_b #2863

Merged
merged 12 commits into from
Dec 22, 2023

Conversation

sachanub
Copy link
Collaborator

@sachanub sachanub commented Dec 21, 2023

Description

Please read our CONTRIBUTING.md prior to creating your first pull request.

The objective of this PR is to include the SAM Fast model with weights corresponding to vit_h and a process batch size of 8. Also, this PR fixes the name of the SAM Fast vit_b model artifact.

The following TAR file contains the sam_fast_vit_b and sam_fast_vit_h MAR files: sam_fast_models.tar.gz

Testing:

  1. Temporarily uploaded sam_fast_vit_b_01ec64.mar and sam_fast_vit_h_4b8939.mar to https://torchserve.pytorch.org/mar_files/sam_fast_vit_b_01ec64.mar and https://torchserve.pytorch.org/mar_files/sam_fast_vit_h_4b8939.mar respectively for the benchmark workflow test.
  2. Executed torch.compile nightly benchmark workflow in the commit bee49d7.
  3. Output of successful benchmark workflow: https://github.com/pytorch/serve/actions/runs/7282252002/job/19844295086?pr=2863
  4. Report file with benchmark results: report.md
  5. Deleted the model artifacts from https://torchserve.pytorch.org/mar_files/sam_fast_vit_b_01ec64.mar and https://torchserve.pytorch.org/mar_files/sam_fast_vit_h_4b8939.mar until the PR is merged.

Steps after merging PR:

  1. Upload sam_fast_vit_b_01ec64.mar and sam_fast_vit_h_4b8939.mar to https://torchserve.pytorch.org/mar_files/sam_fast_vit_b_01ec64.mar and https://torchserve.pytorch.org/mar_files/sam_fast_vit_h_4b8939.mar.
  2. Modify torch.compile nightly GPU dashboard to reflect benchmark results for SAM Fast vit_b and SAM Fast vit_h.

UPDATE:

  1. Ran local benchmark test for SAM Fast vit_b and SAM vit_h models and identified that the best TPS and latency are achieved with a process batch size = 4 for both vit_b and vit_h models. Report: report.md
  2. Ran local benchmark test for SAM Fast vit_h and vit_b (process batch size 4) with concurrencies 1, 2, 4 and 8. Identified that the best latency is achieved with concurrency 4. Report: report_sam_fast.md
  3. Updated model artifacts for SAM vit_b and SAM vit_h to use process batch size = 4. Updated artifacts:
    sam_fast_models.tar.gz
  4. Changed name of benchmark model config YAML file to reflect that the settings correspond to the best latency case.

@sachanub sachanub changed the title Run benchmark test SAM vit_h and SAM vit_b Include SAM vit_h in torch.compile nightly benchmark workflow and change model artifact name for SAM vit_b Dec 21, 2023
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.

  1. what is the process batch size for the models?
  2. according to the setting, this PR is for latency test case. Please reflect it in the name of config.yaml

@sachanub
Copy link
Collaborator Author

  1. what is the process batch size for the models?
  2. according to the setting, this PR is for latency test case. Please reflect it in the name of config.yaml
  1. Currently, the process batch size for vit_h is 8 and the process batch size for vit_b is 4. However, we observed that in the case of vit_h, we can achieve higher TPS with process batch size 4. I will update the model artifact for vit_h to reflect process batch size 4.
  2. Running local tests for SAM Fast vit_h and vit_b (process batch size 4) with concurrencies 1, 2, 4 and 8. We can update the name of the config.yaml once we identify the optimal concurrency.

@sachanub sachanub added this pull request to the merge queue Dec 21, 2023
batch_size:
- 1
input: "./examples/large_models/segment_anything_fast/kitten.jpg"
requests: 1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are 1000 requests sufficient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that the TPS for vit_h is around 4 and the TPS for vit_b is around 5.2, I think 1000 requests should be sufficient. I have tried running the benchmark with a higher number of requests but the results were the same

Merged via the queue into master with commit 030ee8d Dec 22, 2023
13 checks passed
sachanub added a commit that referenced this pull request Dec 22, 2023
… and change model artifact name for SAM vit_b (#2863)"

This reverts commit 030ee8d.
@chauhang chauhang added this to the v0.10.0 milestone Feb 27, 2024
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.

None yet

4 participants