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 skeleton codes for dynamic grpc client support #213

Draft
wants to merge 8 commits into
base: maxine-poc
Choose a base branch
from

Conversation

nv-hwoo
Copy link
Contributor

@nv-hwoo nv-hwoo commented Dec 10, 2024

This PR adds skeleton codes for dynamic grpc client support. It declares all (or most) of the necessary classes that client backend needs with empty definitions. The code successfully compiles but does not work yet. Next few PRs will fill in the contents of the class function bodies.

New classes added are:

  • DynamicGrpcClientBackend
  • DynamicGrpcInferRequestedOutput
  • DynamicGrpcInferInput
  • DynamicGrpcClient
  • DynamicGrpcRequest
  • DynamicGrpcInferResult

Also added

  • cmake script to build the dynamic grpc client backend
  • new backend type DYNAMIC_GRPC
  • placeholder function in ModelParser (e.g. InitDynamicGrpc)

@@ -0,0 +1,71 @@
# Copyright 2020-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2020-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
# Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

Comment on lines +56 to +58
target_link_libraries(
dgrpc-client-backend-library
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't necessary since you're not listing any libraries:

https://cmake.org/cmake/help/latest/command/target_link_libraries.html#overview

InferStat infer_stat_;

private:
Error PreRunProcessing(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this isn't inheriting from an interface, there's no technical need to have a function with this name.

I don't really understand the original use of this that's why I'm slightly pushing back on continuing its use.

@matthewkotila
Copy link
Contributor

(Accidental closure--reopened)

@dyastremsky
Copy link
Contributor

(Accidental closure--reopened)

Thanks for catching and fixing this, Matt! Apologies for the accidental closure.

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.

3 participants