-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Serve] resnet50 benchmarking #29096
Conversation
bb2ae08
to
e6d066b
Compare
e6d066b
to
e4bb798
Compare
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.
Looks good to me, just had a few questions.
Do you mind adding a short description in a comment at the top of the benchmark.py? It could just be what you have in the PR description
|
||
async def fetch(session): | ||
async with session.get( | ||
"http://localhost:8000/", json=input_uris * int(data_size / len(input_uris)) |
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.
Looks like we're repeating the images here. Probably a dumb question but the inference doesn't do any caching of the results anywhere right? If it did, the benchmark wouldn't be correct
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.
yes, we don't do any caching. Caching results is not under this scope.
|
||
save_test_results( | ||
{test_name: result}, | ||
default_output_file="/tmp/serve_resent_benchmark.json", |
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 you know how the release test infra finds this file? It might have to be named /tmp/release_test_out.json
or use the env var TEST_OUTPUT_JSON
in order for the "fetch results" step to work, do you mind double checking 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.
+1 please use TEST_OUTPUT_JSON
and follow other files for the JSON schema.
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.
so it can be shown in our perf dashboard
5ff8f29
to
2f323dd
Compare
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.
should we send uris or should we send the image directly? what is this benchmark exactly built to test/evaluate?
) | ||
|
||
async def _get_tensor_from_img(self, uri: str): | ||
return await asyncio.coroutine(self.utils.prepare_input_from_uri)(uri) |
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.
if prepare_input_from_uri
is blocking, making them async won't help 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.
make sense
Test: CPU + GPU + Resnet performance I think it is more practical to download image and convert them to tensor inside the deployment code instead of passing image directly from http request. |
I see. if downloading is on the critical path, we should definite put them in s3 |
0f56c7d
to
a3e58c0
Compare
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.
Please see Archit's comment about TEST_OUTPUT_JSON
.
We can merge this after a successful release test demo run. |
(as a stretch goal, a smoke test would be preferred because so we can run it in CI as well: https://github.com/ray-project/ray/blob/master/release/BUILD#L5-L39) |
Can it be merged? |
a3e58c0
to
02e55eb
Compare
Lint failed |
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
56b2b23
to
7fd203a
Compare
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Sihan Wang <[email protected]>
Signed-off-by: Weichen Xu <[email protected]>
Signed-off-by: Sihan Wang [email protected]
Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.