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

Mkulakow/mediapipe kfs rest test #2377

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

michalkulakowski
Copy link
Collaborator

@michalkulakowski michalkulakowski commented Mar 21, 2024

@michalkulakowski michalkulakowski force-pushed the mkulakow/mediapipe_kfs_rest_test branch 2 times, most recently from 84cb674 to c1c3491 Compare April 11, 2024 12:31
@michalkulakowski michalkulakowski added the WIP Do not merge until resolved label Apr 11, 2024
}
}

static void testInferenceNegative(int headerLength, std::string& request_body, std::unique_ptr<HttpRestApiHandler>& handler, ovms::Status status) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static void testInferenceNegative(int headerLength, std::string& request_body, std::unique_ptr<HttpRestApiHandler>& handler, ovms::Status status) {
static void testInferenceNegative(int headerLength, std::string& request_body, std::unique_ptr<HttpRestApiHandler>& handler, ovms::Status processorStatus) {

It's not worth at this point probably to add possibility for testing parsing components, but eventually we could add it later here as well.

@@ -0,0 +1,31 @@
#
# Copyright 2023 Intel Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2023 Intel Corporation
# Copyright 2024 Intel Corporation

class HttpRestApiHandlerWithMediapipeForkTest : public HttpRestApiHandlerWithMediapipe {
public:
void SetUp() {
SetUpServer("/ovms/src/test/mediapipe/config_summator.json");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we remove this json & pbtxt?

if (precision == ov::element::Type_t::string) {
expectedBytes = 0;
for (auto contents : request.inputs(inputIndex).contents().bytes_contents()) {
expectedBytes = expectedBytes + contents.size() + sizeof(uint32_t);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't size written in first bytes of the input? If so I suggest keeping the order also here.

Suggested change
expectedBytes = expectedBytes + contents.size() + sizeof(uint32_t);
expectedBytes += sizeof(uint32_t) + contents.size();

case ov::element::Type_t::string: {
uint32_t offset = 0;
for (auto contents : request.inputs(inputIndex).contents().bytes_contents()) {
uint32_t size = contents.size();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is used only below as a buffer to copy from. Do we need that? If so, can we make it const and replace following contents.size() calls?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be const?

@@ -83,6 +85,7 @@ const std::unordered_map<std::string, py::ssize_t> bufferFormatToItemsize{
{"e", 2},
{"f", 4},
{"d", 8},
{"s", 1},
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is it 1? BYTES type was considered custom type since in a way it's defined in KServe, it's impossible to determine size of a single element.

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wnarrowing"

TEST_F(HttpRestApiHandlerWithMediapipeForkTest, inferRequestFP32) {
TEST_F(HttpRestApiHandlerWithMediapipe, inferRequestFP64) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a lot of very similar tests here. Maybe we could parametrize some of them? The only differences I see are in datatype and status.

@michalkulakowski michalkulakowski removed the WIP Do not merge until resolved label Apr 16, 2024
@atobiszei atobiszei added this to the 2024.1_RC1 milestone Apr 17, 2024
@michalkulakowski michalkulakowski merged commit a300728 into main Apr 17, 2024
8 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.

None yet

3 participants