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

[FEA] Ensure all message classes have a C++ impl #70

Closed
dagardner-nv opened this issue Apr 28, 2022 · 3 comments
Closed

[FEA] Ensure all message classes have a C++ impl #70

dagardner-nv opened this issue Apr 28, 2022 · 3 comments
Assignees
Labels
feature request New feature or request

Comments

@dagardner-nv
Copy link
Contributor

Is your feature request related to a problem? Please describe.
This would allow pipelines like Hammah, where some stages have C++ impls, to run in a hybrid mode.

Describe the solution you'd like
There are a few message classes which don't have C++ impls but inherit from a message class that has a C++ impl. Typically these only add a few attributes.

Describe alternatives you've considered
Options would be:

  1. Just add the new C++ message classes (I have some of them in an old branch).
  2. Investigate pybind11 trampoline classes
@dagardner-nv dagardner-nv added the feature request New feature or request label Apr 28, 2022
@dagardner-nv dagardner-nv self-assigned this Apr 28, 2022
@cwharris
Copy link
Contributor

What if we just had a limited number of message types, and those messages types were simply data frames (for batch messages) or a single row (for non-batched types), and we had an associated list of available attributes on those messages? So rather than a hierarchy of messages classes, each stage would get it's own ingress and egress manifest, where each row must conform to the manifest? essentially we type-erase the message itself, and the associated manifest is the source of truth for type checking. This limits us to "has column with type" sorts of type checks, but that's probably good enough. Thoughts?

@dagardner-nv
Copy link
Contributor Author

Relevant docs for pybind11 trampoline classes:
https://pybind11.readthedocs.io/en/stable/advanced/classes.html?highlight=trampoline#combining-virtual-functions-and-inheritance

I think this could work for us, but would likely require us to remove the python impls for the message classes which have a C++ impl. Such that the message classes without a C++ impl are always inheriting from the C++ base.

Personally I think this is a good thing, and would remove the need to maintain redundant Python & C++ impls for the same classes.

What if we just had a limited number of message types,... This limits us to "has column with type" sorts of type checks, but that's probably good enough. Thoughts?

Maybe I'm mis-understanding what your proposing, but I'm not sure that would work for the inference & response messages MultiInferenceMessage & MultiResponseMessage which in addition to the dataframe contain a set of tensors. Specifically the amount of heavy lifting those classes are doing to ensure the offsets are properly calculated for both the dataframe & tensors.

@dagardner-nv
Copy link
Contributor Author

I believe the current plan is to migrate everything towards the control message.

@dagardner-nv dagardner-nv closed this as not planned Won't fix, can't repro, duplicate, stale Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
Status: Done
Development

No branches or pull requests

2 participants