Enable Triton for SageMaker MME mode #4181
Conversation
nskool
changed the title
src/sagemaker_server.h
Outdated
| @@ -103,6 +125,9 @@ class SagemakerAPIServer : public HTTPAPIServer { | |||
| const std::string model_version_str_; | |||
|
|
|||
| static const std::string binary_mime_type_; | |||
|
|
|||
| // Maintain list of loaded models | |||
| std::unordered_map<std::string, std::string> sagemaker_models_list; | |||
There was a problem hiding this comment.
Add "_" suffix to indicate that it is a member variable.
There was a problem hiding this comment.
Looks like there can be concurrent read / write to this object (concurrent calls to different endpoint APIs), should avoid race condition with mutex.
| DIR* dir; | ||
| struct dirent* ent; | ||
| int dir_count = 0; | ||
| if ((dir = opendir(repo_path.c_str())) != NULL) { |
There was a problem hiding this comment.
This may not be portable to other platform like Windows? Should the CMakeLists.txt be updated to disallow building with SM endpoint on Windows?
There was a problem hiding this comment.
Seems like it depends on the compiler being used for Windows: https://stackoverflow.com/questions/5530933/dirent-h-in-visual-studio-2010-or-2008. Is there a list of supported compilers for Windows? But SageMaker is linux-only so we could disable it, could you point how to disable the SM endpoint build on Windows?
Thanks for the review! I have made changes as per the comments, if they look good, could you enable the CI? |
|
Just triggered the CI, will also take another look of the PR at the meanwhile |
There was a problem hiding this comment.
The CI is looking good, tag @CoderHam @tanmayv25 for awareness
[Currently work-in-progress, uses references to personal repo (github.com/nskool). Will be removed before PR is marked Ready for review]