-
Notifications
You must be signed in to change notification settings - Fork 269
watcher started only when needed #4369
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,7 +196,7 @@ Status ModelManager::start(const Config& config) { | |
| SPDLOG_LOGGER_ERROR(modelmanager_logger, "Couldn't start model manager"); | ||
| return status; | ||
| } | ||
| startWatcher(isStartedWithConfigFile()); | ||
| evaluatePollingState(); | ||
| if (resourcesCleanupIntervalMillisec > 0) | ||
| startCleaner(); | ||
|
|
||
|
|
@@ -212,6 +212,38 @@ void ModelManager::startWatcher(bool watchConfigFile) { | |
| } | ||
| } | ||
|
|
||
| void ModelManager::evaluatePollingState() { | ||
| static const std::set<std::string> singleFileExtensions = { | ||
| ".xml", ".onnx", ".pdmodel", ".pdiparams", ".pb", ".tflite"}; | ||
|
Comment on lines
+216
to
+217
|
||
|
|
||
| versionPollingNeeded = false; | ||
|
Comment on lines
+215
to
+219
|
||
| for (const auto& [name, config] : servedModelConfigs) { | ||
| const std::string& basePath = config.getBasePath(); | ||
| auto ext = std::filesystem::path(basePath).extension().string(); | ||
| if (singleFileExtensions.count(ext) == 0) { | ||
| SPDLOG_LOGGER_TRACE(modelmanager_logger, "Model {} with path {} requires version polling", name, basePath); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would put it on debug or even info if this is a one time call during model loading phase |
||
| versionPollingNeeded = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (watcherStarted) { | ||
| return; | ||
| } | ||
|
|
||
| bool needWatcher = versionPollingNeeded || isStartedWithConfigFile(); | ||
| if (needWatcher && watcherIntervalMillisec > 0) { | ||
| if (versionPollingNeeded) { | ||
| SPDLOG_LOGGER_DEBUG(modelmanager_logger, "Filesystem version polling enabled - at least one model uses version directories"); | ||
| } else { | ||
| SPDLOG_LOGGER_DEBUG(modelmanager_logger, "Filesystem version polling disabled - no model uses version directories"); | ||
| } | ||
| startWatcher(isStartedWithConfigFile()); | ||
| } else { | ||
| SPDLOG_LOGGER_DEBUG(modelmanager_logger, "Filesystem version polling disabled - no model uses version directories"); | ||
|
Comment on lines
+237
to
+243
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe it could be on INFO level logging |
||
| } | ||
|
Comment on lines
+242
to
+244
|
||
| } | ||
|
|
||
| void ModelManager::startCleaner() { | ||
| if ((!cleanerStarted)) { | ||
| std::future<void> exitSignal = cleanerExitTrigger.get_future(); | ||
|
|
@@ -1047,18 +1079,22 @@ Status ModelManager::configFileReloadNeeded(bool& isNeeded) { | |
| void ModelManager::watcher(std::future<void> exitSignal, bool watchConfigFile) { | ||
| SPDLOG_LOGGER_INFO(modelmanager_logger, "Started model manager thread"); | ||
| while (exitSignal.wait_for(std::chrono::milliseconds(this->watcherIntervalMillisec)) == std::future_status::timeout) { | ||
| SPDLOG_LOGGER_TRACE(modelmanager_logger, "Models configuration and filesystem check cycle begin"); | ||
| std::unique_lock<std::recursive_mutex> loadingLock(configMtx); | ||
| if (watchConfigFile) { | ||
| SPDLOG_LOGGER_TRACE(modelmanager_logger, "Checking if config file changed"); | ||
| bool isNeeded; | ||
| configFileReloadNeeded(isNeeded); | ||
| if (isNeeded) { | ||
| loadConfig(); | ||
| evaluatePollingState(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In it's current implementation, that method can start a watcher thread. Does not look like something that should be run in a while loop. |
||
| } | ||
|
Comment on lines
1086
to
1090
|
||
| } | ||
| updateConfigurationWithoutConfigFile(); | ||
| if (versionPollingNeeded) { | ||
| SPDLOG_LOGGER_TRACE(modelmanager_logger, "Models configuration and filesystem check cycle begin"); | ||
| updateConfigurationWithoutConfigFile(); | ||
| SPDLOG_LOGGER_TRACE(modelmanager_logger, "Models configuration and filesystem check cycle end"); | ||
| } | ||
| loadingLock.unlock(); | ||
| SPDLOG_LOGGER_TRACE(modelmanager_logger, "Models configuration and filesystem check cycle end"); | ||
| } | ||
| SPDLOG_LOGGER_INFO(modelmanager_logger, "Stopped model manager thread"); | ||
| } | ||
|
|
||
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 name of this method hides the fact that it may launch watcher thread. I would either rename it or break it in two, so we have evaluate method that returns information if we need to start watcher or not and the second one that starts it.