From 2c255e42132b6175296bd1711871d383d8286cbf Mon Sep 17 00:00:00 2001 From: kthui <18255193+kthui@users.noreply.github.com> Date: Mon, 12 Aug 2024 18:48:25 -0700 Subject: [PATCH 1/7] Do not reload unmodified loaded model version --- src/model_config_utils.cc | 4 +++- src/model_config_utils.h | 8 +++++--- src/model_repository_manager/model_lifecycle.cc | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/model_config_utils.cc b/src/model_config_utils.cc index cc31f666c..09f7d75d0 100644 --- a/src/model_config_utils.cc +++ b/src/model_config_utils.cc @@ -2432,13 +2432,15 @@ TritonToDataType(const TRITONSERVER_DataType dtype) } bool -EquivalentInNonInstanceGroupConfig( +EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( const inference::ModelConfig& old_config, const inference::ModelConfig& new_config) { ::google::protobuf::util::MessageDifferencer pb_diff; pb_diff.IgnoreField( old_config.descriptor()->FindFieldByLowercaseName("instance_group")); + pb_diff.IgnoreField( + old_config.descriptor()->FindFieldByLowercaseName("version_policy")); return pb_diff.Compare(old_config, new_config); } diff --git a/src/model_config_utils.h b/src/model_config_utils.h index 8bd9af600..4f1e1f339 100644 --- a/src/model_config_utils.h +++ b/src/model_config_utils.h @@ -288,12 +288,14 @@ TRITONSERVER_DataType DataTypeToTriton(const inference::DataType dtype); /// \return The data type. inference::DataType TritonToDataType(const TRITONSERVER_DataType dtype); -/// Check if non instance group settings on the model configs are equivalent. +/// Check if non instance group and non version policy settings on the model +/// configs are equivalent. /// \param old_config The old model config. /// \param new_config The new model config. /// \return True if the model configs are equivalent in all non instance group -/// settings. False if they differ in non instance group settings. -bool EquivalentInNonInstanceGroupConfig( +/// and non version policy settings. False if they differ in non instance group +/// and non version policy settings. +bool EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( const inference::ModelConfig& old_config, const inference::ModelConfig& new_config); diff --git a/src/model_repository_manager/model_lifecycle.cc b/src/model_repository_manager/model_lifecycle.cc index 437829a4c..1a8978249 100644 --- a/src/model_repository_manager/model_lifecycle.cc +++ b/src/model_repository_manager/model_lifecycle.cc @@ -489,7 +489,7 @@ ModelLifeCycle::AsyncLoad( // The model is currently being served. Check if the model load could // be completed with a simple config update. if (!is_model_file_updated && !serving_model->is_ensemble_ && - EquivalentInNonInstanceGroupConfig( + EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( serving_model->model_config_, model_config)) { // Update the model model_info = serving_model.get(); From 3d2f5c3451ddbf6ba16581014d8e6a8ca2b3fdcf Mon Sep 17 00:00:00 2001 From: kthui <18255193+kthui@users.noreply.github.com> Date: Tue, 13 Aug 2024 18:35:28 -0700 Subject: [PATCH 2/7] Track model directory timestamps more granularly to detect updates to model version files --- .../model_lifecycle.cc | 5 +- .../model_lifecycle.h | 6 +- .../model_repository_manager.cc | 174 ++++++++++++------ .../model_repository_manager.h | 42 +++-- 4 files changed, 158 insertions(+), 69 deletions(-) diff --git a/src/model_repository_manager/model_lifecycle.cc b/src/model_repository_manager/model_lifecycle.cc index 1a8978249..a78fac5d9 100644 --- a/src/model_repository_manager/model_lifecycle.cc +++ b/src/model_repository_manager/model_lifecycle.cc @@ -434,7 +434,7 @@ Status ModelLifeCycle::AsyncLoad( const ModelIdentifier& model_id, const std::string& model_path, const inference::ModelConfig& model_config, const bool is_config_provided, - const bool is_model_file_updated, + const ModelTimestamp& prev_timestamp, const ModelTimestamp& curr_timestamp, const std::shared_ptr& agent_model_list, std::function&& OnComplete) { @@ -488,7 +488,8 @@ ModelLifeCycle::AsyncLoad( if (serving_model->state_ == ModelReadyState::READY) { // The model is currently being served. Check if the model load could // be completed with a simple config update. - if (!is_model_file_updated && !serving_model->is_ensemble_ && + if (!serving_model->is_ensemble_ && + !prev_timestamp.IsModelVersionModified(curr_timestamp, version) && EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( serving_model->model_config_, model_config)) { // Update the model diff --git a/src/model_repository_manager/model_lifecycle.h b/src/model_repository_manager/model_lifecycle.h index 7016ef29a..13d3aa54d 100644 --- a/src/model_repository_manager/model_lifecycle.h +++ b/src/model_repository_manager/model_lifecycle.h @@ -1,4 +1,4 @@ -// Copyright 2022-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// Copyright 2022-2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. // // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions @@ -162,6 +162,7 @@ class TritonRepoAgentModelList { }; class InferenceServer; +class ModelTimestamp; class ModelLifeCycle { public: @@ -183,7 +184,8 @@ class ModelLifeCycle { Status AsyncLoad( const ModelIdentifier& model_id, const std::string& model_path, const inference::ModelConfig& model_config, const bool is_config_provided, - const bool is_model_file_updated, + const ModelTimestamp& prev_timestamp, + const ModelTimestamp& curr_timestamp, const std::shared_ptr& agent_model_list, std::function&& OnComplete); diff --git a/src/model_repository_manager/model_repository_manager.cc b/src/model_repository_manager/model_repository_manager.cc index c5609feb3..a7ccc6093 100644 --- a/src/model_repository_manager/model_repository_manager.cc +++ b/src/model_repository_manager/model_repository_manager.cc @@ -267,7 +267,7 @@ CreateAgentModelListWithLoadAction( // Return the latest modification time in ns for all files/folders starting // from the path. The modification time will be 0 if there is an error. int64_t -GetModifiedTime(const std::string& path) +GetPathModifiedTime(const std::string& path) { // If there is an error in any step the fall-back default // modification time is 0. This means that in error cases 'path' @@ -306,18 +306,15 @@ GetModifiedTime(const std::string& path) for (const auto& child : contents) { const auto full_path = JoinPath({path, child}); - mtime = std::max(mtime, GetModifiedTime(full_path)); + mtime = std::max(mtime, GetPathModifiedTime(full_path)); } return mtime; } -// Return the latest modification time in ns for '' -// in a model directory path. The time for "config.pbtxt" will be 0 if not -// found at "[model_dir_path]/config.pbtxt" or "[model_dir_path]/configs/ -// .pbtxt" if "--model-config-name" is set. The time for -// "model files" includes the time for 'model_dir_path'. -std::pair -GetDetailedModifiedTime( + +} // namespace + +ModelTimestamp::ModelTimestamp( const std::string& model_dir_path, const std::string& model_config_path) { // Check if 'model_dir_path' is a directory. @@ -326,64 +323,131 @@ GetDetailedModifiedTime( if (!status.IsOk()) { LOG_ERROR << "Failed to determine modification time for '" << model_dir_path << "': " << status.AsString(); - return std::make_pair(0, 0); + return; } if (!is_dir) { LOG_ERROR << "Failed to determine modification time for '" << model_dir_path << "': Model directory path is not a directory"; - return std::make_pair(0, 0); + return; } - std::pair mtime(0, 0); // - - // Populate 'model files' time to the model directory time - status = FileModificationTime(model_dir_path, &mtime.second); + // Populate time for 'model_dir_path'. + int64_t model_dir_time = 0; + status = FileModificationTime(model_dir_path, &model_dir_time); if (!status.IsOk()) { LOG_ERROR << "Failed to determine modification time for '" << model_dir_path << "': " << status.AsString(); - return std::make_pair(0, 0); + return; } + model_timestamps_.emplace("", model_dir_time); - // Get files/folders in model_dir_path. - std::set contents; - status = GetDirectoryContents(model_dir_path, &contents); + // Populate time for all immediate files/folders in 'model_dir_path'. + std::set dir_contents; + status = GetDirectoryContents(model_dir_path, &dir_contents); if (!status.IsOk()) { LOG_ERROR << "Failed to determine modification time for '" << model_dir_path << "': " << status.AsString(); - return std::make_pair(0, 0); - } - // Get latest modification time for each files/folders, and place it at the - // correct category. - for (const auto& child : contents) { - const auto full_path = JoinPath({model_dir_path, child}); - if (full_path == model_config_path) { - // config.pbtxt or customized config file in configs folder - mtime.first = GetModifiedTime(full_path); - } else { - // model files - mtime.second = std::max(mtime.second, GetModifiedTime(full_path)); + model_timestamps_.clear(); + return; + } + for (const auto& content_name : dir_contents) { + const auto content_path = JoinPath({model_dir_path, content_name}); + bool is_model_config = model_config_path.rfind(content_path, 0) == 0; + if (is_model_config) { + if (!model_config_content_name_.empty()) { + LOG_ERROR << "Failed to determine modification time for '" + << model_dir_path << "': Duplicate model config is detected"; + model_timestamps_.clear(); + model_config_content_name_.clear(); + return; + } + model_config_content_name_ = content_name; } + model_timestamps_.emplace(content_name, GetPathModifiedTime(content_path)); } +} - return mtime; +bool +ModelTimestamp::IsModified(const ModelTimestamp& new_timestamp) const +{ + int64_t old_modified_time = GetModifiedTime(); + int64_t new_modified_time = new_timestamp.GetModifiedTime(); + return new_modified_time > old_modified_time; } -// Return true if any file in the 'model_dir_path' has been modified more -// recently than 'last_ns', which represents last modified time for -// ''. Update the 'last_ns' to the most-recent -// modified time. + bool -IsModified( - const std::string& model_dir_path, const std::string& model_config_path, - std::pair* last_ns) +ModelTimestamp::IsModelVersionModified( + const ModelTimestamp& new_timestamp, const int64_t version) const { - auto new_ns = GetDetailedModifiedTime(model_dir_path, model_config_path); - bool modified = std::max(new_ns.first, new_ns.second) > - std::max(last_ns->first, last_ns->second); - last_ns->swap(new_ns); - return modified; + int64_t old_modified_time = std::max( + GetModelVersionModifiedTime(version), + GetNonModelConfigNorVersionNorDirModifiedTime()); + int64_t new_modified_time = std::max( + new_timestamp.GetModelVersionModifiedTime(version), + new_timestamp.GetNonModelConfigNorVersionNorDirModifiedTime()); + return new_modified_time > old_modified_time; } -} // namespace +int64_t +ModelTimestamp::GetModifiedTime() const +{ + int64_t modified_time = 0; + for (const auto& pair : model_timestamps_) { + const int64_t time = pair.second; + modified_time = std::max(modified_time, time); + } + return modified_time; +} + +int64_t +ModelTimestamp::GetModelVersionModifiedTime(const int64_t version) const +{ + int64_t modified_time = 0; + auto itr = model_timestamps_.find(std::to_string(version)); + if (itr != model_timestamps_.end()) { + modified_time = itr->second; + } + return modified_time; +} + +int64_t +ModelTimestamp::GetNonModelConfigNorVersionNorDirModifiedTime() const +{ + // Get modified time excluding time from model config, model version + // directory(s) and model directory. + int64_t modified_time = 0; + for (const auto& pair : model_timestamps_) { + const std::string& content_name = pair.first; + bool found_non_digit_in_content_name = false; + for (const char& c : content_name) { + if (std::isdigit(c) == 0) { + found_non_digit_in_content_name = true; + break; + } + } + // All model version directory(s) will not 'found_non_digit_in_content_name' + // as they are all digit(s). + // Model directory name will not 'found_non_digit_in_content_name' as it is + // empty. + if (found_non_digit_in_content_name && + content_name != model_config_content_name_) { + const int64_t time = pair.second; + modified_time = std::max(modified_time, time); + } + } + return modified_time; +} + +void +ModelTimestamp::SetModelConfigModifiedTime(const int64_t time_ns) +{ + if (model_config_content_name_.empty()) { + LOG_ERROR << "Failed to set config modification time: " + "model_config_content_name_ is empty"; + return; + } + model_timestamps_[model_config_content_name_] = time_ns; +} ModelRepositoryManager::ModelRepositoryManager( const std::set& repository_paths, const bool autofill, @@ -624,7 +688,7 @@ ModelRepositoryManager::LoadModelByDependency( auto status = model_life_cycle_->AsyncLoad( valid_model->model_id_, itr->second->model_path_, valid_model->model_config_, itr->second->is_config_provided_, - itr->second->mtime_nsec_.second > itr->second->prev_mtime_ns_.second, + itr->second->prev_mtime_ns_, itr->second->mtime_nsec_, itr->second->agent_model_list_, [model_state](Status load_status) { model_state->status_ = load_status; model_state->ready_.set_value(); @@ -1406,7 +1470,7 @@ ModelRepositoryManager::InitializeModelInfo( if (iitr != infos_.end()) { linfo->prev_mtime_ns_ = iitr->second->mtime_nsec_; } else { - linfo->prev_mtime_ns_ = std::make_pair(0, 0); + linfo->prev_mtime_ns_ = ModelTimestamp(); } // Set 'mtime_nsec_' and override 'model_path_' if current path is empty @@ -1435,7 +1499,7 @@ ModelRepositoryManager::InitializeModelInfo( // For file override, set 'mtime_nsec_' to minimum value so that // the next load without override will trigger re-load to undo // the override while the local files may still be unchanged. - linfo->mtime_nsec_ = std::make_pair(0, 0); + linfo->mtime_nsec_ = ModelTimestamp(); linfo->model_path_ = location; linfo->model_config_path_ = JoinPath({location, kModelConfigPbTxt}); linfo->agent_model_list_.reset(new TritonRepoAgentModelList()); @@ -1445,14 +1509,16 @@ ModelRepositoryManager::InitializeModelInfo( GetModelConfigFullPath(linfo->model_path_, model_config_name_); // Model is not loaded. if (iitr == infos_.end()) { - linfo->mtime_nsec_ = GetDetailedModifiedTime( - linfo->model_path_, linfo->model_config_path_); + linfo->mtime_nsec_ = + ModelTimestamp(linfo->model_path_, linfo->model_config_path_); } else { // Check the current timestamps to determine if model actually has been // modified linfo->mtime_nsec_ = linfo->prev_mtime_ns_; - unmodified = !IsModified( - linfo->model_path_, linfo->model_config_path_, &linfo->mtime_nsec_); + ModelTimestamp new_mtime_ns = + ModelTimestamp(linfo->model_path_, linfo->model_config_path_); + unmodified = !linfo->mtime_nsec_.IsModified(new_mtime_ns); + linfo->mtime_nsec_ = new_mtime_ns; } } @@ -1467,9 +1533,9 @@ ModelRepositoryManager::InitializeModelInfo( // the override while the local files may still be unchanged. auto time_since_epoch = std::chrono::system_clock::now().time_since_epoch(); - linfo->mtime_nsec_.first = + linfo->mtime_nsec_.SetModelConfigModifiedTime( std::chrono::duration_cast(time_since_epoch) - .count(); + .count()); unmodified = false; const std::string& override_config = override_parameter->ValueString(); diff --git a/src/model_repository_manager/model_repository_manager.h b/src/model_repository_manager/model_repository_manager.h index ffc0d4c1b..0cb78ed84 100644 --- a/src/model_repository_manager/model_repository_manager.h +++ b/src/model_repository_manager/model_repository_manager.h @@ -49,6 +49,33 @@ enum ActionType { NO_ACTION, LOAD, UNLOAD }; /// Predefined reason strings #define MODEL_READY_REASON_DUPLICATE "model appears in two or more repositories" +// Information about timestamps in model directory. +class ModelTimestamp { + public: + ModelTimestamp() {} + ModelTimestamp( + const std::string& model_dir_path, const std::string& model_config_path); + + bool IsModified(const ModelTimestamp& new_timestamp) const; + bool IsModelVersionModified( + const ModelTimestamp& new_timestamp, const int64_t version) const; + + void SetModelConfigModifiedTime(const int64_t time_ns); + + private: + int64_t GetModifiedTime() const; + int64_t GetModelVersionModifiedTime(const int64_t version) const; + int64_t GetNonModelConfigNorVersionNorDirModifiedTime() const; + + // Timestamps of all contents in the model directory, i.e. + // - "": ns timestamp of model config (directory) + // - "1": ns timestamp of model version 1 + // - "": ns timestamp of the model directory + std::unordered_map model_timestamps_; + // If empty, model config is not detected on the model directory. + std::string model_config_content_name_; +}; + /// An object to manage the model repository active in the server. class ModelRepositoryManager { public: @@ -77,23 +104,16 @@ class ModelRepositoryManager { // Information about the model. struct ModelInfo { ModelInfo( - const std::pair& mtime_nsec, - const std::pair& prev_mtime_ns, + const ModelTimestamp& mtime_nsec, const ModelTimestamp& prev_mtime_ns, const std::string& model_path, const std::string& model_config_path) : mtime_nsec_(mtime_nsec), prev_mtime_ns_(prev_mtime_ns), explicitly_load_(true), model_path_(model_path), model_config_path_(model_config_path), is_config_provided_(false) { } - ModelInfo() - : mtime_nsec_(0, 0), prev_mtime_ns_(0, 0), explicitly_load_(true), - is_config_provided_(false) - { - } - // Current last modified time in ns, for '' - std::pair mtime_nsec_; - // Previous last modified time in ns, for '' - std::pair prev_mtime_ns_; + ModelInfo() : explicitly_load_(true), is_config_provided_(false) {} + ModelTimestamp mtime_nsec_; + ModelTimestamp prev_mtime_ns_; bool explicitly_load_; inference::ModelConfig model_config_; std::string model_path_; From e6da04c1641636937a42ebbec71c25f81ae4876e Mon Sep 17 00:00:00 2001 From: kthui <18255193+kthui@users.noreply.github.com> Date: Fri, 16 Aug 2024 14:22:38 -0700 Subject: [PATCH 3/7] Rename model config util config change require reload function --- src/model_config_utils.cc | 4 ++-- src/model_config_utils.h | 6 +++--- src/model_repository_manager/model_lifecycle.cc | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/model_config_utils.cc b/src/model_config_utils.cc index 09f7d75d0..4fd1e1be0 100644 --- a/src/model_config_utils.cc +++ b/src/model_config_utils.cc @@ -2432,7 +2432,7 @@ TritonToDataType(const TRITONSERVER_DataType dtype) } bool -EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( +ConfigChangeRequiresReload( const inference::ModelConfig& old_config, const inference::ModelConfig& new_config) { @@ -2441,7 +2441,7 @@ EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( old_config.descriptor()->FindFieldByLowercaseName("instance_group")); pb_diff.IgnoreField( old_config.descriptor()->FindFieldByLowercaseName("version_policy")); - return pb_diff.Compare(old_config, new_config); + return !pb_diff.Compare(old_config, new_config); } bool diff --git a/src/model_config_utils.h b/src/model_config_utils.h index 4f1e1f339..ba4d75636 100644 --- a/src/model_config_utils.h +++ b/src/model_config_utils.h @@ -292,10 +292,10 @@ inference::DataType TritonToDataType(const TRITONSERVER_DataType dtype); /// configs are equivalent. /// \param old_config The old model config. /// \param new_config The new model config. -/// \return True if the model configs are equivalent in all non instance group -/// and non version policy settings. False if they differ in non instance group +/// \return False if the model configs are equivalent in all non instance group +/// and non version policy settings. True if they differ in non instance group /// and non version policy settings. -bool EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( +bool ConfigChangeRequiresReload( const inference::ModelConfig& old_config, const inference::ModelConfig& new_config); diff --git a/src/model_repository_manager/model_lifecycle.cc b/src/model_repository_manager/model_lifecycle.cc index a78fac5d9..a04d99a97 100644 --- a/src/model_repository_manager/model_lifecycle.cc +++ b/src/model_repository_manager/model_lifecycle.cc @@ -490,7 +490,7 @@ ModelLifeCycle::AsyncLoad( // be completed with a simple config update. if (!serving_model->is_ensemble_ && !prev_timestamp.IsModelVersionModified(curr_timestamp, version) && - EquivalentInNonInstanceGroupAndNonVersionPolicyConfig( + !ConfigChangeRequiresReload( serving_model->model_config_, model_config)) { // Update the model model_info = serving_model.get(); From 41e57e55cf55c3f0496fd95735e06e4764cf5659 Mon Sep 17 00:00:00 2001 From: kthui <18255193+kthui@users.noreply.github.com> Date: Fri, 16 Aug 2024 17:22:53 -0700 Subject: [PATCH 4/7] Re-organize ModelTimestamp() and throw exception --- .../model_repository_manager.cc | 140 ++++++++++-------- .../model_repository_manager.h | 18 ++- 2 files changed, 94 insertions(+), 64 deletions(-) diff --git a/src/model_repository_manager/model_repository_manager.cc b/src/model_repository_manager/model_repository_manager.cc index a7ccc6093..610826fae 100644 --- a/src/model_repository_manager/model_repository_manager.cc +++ b/src/model_repository_manager/model_repository_manager.cc @@ -276,9 +276,9 @@ GetPathModifiedTime(const std::string& path) bool path_is_dir; Status status = IsDirectory(path, &path_is_dir); if (!status.IsOk()) { - LOG_ERROR << "Failed to determine modification time for '" << path - << "': " << status.AsString(); - return 0; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + path + + "': " + status.AsString()); } // If 'path' is a file return its mtime. Otherwise, using the modification @@ -286,9 +286,9 @@ GetPathModifiedTime(const std::string& path) int64_t mtime = 0; status = FileModificationTime(path, &mtime); if (!status.IsOk()) { - LOG_ERROR << "Failed to determine modification time for '" << path - << "': " << status.AsString(); - return 0; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + path + + "': " + status.AsString()); } if (!path_is_dir) { return mtime; @@ -299,9 +299,9 @@ GetPathModifiedTime(const std::string& path) std::set contents; status = GetDirectoryContents(path, &contents); if (!status.IsOk()) { - LOG_ERROR << "Failed to determine modification time for '" << path - << "': " << status.AsString(); - return 0; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + path + + "': " + status.AsString()); } for (const auto& child : contents) { @@ -317,49 +317,60 @@ GetPathModifiedTime(const std::string& path) ModelTimestamp::ModelTimestamp( const std::string& model_dir_path, const std::string& model_config_path) { - // Check if 'model_dir_path' is a directory. + AssertPathIsDirectory(model_dir_path); + PopulateModelDirectoryTime(model_dir_path); + PopulateModelDirectoryContentsTime(model_dir_path, model_config_path); +} + +void +ModelTimestamp::AssertPathIsDirectory(const std::string& path) const +{ bool is_dir; - Status status = IsDirectory(model_dir_path, &is_dir); + Status status = IsDirectory(path, &is_dir); if (!status.IsOk()) { - LOG_ERROR << "Failed to determine modification time for '" << model_dir_path - << "': " << status.AsString(); - return; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + path + + "': " + status.AsString()); } if (!is_dir) { - LOG_ERROR << "Failed to determine modification time for '" << model_dir_path - << "': Model directory path is not a directory"; - return; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + path + + "': Path is not a directory"); } +} - // Populate time for 'model_dir_path'. - int64_t model_dir_time = 0; - status = FileModificationTime(model_dir_path, &model_dir_time); +void +ModelTimestamp::PopulateModelDirectoryTime(const std::string& dir_path) +{ + int64_t time_ns = 0; + Status status = FileModificationTime(dir_path, &time_ns); if (!status.IsOk()) { - LOG_ERROR << "Failed to determine modification time for '" << model_dir_path - << "': " << status.AsString(); - return; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + dir_path + + "': " + status.AsString()); } - model_timestamps_.emplace("", model_dir_time); + model_timestamps_.emplace("", time_ns); +} - // Populate time for all immediate files/folders in 'model_dir_path'. +void +ModelTimestamp::PopulateModelDirectoryContentsTime( + const std::string& dir_path, const std::string& config_path) +{ std::set dir_contents; - status = GetDirectoryContents(model_dir_path, &dir_contents); + Status status = GetDirectoryContents(dir_path, &dir_contents); if (!status.IsOk()) { - LOG_ERROR << "Failed to determine modification time for '" << model_dir_path - << "': " << status.AsString(); - model_timestamps_.clear(); - return; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + dir_path + + "': " + status.AsString()); } for (const auto& content_name : dir_contents) { - const auto content_path = JoinPath({model_dir_path, content_name}); - bool is_model_config = model_config_path.rfind(content_path, 0) == 0; + const auto content_path = JoinPath({dir_path, content_name}); + bool is_model_config = config_path.rfind(content_path, 0) == 0; if (is_model_config) { if (!model_config_content_name_.empty()) { - LOG_ERROR << "Failed to determine modification time for '" - << model_dir_path << "': Duplicate model config is detected"; - model_timestamps_.clear(); - model_config_content_name_.clear(); - return; + throw std::runtime_error( + std::string("Failed to determine modification time for '") + + dir_path + "': Duplicate model config is detected"); } model_config_content_name_ = content_name; } @@ -368,7 +379,7 @@ ModelTimestamp::ModelTimestamp( } bool -ModelTimestamp::IsModified(const ModelTimestamp& new_timestamp) const +ModelTimestamp::IsModified(const ModelTimestamp& new_timestamp) const noexcept { int64_t old_modified_time = GetModifiedTime(); int64_t new_modified_time = new_timestamp.GetModifiedTime(); @@ -377,7 +388,7 @@ ModelTimestamp::IsModified(const ModelTimestamp& new_timestamp) const bool ModelTimestamp::IsModelVersionModified( - const ModelTimestamp& new_timestamp, const int64_t version) const + const ModelTimestamp& new_timestamp, const int64_t version) const noexcept { int64_t old_modified_time = std::max( GetModelVersionModifiedTime(version), @@ -389,7 +400,7 @@ ModelTimestamp::IsModelVersionModified( } int64_t -ModelTimestamp::GetModifiedTime() const +ModelTimestamp::GetModifiedTime() const noexcept { int64_t modified_time = 0; for (const auto& pair : model_timestamps_) { @@ -400,7 +411,8 @@ ModelTimestamp::GetModifiedTime() const } int64_t -ModelTimestamp::GetModelVersionModifiedTime(const int64_t version) const +ModelTimestamp::GetModelVersionModifiedTime( + const int64_t version) const noexcept { int64_t modified_time = 0; auto itr = model_timestamps_.find(std::to_string(version)); @@ -411,7 +423,7 @@ ModelTimestamp::GetModelVersionModifiedTime(const int64_t version) const } int64_t -ModelTimestamp::GetNonModelConfigNorVersionNorDirModifiedTime() const +ModelTimestamp::GetNonModelConfigNorVersionNorDirModifiedTime() const noexcept { // Get modified time excluding time from model config, model version // directory(s) and model directory. @@ -442,9 +454,9 @@ void ModelTimestamp::SetModelConfigModifiedTime(const int64_t time_ns) { if (model_config_content_name_.empty()) { - LOG_ERROR << "Failed to set config modification time: " - "model_config_content_name_ is empty"; - return; + throw std::runtime_error( + "Failed to set model configuration modification time: " + "model_config_content_name_ is empty"); } model_timestamps_[model_config_content_name_] = time_ns; } @@ -1508,17 +1520,22 @@ ModelRepositoryManager::InitializeModelInfo( linfo->model_config_path_ = GetModelConfigFullPath(linfo->model_path_, model_config_name_); // Model is not loaded. - if (iitr == infos_.end()) { - linfo->mtime_nsec_ = - ModelTimestamp(linfo->model_path_, linfo->model_config_path_); - } else { - // Check the current timestamps to determine if model actually has been - // modified - linfo->mtime_nsec_ = linfo->prev_mtime_ns_; - ModelTimestamp new_mtime_ns = - ModelTimestamp(linfo->model_path_, linfo->model_config_path_); - unmodified = !linfo->mtime_nsec_.IsModified(new_mtime_ns); - linfo->mtime_nsec_ = new_mtime_ns; + try { // ModelTimestamp(...) may throw + if (iitr == infos_.end()) { + linfo->mtime_nsec_ = + ModelTimestamp(linfo->model_path_, linfo->model_config_path_); + } else { + // Check the current timestamps to determine if model actually has been + // modified + linfo->mtime_nsec_ = linfo->prev_mtime_ns_; + ModelTimestamp new_mtime_ns = + ModelTimestamp(linfo->model_path_, linfo->model_config_path_); + unmodified = !linfo->mtime_nsec_.IsModified(new_mtime_ns); + linfo->mtime_nsec_ = new_mtime_ns; + } + } + catch (const std::runtime_error& e) { + return Status(Status::Code::INTERNAL, e.what()); } } @@ -1533,9 +1550,16 @@ ModelRepositoryManager::InitializeModelInfo( // the override while the local files may still be unchanged. auto time_since_epoch = std::chrono::system_clock::now().time_since_epoch(); - linfo->mtime_nsec_.SetModelConfigModifiedTime( + int64_t time_since_epoch_ns = std::chrono::duration_cast(time_since_epoch) - .count()); + .count(); + try { + linfo->mtime_nsec_.SetModelConfigModifiedTime(time_since_epoch_ns); + } + catch (const std::runtime_error& e) { + return Status(Status::Code::INTERNAL, e.what()); + } + unmodified = false; const std::string& override_config = override_parameter->ValueString(); diff --git a/src/model_repository_manager/model_repository_manager.h b/src/model_repository_manager/model_repository_manager.h index 0cb78ed84..f03773b96 100644 --- a/src/model_repository_manager/model_repository_manager.h +++ b/src/model_repository_manager/model_repository_manager.h @@ -52,20 +52,26 @@ enum ActionType { NO_ACTION, LOAD, UNLOAD }; // Information about timestamps in model directory. class ModelTimestamp { public: - ModelTimestamp() {} + ModelTimestamp() noexcept {} ModelTimestamp( const std::string& model_dir_path, const std::string& model_config_path); - bool IsModified(const ModelTimestamp& new_timestamp) const; + bool IsModified(const ModelTimestamp& new_timestamp) const noexcept; bool IsModelVersionModified( - const ModelTimestamp& new_timestamp, const int64_t version) const; + const ModelTimestamp& new_timestamp, + const int64_t version) const noexcept; void SetModelConfigModifiedTime(const int64_t time_ns); private: - int64_t GetModifiedTime() const; - int64_t GetModelVersionModifiedTime(const int64_t version) const; - int64_t GetNonModelConfigNorVersionNorDirModifiedTime() const; + void AssertPathIsDirectory(const std::string& path) const; + void PopulateModelDirectoryTime(const std::string& dir_path); + void PopulateModelDirectoryContentsTime( + const std::string& dir_path, const std::string& config_path); + + int64_t GetModifiedTime() const noexcept; + int64_t GetModelVersionModifiedTime(const int64_t version) const noexcept; + int64_t GetNonModelConfigNorVersionNorDirModifiedTime() const noexcept; // Timestamps of all contents in the model directory, i.e. // - "": ns timestamp of model config (directory) From 413e510f38c9d466ca168e29557e8d979a973459 Mon Sep 17 00:00:00 2001 From: kthui <18255193+kthui@users.noreply.github.com> Date: Fri, 16 Aug 2024 18:38:02 -0700 Subject: [PATCH 5/7] Revert "Re-organize ModelTimestamp() and throw exception" This reverts commit 41e57e55cf55c3f0496fd95735e06e4764cf5659. --- .../model_repository_manager.cc | 140 ++++++++---------- .../model_repository_manager.h | 18 +-- 2 files changed, 64 insertions(+), 94 deletions(-) diff --git a/src/model_repository_manager/model_repository_manager.cc b/src/model_repository_manager/model_repository_manager.cc index 610826fae..a7ccc6093 100644 --- a/src/model_repository_manager/model_repository_manager.cc +++ b/src/model_repository_manager/model_repository_manager.cc @@ -276,9 +276,9 @@ GetPathModifiedTime(const std::string& path) bool path_is_dir; Status status = IsDirectory(path, &path_is_dir); if (!status.IsOk()) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + path + - "': " + status.AsString()); + LOG_ERROR << "Failed to determine modification time for '" << path + << "': " << status.AsString(); + return 0; } // If 'path' is a file return its mtime. Otherwise, using the modification @@ -286,9 +286,9 @@ GetPathModifiedTime(const std::string& path) int64_t mtime = 0; status = FileModificationTime(path, &mtime); if (!status.IsOk()) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + path + - "': " + status.AsString()); + LOG_ERROR << "Failed to determine modification time for '" << path + << "': " << status.AsString(); + return 0; } if (!path_is_dir) { return mtime; @@ -299,9 +299,9 @@ GetPathModifiedTime(const std::string& path) std::set contents; status = GetDirectoryContents(path, &contents); if (!status.IsOk()) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + path + - "': " + status.AsString()); + LOG_ERROR << "Failed to determine modification time for '" << path + << "': " << status.AsString(); + return 0; } for (const auto& child : contents) { @@ -317,60 +317,49 @@ GetPathModifiedTime(const std::string& path) ModelTimestamp::ModelTimestamp( const std::string& model_dir_path, const std::string& model_config_path) { - AssertPathIsDirectory(model_dir_path); - PopulateModelDirectoryTime(model_dir_path); - PopulateModelDirectoryContentsTime(model_dir_path, model_config_path); -} - -void -ModelTimestamp::AssertPathIsDirectory(const std::string& path) const -{ + // Check if 'model_dir_path' is a directory. bool is_dir; - Status status = IsDirectory(path, &is_dir); + Status status = IsDirectory(model_dir_path, &is_dir); if (!status.IsOk()) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + path + - "': " + status.AsString()); + LOG_ERROR << "Failed to determine modification time for '" << model_dir_path + << "': " << status.AsString(); + return; } if (!is_dir) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + path + - "': Path is not a directory"); + LOG_ERROR << "Failed to determine modification time for '" << model_dir_path + << "': Model directory path is not a directory"; + return; } -} -void -ModelTimestamp::PopulateModelDirectoryTime(const std::string& dir_path) -{ - int64_t time_ns = 0; - Status status = FileModificationTime(dir_path, &time_ns); + // Populate time for 'model_dir_path'. + int64_t model_dir_time = 0; + status = FileModificationTime(model_dir_path, &model_dir_time); if (!status.IsOk()) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + dir_path + - "': " + status.AsString()); + LOG_ERROR << "Failed to determine modification time for '" << model_dir_path + << "': " << status.AsString(); + return; } - model_timestamps_.emplace("", time_ns); -} + model_timestamps_.emplace("", model_dir_time); -void -ModelTimestamp::PopulateModelDirectoryContentsTime( - const std::string& dir_path, const std::string& config_path) -{ + // Populate time for all immediate files/folders in 'model_dir_path'. std::set dir_contents; - Status status = GetDirectoryContents(dir_path, &dir_contents); + status = GetDirectoryContents(model_dir_path, &dir_contents); if (!status.IsOk()) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + dir_path + - "': " + status.AsString()); + LOG_ERROR << "Failed to determine modification time for '" << model_dir_path + << "': " << status.AsString(); + model_timestamps_.clear(); + return; } for (const auto& content_name : dir_contents) { - const auto content_path = JoinPath({dir_path, content_name}); - bool is_model_config = config_path.rfind(content_path, 0) == 0; + const auto content_path = JoinPath({model_dir_path, content_name}); + bool is_model_config = model_config_path.rfind(content_path, 0) == 0; if (is_model_config) { if (!model_config_content_name_.empty()) { - throw std::runtime_error( - std::string("Failed to determine modification time for '") + - dir_path + "': Duplicate model config is detected"); + LOG_ERROR << "Failed to determine modification time for '" + << model_dir_path << "': Duplicate model config is detected"; + model_timestamps_.clear(); + model_config_content_name_.clear(); + return; } model_config_content_name_ = content_name; } @@ -379,7 +368,7 @@ ModelTimestamp::PopulateModelDirectoryContentsTime( } bool -ModelTimestamp::IsModified(const ModelTimestamp& new_timestamp) const noexcept +ModelTimestamp::IsModified(const ModelTimestamp& new_timestamp) const { int64_t old_modified_time = GetModifiedTime(); int64_t new_modified_time = new_timestamp.GetModifiedTime(); @@ -388,7 +377,7 @@ ModelTimestamp::IsModified(const ModelTimestamp& new_timestamp) const noexcept bool ModelTimestamp::IsModelVersionModified( - const ModelTimestamp& new_timestamp, const int64_t version) const noexcept + const ModelTimestamp& new_timestamp, const int64_t version) const { int64_t old_modified_time = std::max( GetModelVersionModifiedTime(version), @@ -400,7 +389,7 @@ ModelTimestamp::IsModelVersionModified( } int64_t -ModelTimestamp::GetModifiedTime() const noexcept +ModelTimestamp::GetModifiedTime() const { int64_t modified_time = 0; for (const auto& pair : model_timestamps_) { @@ -411,8 +400,7 @@ ModelTimestamp::GetModifiedTime() const noexcept } int64_t -ModelTimestamp::GetModelVersionModifiedTime( - const int64_t version) const noexcept +ModelTimestamp::GetModelVersionModifiedTime(const int64_t version) const { int64_t modified_time = 0; auto itr = model_timestamps_.find(std::to_string(version)); @@ -423,7 +411,7 @@ ModelTimestamp::GetModelVersionModifiedTime( } int64_t -ModelTimestamp::GetNonModelConfigNorVersionNorDirModifiedTime() const noexcept +ModelTimestamp::GetNonModelConfigNorVersionNorDirModifiedTime() const { // Get modified time excluding time from model config, model version // directory(s) and model directory. @@ -454,9 +442,9 @@ void ModelTimestamp::SetModelConfigModifiedTime(const int64_t time_ns) { if (model_config_content_name_.empty()) { - throw std::runtime_error( - "Failed to set model configuration modification time: " - "model_config_content_name_ is empty"); + LOG_ERROR << "Failed to set config modification time: " + "model_config_content_name_ is empty"; + return; } model_timestamps_[model_config_content_name_] = time_ns; } @@ -1520,22 +1508,17 @@ ModelRepositoryManager::InitializeModelInfo( linfo->model_config_path_ = GetModelConfigFullPath(linfo->model_path_, model_config_name_); // Model is not loaded. - try { // ModelTimestamp(...) may throw - if (iitr == infos_.end()) { - linfo->mtime_nsec_ = - ModelTimestamp(linfo->model_path_, linfo->model_config_path_); - } else { - // Check the current timestamps to determine if model actually has been - // modified - linfo->mtime_nsec_ = linfo->prev_mtime_ns_; - ModelTimestamp new_mtime_ns = - ModelTimestamp(linfo->model_path_, linfo->model_config_path_); - unmodified = !linfo->mtime_nsec_.IsModified(new_mtime_ns); - linfo->mtime_nsec_ = new_mtime_ns; - } - } - catch (const std::runtime_error& e) { - return Status(Status::Code::INTERNAL, e.what()); + if (iitr == infos_.end()) { + linfo->mtime_nsec_ = + ModelTimestamp(linfo->model_path_, linfo->model_config_path_); + } else { + // Check the current timestamps to determine if model actually has been + // modified + linfo->mtime_nsec_ = linfo->prev_mtime_ns_; + ModelTimestamp new_mtime_ns = + ModelTimestamp(linfo->model_path_, linfo->model_config_path_); + unmodified = !linfo->mtime_nsec_.IsModified(new_mtime_ns); + linfo->mtime_nsec_ = new_mtime_ns; } } @@ -1550,16 +1533,9 @@ ModelRepositoryManager::InitializeModelInfo( // the override while the local files may still be unchanged. auto time_since_epoch = std::chrono::system_clock::now().time_since_epoch(); - int64_t time_since_epoch_ns = + linfo->mtime_nsec_.SetModelConfigModifiedTime( std::chrono::duration_cast(time_since_epoch) - .count(); - try { - linfo->mtime_nsec_.SetModelConfigModifiedTime(time_since_epoch_ns); - } - catch (const std::runtime_error& e) { - return Status(Status::Code::INTERNAL, e.what()); - } - + .count()); unmodified = false; const std::string& override_config = override_parameter->ValueString(); diff --git a/src/model_repository_manager/model_repository_manager.h b/src/model_repository_manager/model_repository_manager.h index f03773b96..0cb78ed84 100644 --- a/src/model_repository_manager/model_repository_manager.h +++ b/src/model_repository_manager/model_repository_manager.h @@ -52,26 +52,20 @@ enum ActionType { NO_ACTION, LOAD, UNLOAD }; // Information about timestamps in model directory. class ModelTimestamp { public: - ModelTimestamp() noexcept {} + ModelTimestamp() {} ModelTimestamp( const std::string& model_dir_path, const std::string& model_config_path); - bool IsModified(const ModelTimestamp& new_timestamp) const noexcept; + bool IsModified(const ModelTimestamp& new_timestamp) const; bool IsModelVersionModified( - const ModelTimestamp& new_timestamp, - const int64_t version) const noexcept; + const ModelTimestamp& new_timestamp, const int64_t version) const; void SetModelConfigModifiedTime(const int64_t time_ns); private: - void AssertPathIsDirectory(const std::string& path) const; - void PopulateModelDirectoryTime(const std::string& dir_path); - void PopulateModelDirectoryContentsTime( - const std::string& dir_path, const std::string& config_path); - - int64_t GetModifiedTime() const noexcept; - int64_t GetModelVersionModifiedTime(const int64_t version) const noexcept; - int64_t GetNonModelConfigNorVersionNorDirModifiedTime() const noexcept; + int64_t GetModifiedTime() const; + int64_t GetModelVersionModifiedTime(const int64_t version) const; + int64_t GetNonModelConfigNorVersionNorDirModifiedTime() const; // Timestamps of all contents in the model directory, i.e. // - "": ns timestamp of model config (directory) From c35fc4d9f108094df9edb269f473d819134ca20d Mon Sep 17 00:00:00 2001 From: kthui <18255193+kthui@users.noreply.github.com> Date: Mon, 19 Aug 2024 13:43:30 -0700 Subject: [PATCH 6/7] Break constructor into multiple functions --- .../model_repository_manager.cc | 56 +++++++++++++------ .../model_repository_manager.h | 6 ++ 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/src/model_repository_manager/model_repository_manager.cc b/src/model_repository_manager/model_repository_manager.cc index a7ccc6093..4b566608d 100644 --- a/src/model_repository_manager/model_repository_manager.cc +++ b/src/model_repository_manager/model_repository_manager.cc @@ -317,38 +317,62 @@ GetPathModifiedTime(const std::string& path) ModelTimestamp::ModelTimestamp( const std::string& model_dir_path, const std::string& model_config_path) { - // Check if 'model_dir_path' is a directory. + bool init_success = + ModelDirectoryPathIsValid(model_dir_path) && + ReadModelDirectoryTimestamp(model_dir_path, model_config_path) && + ReadModelDirectoryContentTimestamps(model_dir_path, model_config_path); + if (!init_success) { + // Same as calling the default constructor. All timestamps are presumed to + // be 0 when comparing. + model_timestamps_.clear(); + model_config_content_name_.clear(); + } +} + +bool +ModelTimestamp::ModelDirectoryPathIsValid(const std::string& path) const +{ bool is_dir; - Status status = IsDirectory(model_dir_path, &is_dir); + Status status = IsDirectory(path, &is_dir); if (!status.IsOk()) { - LOG_ERROR << "Failed to determine modification time for '" << model_dir_path + LOG_ERROR << "Failed to determine modification time for '" << path << "': " << status.AsString(); - return; + return false; } if (!is_dir) { - LOG_ERROR << "Failed to determine modification time for '" << model_dir_path + LOG_ERROR << "Failed to determine modification time for '" << path << "': Model directory path is not a directory"; - return; + return false; } + return true; +} - // Populate time for 'model_dir_path'. +bool +ModelTimestamp::ReadModelDirectoryTimestamp( + const std::string& model_dir_path, const std::string& model_config_path) +{ int64_t model_dir_time = 0; - status = FileModificationTime(model_dir_path, &model_dir_time); + Status status = FileModificationTime(model_dir_path, &model_dir_time); if (!status.IsOk()) { LOG_ERROR << "Failed to determine modification time for '" << model_dir_path << "': " << status.AsString(); - return; + return false; } model_timestamps_.emplace("", model_dir_time); - // Populate time for all immediate files/folders in 'model_dir_path'. + return true; +} + +bool +ModelTimestamp::ReadModelDirectoryContentTimestamps( + const std::string& model_dir_path, const std::string& model_config_path) +{ std::set dir_contents; - status = GetDirectoryContents(model_dir_path, &dir_contents); + Status status = GetDirectoryContents(model_dir_path, &dir_contents); if (!status.IsOk()) { LOG_ERROR << "Failed to determine modification time for '" << model_dir_path << "': " << status.AsString(); - model_timestamps_.clear(); - return; + return false; } for (const auto& content_name : dir_contents) { const auto content_path = JoinPath({model_dir_path, content_name}); @@ -357,14 +381,14 @@ ModelTimestamp::ModelTimestamp( if (!model_config_content_name_.empty()) { LOG_ERROR << "Failed to determine modification time for '" << model_dir_path << "': Duplicate model config is detected"; - model_timestamps_.clear(); - model_config_content_name_.clear(); - return; + return false; } model_config_content_name_ = content_name; } model_timestamps_.emplace(content_name, GetPathModifiedTime(content_path)); } + + return true; } bool diff --git a/src/model_repository_manager/model_repository_manager.h b/src/model_repository_manager/model_repository_manager.h index 0cb78ed84..2c382b2dc 100644 --- a/src/model_repository_manager/model_repository_manager.h +++ b/src/model_repository_manager/model_repository_manager.h @@ -63,6 +63,12 @@ class ModelTimestamp { void SetModelConfigModifiedTime(const int64_t time_ns); private: + bool ModelDirectoryPathIsValid(const std::string& path) const; + bool ReadModelDirectoryTimestamp( + const std::string& model_dir_path, const std::string& model_config_path); + bool ReadModelDirectoryContentTimestamps( + const std::string& model_dir_path, const std::string& model_config_path); + int64_t GetModifiedTime() const; int64_t GetModelVersionModifiedTime(const int64_t version) const; int64_t GetNonModelConfigNorVersionNorDirModifiedTime() const; From f73ab61dadc5b1f89ca5a5c3f41cd9fbb5db7db9 Mon Sep 17 00:00:00 2001 From: kthui <18255193+kthui@users.noreply.github.com> Date: Mon, 19 Aug 2024 17:39:21 -0700 Subject: [PATCH 7/7] Comment on should raise an exception when failed to create timestamp --- src/model_repository_manager/model_repository_manager.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/model_repository_manager/model_repository_manager.cc b/src/model_repository_manager/model_repository_manager.cc index 4b566608d..96df7b637 100644 --- a/src/model_repository_manager/model_repository_manager.cc +++ b/src/model_repository_manager/model_repository_manager.cc @@ -317,6 +317,7 @@ GetPathModifiedTime(const std::string& path) ModelTimestamp::ModelTimestamp( const std::string& model_dir_path, const std::string& model_config_path) { + // DLIS-7221: Raise an exception when failed to create timestamp bool init_success = ModelDirectoryPathIsValid(model_dir_path) && ReadModelDirectoryTimestamp(model_dir_path, model_config_path) &&