Sonic dynamic model loading #25
Conversation
…r method in TritonClient. Update BuildFile.xml and fix formatting in header files.
…tructor for TritonClient, and update BuildFile.xml to include Catch2 for testing.
…tests; remove old cfg
…lection; remove unused parameters and improve documentation.
- Introduced `loadModel` and `unloadModel` methods for managing model lifecycle. - Added mutex for thread safety during model operations. - Updated `TritonService` header and implementation to support dynamic model management. - Enhanced logging for model loading and unloading processes. - Updated test configurations to include dynamic model loading tests.
…requirements - Modified input handling to utilize actual model input for "x" instead of dummy data. - Adjusted shape and data allocation for input to meet base class expectations. - Updated parameter set description method to use TritonClient for configuration.
|
I have not looked at the test code yet because I think the logic in the TritonService needs to be addressed first. Another general point: part of the idea for dynamic loading with the fallback server would be to get rid of the |
- Applied scram b code-format to fix formatting issues - Added TRITON_THROW_IF_ERROR for error handling in loadModel/unloadModel - Fixed unload semantics to not erase tracking data on failure - Added comments explaining dynamic loading test requirements - Removed spurious formatting changes
- Removed loadedModels_ set, now derive loaded status from modelRefCount_ > 0 - Simplified loadModel() and unloadModel() to only work with fallback server - Removed server searching logic since only fallback is supported - Added documentation clarifying fallback-only limitation - Updated error messages to reflect this architectural decision - Fixed XML comment syntax in BuildFile.xml
…ading - Updated retry logic to switch to the fallback server directly for model loading. - Removed previous best server selection logic, simplifying the retry process. - Added logging for fallback loading failures and re-evaluation after loading. - Ensured dynamic loading is explicitly handled in TritonService with updated command options.
…in TritonService - Introduced FallbackModelState to track dynamic model state, including reference count and model path. - Updated loadModel and unloadModel methods to utilize FallbackModelState for managing model lifecycle. - Enhanced logging to reflect model state changes and operations. - Adjusted test configuration to reflect changes in module handling for dynamic model loading.
… handling - Eliminated unservedModels_ from TritonService, simplifying model management. - Updated addModel and preBeginJob methods to work directly with models_. - Enhanced error handling in loadModel for fallback server scenarios. - Improved comments for clarity on model path handling and fallback server usage.
| } | ||
| // Ensure model exists in declared models; preserve non-empty path if provided | ||
| auto& modelInfo(models_.emplace(modelName, path).first->second); | ||
| if (modelInfo.path.empty() && !path.empty()) modelInfo.path = path; |
There was a problem hiding this comment.
why is this line necessary? path is used to construct the Model object on the previous line.
| fallbackOpts_.command += " -w " + std::to_string(fallbackOpts_.wait); | ||
| for (const auto& [modelName, model] : unservedModels_) { | ||
| // Explicit model control mode is required for dynamic loading | ||
| fallbackOpts_.command += " --model-control-mode explicit"; |
There was a problem hiding this comment.
this does not go here. it has to be inserted in the correct places in scripts/cmsTriton. probably a short option should be provided to toggle the behavior (e.g., if we make this the default, then -L could disable it).
Here is a patch showing how I had briefly tested this in cmsTriton recently (without adding a toggle):
diff --git a/HeterogeneousCore/SonicTriton/scripts/cmsTriton b/HeterogeneousCore/SonicTriton/scripts/cmsTriton
index ecf84107bac7..a775ed872697 100755
--- a/HeterogeneousCore/SonicTriton/scripts/cmsTriton
+++ b/HeterogeneousCore/SonicTriton/scripts/cmsTriton
@@ -245,7 +245,7 @@ handle_ports(){
start_docker(){
# mount all model repositories
MOUNTARGS=""
- REPOARGS=""
+ REPOARGS="--model-control-mode=explicit"
for REPO in ${REPOS[@]}; do
MOUNTARGS="$MOUNTARGS -v$REPO:$REPO"
REPOARGS="$REPOARGS --model-repository=${REPO}"
@@ -265,7 +265,7 @@ start_docker(){
start_podman(){
# mount all model repositories
MOUNTARGS=""
- REPOARGS=""
+ REPOARGS="--model-control-mode=explicit"
for REPO in ${REPOS[@]}; do
MOUNTARGS="$MOUNTARGS --volume $REPO:$REPO"
REPOARGS="$REPOARGS --model-repository=${REPO}"
@@ -285,7 +285,7 @@ start_podman(){
start_apptainer(){
# mount all model repositories
MOUNTARGS=""
- REPOARGS=""
+ REPOARGS="--model-control-mode=explicit"
for REPO in ${REPOS[@]}; do
MOUNTARGS="$MOUNTARGS -B $REPO"
REPOARGS="$REPOARGS --model-repository=${REPO}"| descriptions.addWithDefaultLabel(desc); | ||
| } | ||
|
|
||
| bool TritonService::loadModel(const std::string& modelName, const std::string& path) { |
There was a problem hiding this comment.
why does this function allow a different path? the model path is part of the Model object, which is already stored.
| // if already loaded, bump refcount | ||
| if (state.refCount > 0) { | ||
| ++state.refCount; | ||
| modelRefCount_[state.modelName] = state.refCount; |
There was a problem hiding this comment.
it seems like there is unnecessary duplication between Model and FallbackModelState objects, and additionally modelRefCount_ and fallbackModels_. Having to maintain two copies of the refCount info is not preferable.
Instead, I would incorporate the refCount member variable into Model itself, and then only deal directly with Model objects from the models_ map. (Possibly keeping a list of model names currently used by the fallback server, and then getting the objects out of the map by name.)
| if (state.modelName.empty()) state.modelName = modelName; | ||
| if (state.path.empty() && !path.empty()) state.path = path; | ||
|
|
||
| return loadModel(state); |
There was a problem hiding this comment.
The lock still applies to everything that happens in the delegated internal function, so this change does not actually address the underlying point. Again, this is fine for a first implementation, but it will need further examination later.
| false); | ||
|
|
||
| auto err = client->UnloadModel(state.modelName); | ||
| TRITON_THROW_IF_ERROR(err, "unloadModel: failed to unload model " + state.modelName + " from fallback server", |
| edm::Service<TritonService> ts; | ||
|
|
||
| // Fallback-only: update client to fallback server and dynamically load the model there | ||
| const std::string& fallbackName = TritonService::Server::fallbackName; |
There was a problem hiding this comment.
there is no reason for this variable to exist. just pass TritonService::Server::fallbackName directly to the function below.
|
|
||
| // Fallback-only: update client to fallback server and dynamically load the model there | ||
| const std::string& fallbackName = TritonService::Server::fallbackName; | ||
| tritonClient->updateServer(fallbackName); |
There was a problem hiding this comment.
the recent changes in this file seem to have completely eliminated the use of getBestServer() to find another remote server. that is not correct. please restore the original functionality.
| testModelName_(cfg.getParameter<std::string>("testModelName")), | ||
| testModelPath_(cfg.getParameter<std::string>("testModelPath")), |
There was a problem hiding this comment.
why are separate model name and path parameters necessary for this test producer? Triton producers already have these parameters through the client. Just use the existing parameters. (this should also eliminate the need for the extra "path" argument in loadModel())
|
|
||
| parser = getParser() | ||
| parser.add_argument("--modules", metavar=("MODULES"), default=["TritonGraphProducer"], nargs='+', type=str, choices=list(models), help="list of modules to run (choices: %(choices)s)") | ||
| parser.add_argument("--modules", metavar=("MODULES"), default=["DynamicModelLoadingProducer"], nargs='+', type=str, choices=list(models), help="list of modules to run (choices: %(choices)s)") |
2 participants
PR description:
This PR introduces dynamic model loading/unloading capabilities and server health monitoring to the SONIC Triton integration in CMSSW. The main features include:
1. Dynamic Model Loading and Unloading:
loadModel()andunloadModel()methods toTritonServicefor managing model lifecycle at runtimeDynamicModelLoadingProducertest module to validate dynamic model management4. Code Improvements:
customize.pyfor better configurabilityTritonClientwith new constructor for testing and enhanced server connection methodsExpected Output Changes:
Dependencies:
Files Modified:
HeterogeneousCore/SonicCore/src/SonicClientBase.ccHeterogeneousCore/SonicCore/plugins/BuildFile.xmlHeterogeneousCore/SonicTriton/interface/TritonService.hHeterogeneousCore/SonicTriton/interface/TritonClient.hHeterogeneousCore/SonicTriton/src/TritonService.ccHeterogeneousCore/SonicTriton/src/TritonClient.ccHeterogeneousCore/SonicTriton/src/RetryActionDiffServer.ccHeterogeneousCore/SonicTriton/test/BuildFile.xmlHeterogeneousCore/SonicTriton/test/tritonTest_cfg.pyNew Files:
HeterogeneousCore/SonicTriton/test/DynamicModelLoadingProducer.ccHeterogeneousCore/SonicTriton/test/test_RetryActionDiffServer.ccRemoved Files:
HeterogeneousCore/SonicTriton/test/RetryActionDiffServer.cc(replaced with unit test)PR validation:
Integration Tests:
scram b -j 8Known Issues:
Documentation:
Backport Information:
This PR is NOT a backport. It is intended for CMSSW_15_1_X release cycle.
If backporting becomes necessary, it would target future release cycles after initial integration and validation in CMSSW_15_1_X.