-
Notifications
You must be signed in to change notification settings - Fork 4.6k
SonicTriton feature updates, improvements, bug fixes #34508
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
Merged
Merged
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
5b246f5
namespace changes for new triton version
kpedro88 084b3f1
enable i/o compression
kpedro88 c31f787
add ssl options
kpedro88 9ecc209
fix some ssl bugs and add more metadata
kpedro88 a1aae81
remove resnet example (caffe2 backend no longer supported); replace w…
kpedro88 c0a5961
add help messages for tritonTest
kpedro88 3a73088
reduce number and improve control of printouts
kpedro88 50b3d80
improve control of server image
kpedro88 b4319f2
fix tests
kpedro88 e5aa25c
prevent calling toServer/fromServer more than once per event
kpedro88 d6d2001
code format
kpedro88 af80723
update default server image
kpedro88 3ba3cc0
minor code review
kpedro88 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,8 @@ | |||||||||
| #include <functional> | ||||||||||
| #include <utility> | ||||||||||
|
|
||||||||||
| #include "grpc_client.h" | ||||||||||
|
|
||||||||||
| //forward declarations | ||||||||||
| namespace edm { | ||||||||||
| class ActivityRegistry; | ||||||||||
|
|
@@ -34,7 +36,9 @@ class TritonService { | |||||||||
| retries(pset.getUntrackedParameter<int>("retries")), | ||||||||||
| wait(pset.getUntrackedParameter<int>("wait")), | ||||||||||
| instanceName(pset.getUntrackedParameter<std::string>("instanceName")), | ||||||||||
| tempDir(pset.getUntrackedParameter<std::string>("tempDir")) {} | ||||||||||
| tempDir(pset.getUntrackedParameter<std::string>("tempDir")), | ||||||||||
| imageName(pset.getUntrackedParameter<std::string>("imageName")), | ||||||||||
| sandboxName(pset.getUntrackedParameter<std::string>("sandboxName")) {} | ||||||||||
|
|
||||||||||
| bool enable; | ||||||||||
| bool debug; | ||||||||||
|
|
@@ -45,17 +49,31 @@ class TritonService { | |||||||||
| int wait; | ||||||||||
| std::string instanceName; | ||||||||||
| std::string tempDir; | ||||||||||
| std::string imageName; | ||||||||||
| std::string sandboxName; | ||||||||||
| }; | ||||||||||
| struct Server { | ||||||||||
| Server(const edm::ParameterSet& pset) | ||||||||||
| : url(pset.getUntrackedParameter<std::string>("address") + ":" + | ||||||||||
| std::to_string(pset.getUntrackedParameter<unsigned>("port"))), | ||||||||||
| isFallback(pset.getUntrackedParameter<std::string>("name") == fallbackName) {} | ||||||||||
| Server(const std::string& name_, const std::string& url_) : url(url_), isFallback(name_ == fallbackName) {} | ||||||||||
| isFallback(pset.getUntrackedParameter<std::string>("name") == fallbackName), | ||||||||||
| type(TritonServerType::Remote), | ||||||||||
| useSsl(pset.getUntrackedParameter<bool>("useSsl")) { | ||||||||||
| if (useSsl) { | ||||||||||
| sslOptions.root_certificates = pset.getUntrackedParameter<std::string>("rootCertificates"); | ||||||||||
| sslOptions.private_key = pset.getUntrackedParameter<std::string>("privateKey"); | ||||||||||
| sslOptions.certificate_chain = pset.getUntrackedParameter<std::string>("certificateChain"); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| Server(const std::string& name_, const std::string& url_, TritonServerType type_) | ||||||||||
| : url(url_), isFallback(name_ == fallbackName), type(type_), useSsl(false) {} | ||||||||||
|
|
||||||||||
| //members | ||||||||||
| std::string url; | ||||||||||
| bool isFallback; | ||||||||||
| TritonServerType type; | ||||||||||
| bool useSsl; | ||||||||||
|
||||||||||
| TritonServerType type; | |
| bool useSsl; | |
| bool useSsl; | |
| TritonServerType type; |
to save (likely) 8 bytes
Oops, something went wrong.
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.
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.
Why is this
mutable?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.
Ok, I see below why. Please add a comment for the
TritonDataclass that it is not (intended to be?) const-thread-safe.Alternatively consider making the
fromServer()method non-const. Is the requirement "can be called only once per event" only a sanity check for the logic usingTritonData, or is there a deeper reason?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.
The output object is given to the
produce()function as const, because the user shouldn't modify it. (So the method has to be const in order to be callable in that context.) SONIC always uses stream modules (or one modules for analyzers), never global modules.Where would you prefer that I put the comment about const-thread-safe?
Allowing it to be called multiple times per event would require a significant change in the current logic, and it would be inefficient anyway. I think it's better to enforce the better pattern.
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'd add at the top of the class definition
cmssw/HeterogeneousCore/SonicTriton/interface/TritonData.h
Lines 41 to 43 in 2fda345
Some short comment here would actually be helpful as well, accompanied with the
CMS_SA_ALLOWdecorationThere 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.
@makortel I added the recommended comments, and also added a more explicit comment about another non-const-thread-safe behavior in
fromServer()that was introduced in the previous #33801