fix: tritonfrontend gRPC Streaming Segmentation Fault
#7671
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (trace_manager_) { | ||
| GrpcServerCarrier carrier(state->context_->ctx_.get()); | ||
| auto start_options = | ||
| trace_manager_->GetTraceStartOptions(carrier, request.model_name()); |
There was a problem hiding this comment.
I see now. I think this is not the only potential place for a SegFault. Do we want to fix it in other places as well? Or we're targeting streaming case at the moment?
There was a problem hiding this comment.
Would be happy to add checks in other places as well. Could you provide an example where these checks would be needed?
There was a problem hiding this comment.
@KrishnanPrash Can you look for TRITON_ENABLE_TRACING blocks and break the logic into separate utility function in grpc_utils.h?
This function will take trace_manager_ as input. If trace_manager_ is nullptr then the logic is skipped, otherwise the same logic is run.
There was a problem hiding this comment.
There was a problem hiding this comment.
@KrishnanPrash, do you need to support trace update with this PR?
There was a problem hiding this comment.
I can see that you've added guards for HandleInfer (link above), was there a reason not to guard HandleGenerate ?
Probably just a missed spot..
There was a problem hiding this comment.
I see in the HandleInfer() method - that the trace_manager_ check is not guarded with #IF TRACING compile time macro - should it be?
I guess secondary question - if tracing is compiled in - should trace_manager_ be null? - could we make that a pre-condition instead of runtime check if tracing is enabled? (just a question)
There was a problem hiding this comment.
I don't believe trace_manager_ needs to be guarded with a #ifdef TRACING_ENABLE_TRACING compile time macro because StartTrace() function has it's own check for if tracing is enabled.
As for the secondary question, if tracing is compiled in, trace_manager_ should technically never be null after being passed to the services. However, because the bindings do not yet support tracing, a hopefully temporary situation arises of tracing being enabled, but trace_manager_ being null.
There was a problem hiding this comment.
ah so this is temporary? - once enable tracing is added we can remove the runtime check?
There was a problem hiding this comment.
We could remove the runtime check, but personally I am in favor of keeping these checks because it allows to catch unexpected behavior earlier, rather than later.
After tracing support is provided in tritonfrontend, we could probably modify these checks and return an error to fail earlier with a cleaner error message.
| if (trace_manager_) { | ||
| GrpcServerCarrier carrier(state->context_->ctx_.get()); | ||
| auto start_options = | ||
| trace_manager_->GetTraceStartOptions(carrier, request.model_name()); |
There was a problem hiding this comment.
@KrishnanPrash Can you look for TRITON_ENABLE_TRACING blocks and break the logic into separate utility function in grpc_utils.h?
This function will take trace_manager_ as input. If trace_manager_ is nullptr then the logic is skipped, otherwise the same logic is run.
|
lgtm, I would also let @rmccorm4 take a look |
| "INPUT0": input_text, | ||
| } | ||
|
|
||
| response = requests.post(url, json=data, stream=True) |
There was a problem hiding this comment.
comment for future: I'm not sure that stream=True here is meaningful -- what was your intention or expected behavior by setting it?
(Can revisit later though)
|
LGTM - pending passing pipeline with latest changes (including L0_build_variants, L0_python_api, L0_trace, etc) |
| std::shared_ptr<TraceManager::Trace> trace = | ||
| StartTrace(req, model_name, &triton_trace); | ||
| std::shared_ptr<TraceManager::Trace> trace; | ||
| if (trace_manager_) { |
There was a problem hiding this comment.
Why not put into StartTrace()
There was a problem hiding this comment.
Will make these changes as well as a part of this refactoring ticket [DLIS-7380].
What does the PR do?
Currently, the
tritonfrontendbindings do not support a tracing because anullptris passed as aTraceManagerobject to the respective frontends. Due to a lack of checks for validTraceManagerobject, any operations issued assuming theTraceManagerobject is valid, leads to a segmentation fault.This PR wraps tracing operations in a
if(trace_manager_obj)to ensure theTraceManageris notnullptr.Additional Changes:
test_streaming_inference()inserver/qa/L0_python_api/test_kserve.pyto catch errors in CI pipelinetritonfrontendREADME.md.testing_utilsis imported and used to prevent shadowing based on this suggestion.Checklist
<commit_type>: <Title>Test plan:
Added
server/qa/L0_python_api/test_kserve.py::test_streaming_inference()