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

add resnet cpp handler #2514

Open
wants to merge 11 commits into
base: cpp_backend
Choose a base branch
from

Conversation

jagadeeshi2i
Copy link
Collaborator

Description

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

Please include a summary of the feature or issue being fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

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

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • Test A
    Logs for Test A

  • Test B
    Logs for Test B

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?

@jagadeeshi2i jagadeeshi2i requested review from lxning and chauhang August 2, 2023 19:14
Ubuntu and others added 3 commits August 3, 2023 15:55
Signed-off-by: jagadeesh <[email protected]>
jagadeesh added 5 commits August 18, 2023 10:27
Signed-off-by: jagadeesh <[email protected]>
Signed-off-by: jagadeesh <[email protected]>
Signed-off-by: jagadeesh <[email protected]>
@jagadeeshi2i jagadeeshi2i changed the title [WIP] add resnet cpp handler add resnet cpp handler Sep 14, 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.

Hi @jagadeeshi2i thanks for your contribution! Overall it looks very good, please address the few comments I left.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to you create a local CMakeLists.txt in the image_classifier folder and use add_subfolder().

target_include_directories(resnet-18_handler PUBLIC ${OPENCV_DIR})
target_include_directories(resnet-18_handler PUBLIC ${RESNET_SRC_DIR})
target_link_libraries(resnet-18_handler PRIVATE ts_backends_torch_scripted ts_utils ${TORCH_LIBRARIES})
target_link_libraries(resnet-18_handler PRIVATE "/usr/local/lib/libopencv_imgcodecs.so")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be better to use find_package and use the parameters set there instead of absolute paths.


try {
if (dtype_it->second == torchserve::PayloadType::kDATA_TYPE_BYTES) {
// case2: the image is sent as bytesarray
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these commented out lines still needed?


// Check if the image was successfully decoded
if (image.empty()) {
std::cerr << "Failed to decode the image.\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we continue in case the image was unsuccessfully loaded? What happens with the code below if image is empty?

gpuImage.download(resultImage);

// Create a tensor from the CPU Mat
torch::Tensor tensorImage = torch::from_blob(
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 a way to create the tensor on GPU? Avoiding the gpuImage.download?

const torch::Tensor& data,
std::pair<std::string&, std::map<uint8_t, std::string>&>& idx_to_req_id,
std::shared_ptr<torchserve::InferenceResponseBatch>& response_batch) {
std::ifstream jsonFile("index_to_name.json");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move loading of the file into the constructor to avoid reloading every time we do postprocessing?

- cleanup

Signed-off-by: jagadeesh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants