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

docs: add disk usage / memory usage benchmark table #751

Merged
merged 17 commits into from
Jun 15, 2022
Merged

Conversation

ZiniuYu
Copy link
Member

@ZiniuYu ZiniuYu commented Jun 13, 2022

No description provided.

@ZiniuYu ZiniuYu linked an issue Jun 13, 2022 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #751 (d2d7be2) into main (4d069a8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #751   +/-   ##
=======================================
  Coverage   81.78%   81.78%           
=======================================
  Files          17       17           
  Lines        1208     1208           
=======================================
  Hits          988      988           
  Misses        220      220           
Flag Coverage Δ
cas 81.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96923f1...d2d7be2. Read the comment docs.

docs/user-guides/server.md Outdated Show resolved Hide resolved
@ZiniuYu ZiniuYu marked this pull request as ready for review June 13, 2022 09:10
@ZiniuYu ZiniuYu requested a review from numb3r3 June 13, 2022 09:15
Comment on lines 65 to 75
| Model | PyTorch | ONNX | TensorRT | Output Dimension | Disk Usage (MB) | Peak Memory Usage (GB) |
|----------------|---------|------|----------|------------------|-----------------|------------------------|
| RN50 | ✅ | ✅ | ✅ | 1024 | 256 | 4.25 |
| RN101 | ✅ | ✅ | ✅ | 512 | 292 | 4.28 |
| RN50x4 | ✅ | ✅ | ✅ | 640 | 422 | 6.92 |
| RN50x16 | ✅ | ✅ | ❌ | 768 | 661 | 13.01 |
| RN50x64 | ✅ | ✅ | ❌ | 1024 | 1382 | 20.26 |
| ViT-B/32 | ✅ | ✅ | ✅ | 512 | 351 | 2.19 |
| ViT-B/16 | ✅ | ✅ | ✅ | 512 | 354 | 3.90 |
| ViT-L/14 | ✅ | ✅ | ✅ | 768 | 933 | 5.38 |
| ViT-L/14-336px | ✅ | ✅ | ❌ | 768 | 934 | 11.36 |
Copy link
Member

Choose a reason for hiding this comment

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

what about VRAM? thats very important.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in fact the VRAM. Maybe change the title. Do we need both RAM and VRAM statistics?

Copy link
Member

@hanxiao hanxiao left a comment

Choose a reason for hiding this comment

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

need a column on VRAM

@ZiniuYu ZiniuYu requested a review from hanxiao June 13, 2022 11:54
Comment on lines 66 to 75
|----------------|---------|------|----------|------------------|-----------------|---------------------|---------------------|
| RN50 | ✅ | ✅ | ✅ | 1024 | 256 | 2.97 | 4.25 |
| RN101 | ✅ | ✅ | ✅ | 512 | 292 | 3.05 | 4.28 |
| RN50x4 | ✅ | ✅ | ✅ | 640 | 422 | 3.18 | 6.92 |
| RN50x16 | ✅ | ✅ | ❌ | 768 | 661 | 3.55 | 13.01 |
| RN50x64 | ✅ | ✅ | ❌ | 1024 | 1382 | 3.95 | 20.26 |
| ViT-B/32 | ✅ | ✅ | ✅ | 512 | 351 | 3.18 | 2.19 |
| ViT-B/16 | ✅ | ✅ | ✅ | 512 | 354 | 3.17 | 3.90 |
| ViT-L/14 | ✅ | ✅ | ✅ | 768 | 933 | 3.64 | 5.38 |
| ViT-L/14-336px | ✅ | ✅ | ❌ | 768 | 934 | 3.67 | 11.36 |
Copy link
Member

Choose a reason for hiding this comment

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

on RAM and vRAM, either the number is wrong or the column names are wrong. Please check again

Copy link
Member Author

Choose a reason for hiding this comment

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

I quickly reran the test and noticed that the RAM usage is under-calculated by ~20%. The table is updated

docs/user-guides/server.md Outdated Show resolved Hide resolved
| ViT-B/32 | ✅ | ✅ | ✅ | 512 | 351 | 3.66 | 2.19 |
| ViT-B/16 | ✅ | ✅ | ✅ | 512 | 354 | 3.58 | 3.90 |
| ViT-L/14 | ✅ | ✅ | ✅ | 768 | 933 | 4.11 | 5.38 |
| ViT-L/14-336px | ✅ | ✅ | ❌ | 768 | 934 | 5.07 | 11.36 |
Copy link
Member

Choose a reason for hiding this comment

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

image

This number is impossible. In general, I believe the whole column is wrong.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, the VRAM data is not wrong. I think the reason I got a large VRAM value is that I was using a mini-batch of size 256 in clip-server. If I stick to the default value, the VRAM indeed drops to ~4GB.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I should clarify on that in the docs

Copy link
Member

@hanxiao hanxiao Jun 14, 2022

Choose a reason for hiding this comment

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

No, you should clarify it in the benchmark docs.

You should use the default one for benchmark otherwise it is meaningless.

  • Imagine the reader install clip-as-service and start to use it in the first time. Your table do not explain how much VRAM they would expect when running it out-of-the-box. People read it, "oh, 11GB that much? oh it uses batch_size 256, then what is the default batch_size and what is the default cost?" see? your benchmark does not solve the first question that users ask. In fact, it raises two questions for the readers: what is the default_batch and how much does it cost by default?
  • Think you buy oranges in the grocery store. The price is always measured on the basic unit. One orange 1 usd, then people can derive a dozen of oranges is probably at 12$ or so. You don't often see that the shop only tells you the price of a dozen, and you have to infer it by yourself. which is basically what you are doing right now. You tell the reader the cost of batch_size 512, and user have to infer the vram cost of batch_size 8 (default, aka "the basic unit")

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Using a different batch size indeed introduces some confusion. Will update the table

@ZiniuYu ZiniuYu requested a review from hanxiao June 14, 2022 08:33
Copy link
Member

@hanxiao hanxiao left a comment

Choose a reason for hiding this comment

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

  • please fix as my comment
  • please resolve conflict

@ZiniuYu ZiniuYu force-pushed the docs_benchmark_table branch 2 times, most recently from 50d3484 to 18e8796 Compare June 15, 2022 03:18
@numb3r3
Copy link
Member

numb3r3 commented Jun 15, 2022

@ZiniuYu please fix the commit message issue.

Error: You have commit messages with errors
⧗   input: docs: RAM usage
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]
✖   found 1 problems, 0 warnings

docs/user-guides/server.md Outdated Show resolved Hide resolved
docs/user-guides/server.md Outdated Show resolved Hide resolved
@github-actions
Copy link

📝 Docs are deployed on https://ft-docs_benchmark_table--jina-docs.netlify.app 🎉

Copy link
Member

@hanxiao hanxiao left a comment

Choose a reason for hiding this comment

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

looks like a test failing

@ZiniuYu
Copy link
Member Author

ZiniuYu commented Jun 15, 2022

looks like a test failing

That's weird. Fixed by reran the failed one. Anyway, learned a lot of lessons from this task🥹

@numb3r3 numb3r3 merged commit 9d872f2 into main Jun 15, 2022
@numb3r3 numb3r3 deleted the docs_benchmark_table branch June 15, 2022 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

complete benchmark table
3 participants