Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix:
tritonfrontend
gRPC Streaming Segmentation Fault #7671fix:
tritonfrontend
gRPC Streaming Segmentation Fault #7671Changes from 4 commits
c2e25bc
2bce2b4
183224c
c45adc2
1eaa8a6
424a325
b37dd97
6786594
f4783af
114edd2
6d9f8a1
085870e
e225cad
4f09718
1e6224a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this place (HandleGenerate) seem to be relevant:
server/src/http_server.cc
Lines 3235 to 3237 in dbb064f
I can see that HandleInfer on HTTP is guarded:
server/src/http_server.cc
Lines 3599 to 3602 in dbb064f
qq to @GuanLuo , I can see that you've added guards for
HandleInfer
(link above), was there a reason not to guardHandleGenerate
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KrishnanPrash, do you need to support trace update with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably just a missed spot..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe
trace_manager_
needs to be guarded with a#ifdef TRACING_ENABLE_TRACING
compile time macro becauseStartTrace()
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 benull
after being passed to the services. However, because the bindings do not yet support tracing, a hopefully temporary situation arises of tracing being enabled, buttrace_manager_
being null.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah so this is temporary? - once enable tracing is added we can remove the runtime check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.