Exposing trace context to python backend #346
Conversation
oandreeva-nv
requested review from
Tabrizian,
nnshah1,
kthui,
rmccorm4,
GuanLuo and
krishung5
There was a problem hiding this comment.
LGTM, especially on the shm part, great work 🚀
I understand that this is only stage 1, but I was wondering if we plan to add some documents on how to interact with the trace context? (maybe we'd want to wait until the stage 2 is completed?) I think the demo code in the server PR would be really helpful(at least for my own understanding)
|
@krishung5, yes good point. I'll update docs in a separate PR, so that this can be cherry-picked faster |
| struct InferenceTraceShm { | ||
| bi::managed_external_buffer::handle_t trace_context_shm_handle; | ||
| // The address of the 'TRITONSERVER_InferTrace' object. | ||
| void* triton_trace; |
There was a problem hiding this comment.
Can you remind me why this is void* ? To avoid C API references out of process in stub?
There was a problem hiding this comment.
yes, stub does not have an access to TRITONSERVER_InferTrace
There was a problem hiding this comment.
Looks great! Had a question, but unrelated to approval: https://github.com/triton-inference-server/python_backend/pull/346/files#r1526885602
Exposing trace context to python backend
| @@ -1611,7 +1611,14 @@ PYBIND11_EMBEDDED_MODULE(c_python_backend_utils, module) | |||
| .export_values(); | |||
|
|
|||
| py::class_<InferenceTrace, std::shared_ptr<InferenceTrace>>( | |||
| module, "InferenceTrace"); | |||
| module, "InferenceTrace") | |||
| .def("get_context", [](InferenceTrace& self) -> py::object { | |||
There was a problem hiding this comment.
Can we remove get_ from here? We usually don't include get_ in the public API names unless there is a name conflict.
| .def("get_context", [](InferenceTrace& self) -> py::object { | ||
| auto context = self.Context(); | ||
| if (context != "") { | ||
| return py::str(context); |
There was a problem hiding this comment.
Is it possible to convert the context into the object that the Python Otel requires?
There was a problem hiding this comment.
Would be good to have a Python object wrapper for context. It would allow us to have more flexibility for future changes.
| if (context != "") { | ||
| return py::str(context); | ||
| } | ||
| return py::none(); |
There was a problem hiding this comment.
Do we have a plan for what context is going to look like for non-Otel scenarios?
Added needed logic to pass trace context to the stub proces + accessing it from the stub process.
Current discussion:
get_tracedoes it need amodefield: https://github.com/triton-inference-server/server/pull/6985/files#r1525367279Most likely
modefield will be removed fromget_traceapi.Related PRs:
core: triton-inference-server/core#334
server: triton-inference-server/server#6985