From e5a40b7136959658610fcb082ad5d7d0ae7d67c5 Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Tue, 5 Sep 2023 20:05:14 -0700 Subject: [PATCH 1/8] Adding ability to specify subdirectory to download for `LocalizePath` --- src/backend_model.cc | 7 ++-- src/filesystem/api.cc | 6 ++-- src/filesystem/api.h | 6 +++- src/filesystem/implementations/as.h | 5 +-- src/filesystem/implementations/common.h | 3 +- src/filesystem/implementations/gcs.h | 5 +-- src/filesystem/implementations/local.h | 5 +-- src/filesystem/implementations/s3.h | 47 ++++++++++++++----------- src/model_config_utils.cc | 4 +-- 9 files changed, 53 insertions(+), 35 deletions(-) diff --git a/src/backend_model.cc b/src/backend_model.cc index 455098bde..232bfecae 100644 --- a/src/backend_model.cc +++ b/src/backend_model.cc @@ -75,10 +75,11 @@ TritonModel::Create( } // Localize the content of the model repository corresponding to - // 'model_path'. This model holds a handle to the localized content - // so that it persists as long as the model is loaded. + // 'model_path' and model's version. This model holds a handle to + // the localized content so that it persists as long as the model is loaded. std::shared_ptr localized_model_dir; - RETURN_IF_ERROR(LocalizePath(model_path, &localized_model_dir)); + RETURN_IF_ERROR( + LocalizePath(model_path, std::to_string(version), &localized_model_dir)); // Localize paths in backend model config // [FIXME] Remove once a more permanent solution is implemented (DLIS-4211) diff --git a/src/filesystem/api.cc b/src/filesystem/api.cc index 4f3883ccc..991d8d782 100644 --- a/src/filesystem/api.cc +++ b/src/filesystem/api.cc @@ -558,11 +558,13 @@ ReadTextProto(const std::string& path, google::protobuf::Message* msg) } Status -LocalizePath(const std::string& path, std::shared_ptr* localized) +LocalizePath( + const std::string& path, const std::string& fetch_subdir, + std::shared_ptr* localized) { std::shared_ptr fs; RETURN_IF_ERROR(fsm_.GetFileSystem(path, fs)); - return fs->LocalizePath(path, localized); + return fs->LocalizePath(path, fetch_subdir, localized); } Status diff --git a/src/filesystem/api.h b/src/filesystem/api.h index e7711ce8e..552fe39c2 100644 --- a/src/filesystem/api.h +++ b/src/filesystem/api.h @@ -145,11 +145,15 @@ Status ReadTextFile(const std::string& path, std::string* contents); /// Create an object representing a local copy of a path. /// \param path The path of the directory or file. +/// \param fetch_subdir If specified, will only download provided +/// sub directory, otherwise all subdirectories will be downloaded. +/// Does not affect files individual files, located under `path`. /// \param localized Returns the LocalizedPath object /// representing the local copy of the path. /// \return Error status Status LocalizePath( - const std::string& path, std::shared_ptr* localized); + const std::string& path, const std::string& fetch_subdir, + std::shared_ptr* localized); /// Write a string to a file. /// \param path The path of the file. diff --git a/src/filesystem/implementations/as.h b/src/filesystem/implementations/as.h index 13eb80d99..dd7b4a4ff 100644 --- a/src/filesystem/implementations/as.h +++ b/src/filesystem/implementations/as.h @@ -86,7 +86,7 @@ class ASFileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, + const std::string& path, const std::string& fetch_subdir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -424,7 +424,8 @@ ASFileSystem::DownloadFolder( Status ASFileSystem::LocalizePath( - const std::string& path, std::shared_ptr* localized) + const std::string& path, const std::string& fetch_subdir, + std::shared_ptr* localized) { bool exists; RETURN_IF_ERROR(FileExists(path, &exists)); diff --git a/src/filesystem/implementations/common.h b/src/filesystem/implementations/common.h index 5f45444dd..f5e7f0a29 100644 --- a/src/filesystem/implementations/common.h +++ b/src/filesystem/implementations/common.h @@ -83,7 +83,8 @@ class FileSystem { virtual Status ReadTextFile( const std::string& path, std::string* contents) = 0; virtual Status LocalizePath( - const std::string& path, std::shared_ptr* localized) = 0; + const std::string& path, const std::string& fetch_subdir, + std::shared_ptr* localized) = 0; virtual Status WriteTextFile( const std::string& path, const std::string& contents) = 0; virtual Status WriteBinaryFile( diff --git a/src/filesystem/implementations/gcs.h b/src/filesystem/implementations/gcs.h index 56c3d8d34..39de61fce 100644 --- a/src/filesystem/implementations/gcs.h +++ b/src/filesystem/implementations/gcs.h @@ -75,7 +75,7 @@ class GCSFileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, + const std::string& path, const std::string& fetch_subdir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -363,7 +363,8 @@ GCSFileSystem::ReadTextFile(const std::string& path, std::string* contents) Status GCSFileSystem::LocalizePath( - const std::string& path, std::shared_ptr* localized) + const std::string& path, const std::string& fetch_subdir, + std::shared_ptr* localized) { bool exists; RETURN_IF_ERROR(FileExists(path, &exists)); diff --git a/src/filesystem/implementations/local.h b/src/filesystem/implementations/local.h index f6deeb5e6..07d0cfa5f 100644 --- a/src/filesystem/implementations/local.h +++ b/src/filesystem/implementations/local.h @@ -49,7 +49,7 @@ class LocalFileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, + const std::string& path, const std::string& fetch_subdir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -204,7 +204,8 @@ LocalFileSystem::ReadTextFile(const std::string& path, std::string* contents) Status LocalFileSystem::LocalizePath( - const std::string& path, std::shared_ptr* localized) + const std::string& path, const std::string& fetch_subdir, + std::shared_ptr* localized) { // For local file system we don't actually need to download the // directory or file. We use it in place. diff --git a/src/filesystem/implementations/s3.h b/src/filesystem/implementations/s3.h index fe6da18c3..68f967807 100644 --- a/src/filesystem/implementations/s3.h +++ b/src/filesystem/implementations/s3.h @@ -151,7 +151,7 @@ class S3FileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, + const std::string& path, const std::string& fetch_subdir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -628,7 +628,8 @@ S3FileSystem::ReadTextFile(const std::string& path, std::string* contents) Status S3FileSystem::LocalizePath( - const std::string& path, std::shared_ptr* localized) + const std::string& path, const std::string& fetch_subdir, + std::shared_ptr* localized) { // Check if the directory or file exists bool exists; @@ -693,28 +694,34 @@ S3FileSystem::LocalizePath( : JoinPath({(*localized)->Path(), s3_removed_path}); bool is_subdir; RETURN_IF_ERROR(IsDirectory(s3_fpath, &is_subdir)); + bool copy_subdir = + !fetch_subdir.empty() + ? s3_fpath == JoinPath({effective_path, fetch_subdir}) + : true; if (is_subdir) { - // Create local mirror of sub-directories + if (copy_subdir) { + // Create local mirror of sub-directories #ifdef _WIN32 - int status = mkdir(const_cast(local_fpath.c_str())); + int status = mkdir(const_cast(local_fpath.c_str())); #else - int status = mkdir( - const_cast(local_fpath.c_str()), - S_IRUSR | S_IWUSR | S_IXUSR); + int status = mkdir( + const_cast(local_fpath.c_str()), + S_IRUSR | S_IWUSR | S_IXUSR); #endif - if (status == -1) { - return Status( - Status::Code::INTERNAL, - "Failed to create local folder: " + local_fpath + - ", errno:" + strerror(errno)); - } - - // Add sub-directories and deeper files to contents - std::set subdir_contents; - RETURN_IF_ERROR(GetDirectoryContents(s3_fpath, &subdir_contents)); - for (auto itr = subdir_contents.begin(); itr != subdir_contents.end(); - ++itr) { - contents.insert(JoinPath({s3_fpath, *itr})); + if (status == -1) { + return Status( + Status::Code::INTERNAL, + "Failed to create local folder: " + local_fpath + + ", errno:" + strerror(errno)); + } + + // Add sub-directories and deeper files to contents + std::set subdir_contents; + RETURN_IF_ERROR(GetDirectoryContents(s3_fpath, &subdir_contents)); + for (auto itr = subdir_contents.begin(); itr != subdir_contents.end(); + ++itr) { + contents.insert(JoinPath({s3_fpath, *itr})); + } } } else { // Create local copy of file diff --git a/src/model_config_utils.cc b/src/model_config_utils.cc index 850c322c8..050bfe294 100644 --- a/src/model_config_utils.cc +++ b/src/model_config_utils.cc @@ -918,8 +918,8 @@ LocalizePythonBackendExecutionEnvironmentPath( model_path_slash) { // Localize the file std::shared_ptr localized_exec_env_path; - RETURN_IF_ERROR( - LocalizePath(abs_exec_env_path, &localized_exec_env_path)); + RETURN_IF_ERROR(LocalizePath( + abs_exec_env_path, "" /*fetch_subdir*/, &localized_exec_env_path)); // Persist the localized temporary path (*localized_model_dir) ->other_localized_path.push_back(localized_exec_env_path); From 8474fa0754600d0dfea9c14eefefa544f21da42b Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Mon, 11 Sep 2023 17:57:11 -0700 Subject: [PATCH 2/8] Generalized implementation of : added recursive flag and mount_dir --- src/backend_model.cc | 17 +++++-- src/filesystem/api.cc | 14 +++++- src/filesystem/api.h | 20 ++++++-- src/filesystem/implementations/as.h | 5 +- src/filesystem/implementations/common.h | 3 +- src/filesystem/implementations/gcs.h | 5 +- src/filesystem/implementations/local.h | 13 +++-- src/filesystem/implementations/s3.h | 65 +++++++++++++------------ src/model_config_utils.cc | 2 +- 9 files changed, 92 insertions(+), 52 deletions(-) diff --git a/src/backend_model.cc b/src/backend_model.cc index 232bfecae..f6eb27e6a 100644 --- a/src/backend_model.cc +++ b/src/backend_model.cc @@ -75,11 +75,19 @@ TritonModel::Create( } // Localize the content of the model repository corresponding to - // 'model_path' and model's version. This model holds a handle to + // 'model_path'. This model holds a handle to // the localized content so that it persists as long as the model is loaded. std::shared_ptr localized_model_dir; RETURN_IF_ERROR( - LocalizePath(model_path, std::to_string(version), &localized_model_dir)); + LocalizePath(model_path, false /*recursive*/, &localized_model_dir)); + + const auto version_path = JoinPath({model_path, std::to_string(version)}); + std::shared_ptr localized_version_dir; + RETURN_IF_ERROR(LocalizePath( + version_path, true /*recursive*/, localized_model_dir->Path(), + &localized_version_dir)); + + localized_model_dir->other_localized_path.push_back(localized_version_dir); // Localize paths in backend model config // [FIXME] Remove once a more permanent solution is implemented (DLIS-4211) @@ -111,12 +119,11 @@ TritonModel::Create( // Get the path to the backend shared library. Search path is // version directory, model directory, global backend directory. const auto localized_model_path = localized_model_dir->Path(); - const auto version_path = - JoinPath({localized_model_path, std::to_string(version)}); + const auto localized_version_path = localized_version_dir->Path(); const std::string global_path = JoinPath({backend_dir, specialized_backend_name}); const std::vector search_paths = { - version_path, localized_model_path, global_path}; + localized_version_path, localized_model_path, global_path}; std::string backend_libdir; std::string backend_libpath; diff --git a/src/filesystem/api.cc b/src/filesystem/api.cc index 991d8d782..2ec0710bc 100644 --- a/src/filesystem/api.cc +++ b/src/filesystem/api.cc @@ -559,12 +559,22 @@ ReadTextProto(const std::string& path, google::protobuf::Message* msg) Status LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, std::shared_ptr* localized) { std::shared_ptr fs; RETURN_IF_ERROR(fsm_.GetFileSystem(path, fs)); - return fs->LocalizePath(path, fetch_subdir, localized); + return fs->LocalizePath(path, recursive, "", localized); +} + +Status +LocalizePath( + const std::string& path, const bool recursive, const std::string& mount_dir, + std::shared_ptr* localized) +{ + std::shared_ptr fs; + RETURN_IF_ERROR(fsm_.GetFileSystem(path, fs)); + return fs->LocalizePath(path, recursive, mount_dir, localized); } Status diff --git a/src/filesystem/api.h b/src/filesystem/api.h index 552fe39c2..bd9efbd74 100644 --- a/src/filesystem/api.h +++ b/src/filesystem/api.h @@ -145,14 +145,26 @@ Status ReadTextFile(const std::string& path, std::string* contents); /// Create an object representing a local copy of a path. /// \param path The path of the directory or file. -/// \param fetch_subdir If specified, will only download provided -/// sub directory, otherwise all subdirectories will be downloaded. -/// Does not affect files individual files, located under `path`. +/// \param recursive If true, will fetch all sub-directories in +/// the provided path. /// \param localized Returns the LocalizedPath object /// representing the local copy of the path. /// \return Error status Status LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, + std::shared_ptr* localized); + +/// Create an object representing a local copy of a path. +/// \param path The path of the directory or file. +/// \param recursive If true, will fetch all sub-directories in +/// the provided path. +/// \param mount_dir If specified, will use provided local directory +/// for localization. +/// \param localized Returns the LocalizedPath object +/// representing the local copy of the path. +/// \return Error status +Status LocalizePath( + const std::string& path, const bool recursive, const std::string& mount_dir, std::shared_ptr* localized); /// Write a string to a file. diff --git a/src/filesystem/implementations/as.h b/src/filesystem/implementations/as.h index dd7b4a4ff..bfad407c4 100644 --- a/src/filesystem/implementations/as.h +++ b/src/filesystem/implementations/as.h @@ -86,7 +86,8 @@ class ASFileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, + const std::string& mount_dir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -424,7 +425,7 @@ ASFileSystem::DownloadFolder( Status ASFileSystem::LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, const std::string& mount_dir, std::shared_ptr* localized) { bool exists; diff --git a/src/filesystem/implementations/common.h b/src/filesystem/implementations/common.h index f5e7f0a29..b2a236f37 100644 --- a/src/filesystem/implementations/common.h +++ b/src/filesystem/implementations/common.h @@ -83,7 +83,8 @@ class FileSystem { virtual Status ReadTextFile( const std::string& path, std::string* contents) = 0; virtual Status LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, + const std::string& mount_dir, std::shared_ptr* localized) = 0; virtual Status WriteTextFile( const std::string& path, const std::string& contents) = 0; diff --git a/src/filesystem/implementations/gcs.h b/src/filesystem/implementations/gcs.h index 39de61fce..fa96b999f 100644 --- a/src/filesystem/implementations/gcs.h +++ b/src/filesystem/implementations/gcs.h @@ -75,7 +75,8 @@ class GCSFileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, + const std::string& mount_dir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -363,7 +364,7 @@ GCSFileSystem::ReadTextFile(const std::string& path, std::string* contents) Status GCSFileSystem::LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, const std::string& mount_dir, std::shared_ptr* localized) { bool exists; diff --git a/src/filesystem/implementations/local.h b/src/filesystem/implementations/local.h index 07d0cfa5f..fc26368a9 100644 --- a/src/filesystem/implementations/local.h +++ b/src/filesystem/implementations/local.h @@ -49,7 +49,8 @@ class LocalFileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, + const std::string& mount_dir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -204,7 +205,7 @@ LocalFileSystem::ReadTextFile(const std::string& path, std::string* contents) Status LocalFileSystem::LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, const std::string& mount_dir, std::shared_ptr* localized) { // For local file system we don't actually need to download the @@ -255,8 +256,12 @@ LocalFileSystem::MakeDirectory(const std::string& dir, const bool recursive) if (mkdir(dir.c_str(), S_IRWXU) == -1) #endif { - // Only allow the error due to parent directory does not exist - // if 'recursive' is requested + // Return success if directory already exists + if (errno == EEXIST) { + return Status::Success; + } + // In all other cases only allow the error due to parent directory + // does not exist, if 'recursive' is requested if ((errno == ENOENT) && (!dir.empty()) && recursive) { RETURN_IF_ERROR(MakeDirectory(DirName(dir), recursive)); // Retry the creation diff --git a/src/filesystem/implementations/s3.h b/src/filesystem/implementations/s3.h index 68f967807..d39cabca5 100644 --- a/src/filesystem/implementations/s3.h +++ b/src/filesystem/implementations/s3.h @@ -151,7 +151,8 @@ class S3FileSystem : public FileSystem { const std::string& path, std::set* files) override; Status ReadTextFile(const std::string& path, std::string* contents) override; Status LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, + const std::string& mount_dir, std::shared_ptr* localized) override; Status WriteTextFile( const std::string& path, const std::string& contents) override; @@ -628,7 +629,7 @@ S3FileSystem::ReadTextFile(const std::string& path, std::string* contents) Status S3FileSystem::LocalizePath( - const std::string& path, const std::string& fetch_subdir, + const std::string& path, const bool recursive, const std::string& mount_dir, std::shared_ptr* localized) { // Check if the directory or file exists @@ -653,10 +654,18 @@ S3FileSystem::LocalizePath( effective_path = path; } - // Create temporary directory + // Create a local directory for s3 model store + const char* env_mount_dir = std::getenv("TRITON_AWS_MOUNT_DIRECTORY"); std::string tmp_folder; - RETURN_IF_ERROR( - triton::core::MakeTemporaryDirectory(FileSystemType::LOCAL, &tmp_folder)); + if (mount_dir.empty() && env_mount_dir == nullptr) { + RETURN_IF_ERROR(triton::core::MakeTemporaryDirectory( + FileSystemType::LOCAL, &tmp_folder)); + } else { + tmp_folder = mount_dir.empty() ? std::string(env_mount_dir) : mount_dir; + tmp_folder = + JoinPath({tmp_folder, path.substr(path.find_last_of('/') + 1)}); + RETURN_IF_ERROR(triton::core::MakeDirectory(tmp_folder, true)); + } // Specify contents to be downloaded std::set contents; @@ -694,36 +703,30 @@ S3FileSystem::LocalizePath( : JoinPath({(*localized)->Path(), s3_removed_path}); bool is_subdir; RETURN_IF_ERROR(IsDirectory(s3_fpath, &is_subdir)); - bool copy_subdir = - !fetch_subdir.empty() - ? s3_fpath == JoinPath({effective_path, fetch_subdir}) - : true; - if (is_subdir) { - if (copy_subdir) { - // Create local mirror of sub-directories + if (recursive && is_subdir) { + // Create local mirror of sub-directories #ifdef _WIN32 - int status = mkdir(const_cast(local_fpath.c_str())); + int status = mkdir(const_cast(local_fpath.c_str())); #else - int status = mkdir( - const_cast(local_fpath.c_str()), - S_IRUSR | S_IWUSR | S_IXUSR); + int status = mkdir( + const_cast(local_fpath.c_str()), + S_IRUSR | S_IWUSR | S_IXUSR); #endif - if (status == -1) { - return Status( - Status::Code::INTERNAL, - "Failed to create local folder: " + local_fpath + - ", errno:" + strerror(errno)); - } - - // Add sub-directories and deeper files to contents - std::set subdir_contents; - RETURN_IF_ERROR(GetDirectoryContents(s3_fpath, &subdir_contents)); - for (auto itr = subdir_contents.begin(); itr != subdir_contents.end(); - ++itr) { - contents.insert(JoinPath({s3_fpath, *itr})); - } + if (status == -1) { + return Status( + Status::Code::INTERNAL, + "Failed to create local folder: " + local_fpath + + ", errno:" + strerror(errno)); + } + + // Add sub-directories and deeper files to contents + std::set subdir_contents; + RETURN_IF_ERROR(GetDirectoryContents(s3_fpath, &subdir_contents)); + for (auto itr = subdir_contents.begin(); itr != subdir_contents.end(); + ++itr) { + contents.insert(JoinPath({s3_fpath, *itr})); } - } else { + } else if (!is_subdir) { // Create local copy of file std::string file_bucket, file_object; RETURN_IF_ERROR(ParsePath(s3_fpath, &file_bucket, &file_object)); diff --git a/src/model_config_utils.cc b/src/model_config_utils.cc index 050bfe294..729c243af 100644 --- a/src/model_config_utils.cc +++ b/src/model_config_utils.cc @@ -919,7 +919,7 @@ LocalizePythonBackendExecutionEnvironmentPath( // Localize the file std::shared_ptr localized_exec_env_path; RETURN_IF_ERROR(LocalizePath( - abs_exec_env_path, "" /*fetch_subdir*/, &localized_exec_env_path)); + abs_exec_env_path, true /*recursive*/, &localized_exec_env_path)); // Persist the localized temporary path (*localized_model_dir) ->other_localized_path.push_back(localized_exec_env_path); From 85119432233946e9e1e2f6ddb3de182aea85c4b6 Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Mon, 11 Sep 2023 19:26:48 -0700 Subject: [PATCH 3/8] Added comments --- src/filesystem/implementations/s3.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/filesystem/implementations/s3.h b/src/filesystem/implementations/s3.h index d39cabca5..b460bd2f2 100644 --- a/src/filesystem/implementations/s3.h +++ b/src/filesystem/implementations/s3.h @@ -654,7 +654,11 @@ S3FileSystem::LocalizePath( effective_path = path; } - // Create a local directory for s3 model store + // Create a local directory for s3 model store. + // If `mount_dir` or ENV variable are not set, + // creates a temporary directory under `/tmp` with the format: "folderXXXXXX". + // Otherwise, will create a folder under specified directory with the name + // indicated in path (i.e. everything after the last encounter of `/`). const char* env_mount_dir = std::getenv("TRITON_AWS_MOUNT_DIRECTORY"); std::string tmp_folder; if (mount_dir.empty() && env_mount_dir == nullptr) { From af94ad7ff82ac6a13afb58bc26617c11e5efbe8a Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Tue, 12 Sep 2023 12:13:34 -0700 Subject: [PATCH 4/8] Added allow_dir_exist to MakeDirectory + clarified comments --- src/filesystem/api.cc | 5 +++-- src/filesystem/api.h | 10 ++++++++-- src/filesystem/implementations/as.h | 7 +++++-- src/filesystem/implementations/common.h | 3 ++- src/filesystem/implementations/gcs.h | 7 +++++-- src/filesystem/implementations/local.h | 13 ++++++++----- src/filesystem/implementations/s3.h | 10 +++++++--- 7 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/filesystem/api.cc b/src/filesystem/api.cc index 2ec0710bc..477347619 100644 --- a/src/filesystem/api.cc +++ b/src/filesystem/api.cc @@ -619,11 +619,12 @@ ReadBinaryProto(const std::string& path, google::protobuf::MessageLite* msg) } Status -MakeDirectory(const std::string& dir, const bool recursive) +MakeDirectory( + const std::string& dir, const bool recursive, const bool allow_dir_exist) { std::shared_ptr fs; RETURN_IF_ERROR(fsm_.GetFileSystem(dir, fs)); - return fs->MakeDirectory(dir, recursive); + return fs->MakeDirectory(dir, recursive, allow_dir_exist); } Status diff --git a/src/filesystem/api.h b/src/filesystem/api.h index bd9efbd74..040889efa 100644 --- a/src/filesystem/api.h +++ b/src/filesystem/api.h @@ -158,7 +158,7 @@ Status LocalizePath( /// \param path The path of the directory or file. /// \param recursive If true, will fetch all sub-directories in /// the provided path. -/// \param mount_dir If specified, will use provided local directory +/// \param mount_dir If not empty, will use provided local directory /// for localization. /// \param localized Returns the LocalizedPath object /// representing the local copy of the path. @@ -205,8 +205,14 @@ Status ReadBinaryProto( /// \param dir The path to the directory. /// \param recursive Whether the parent directories will be created /// if not exist. +/// \param allow_dir_exist Controls the behavior on condition, +/// when `dir` already exists. If true and `dir` exists, returns success. +/// If false and `dir` exists, fails with `Status::Code::INTERNAL` +/// and reports errno EEXIST. Default value: false. /// \return Error status if the directory can't be created -Status MakeDirectory(const std::string& dir, const bool recursive); +Status MakeDirectory( + const std::string& dir, const bool recursive, + const bool allow_dir_exist = false); /// Create a temporary directory of the specified filesystem type. /// \param type The type of the filesystem. diff --git a/src/filesystem/implementations/as.h b/src/filesystem/implementations/as.h index bfad407c4..cf86f3952 100644 --- a/src/filesystem/implementations/as.h +++ b/src/filesystem/implementations/as.h @@ -94,7 +94,9 @@ class ASFileSystem : public FileSystem { Status WriteBinaryFile( const std::string& path, const char* contents, const size_t content_len) override; - Status MakeDirectory(const std::string& dir, const bool recursive) override; + Status MakeDirectory( + const std::string& dir, const bool recursive, + const bool allow_dir_exist) override; Status MakeTemporaryDirectory(std::string* temp_dir) override; Status DeletePath(const std::string& path) override; @@ -489,7 +491,8 @@ ASFileSystem::WriteBinaryFile( } Status -ASFileSystem::MakeDirectory(const std::string& dir, const bool recursive) +ASFileSystem::MakeDirectory( + const std::string& dir, const bool recursive, const bool allow_dir_exist) { return Status( Status::Code::UNSUPPORTED, diff --git a/src/filesystem/implementations/common.h b/src/filesystem/implementations/common.h index b2a236f37..8deceba10 100644 --- a/src/filesystem/implementations/common.h +++ b/src/filesystem/implementations/common.h @@ -92,7 +92,8 @@ class FileSystem { const std::string& path, const char* contents, const size_t content_len) = 0; virtual Status MakeDirectory( - const std::string& dir, const bool recursive) = 0; + const std::string& dir, const bool recursive, + const bool allow_dir_exist) = 0; virtual Status MakeTemporaryDirectory(std::string* temp_dir) = 0; virtual Status DeletePath(const std::string& path) = 0; }; diff --git a/src/filesystem/implementations/gcs.h b/src/filesystem/implementations/gcs.h index fa96b999f..88754c29b 100644 --- a/src/filesystem/implementations/gcs.h +++ b/src/filesystem/implementations/gcs.h @@ -83,7 +83,9 @@ class GCSFileSystem : public FileSystem { Status WriteBinaryFile( const std::string& path, const char* contents, const size_t content_len) override; - Status MakeDirectory(const std::string& dir, const bool recursive) override; + Status MakeDirectory( + const std::string& dir, const bool recursive, + const bool allow_dir_exist) override; Status MakeTemporaryDirectory(std::string* temp_dir) override; Status DeletePath(const std::string& path) override; @@ -474,7 +476,8 @@ GCSFileSystem::WriteBinaryFile( } Status -GCSFileSystem::MakeDirectory(const std::string& dir, const bool recursive) +GCSFileSystem::MakeDirectory( + const std::string& dir, const bool recursive, const bool allow_dir_exist) { return Status( Status::Code::UNSUPPORTED, diff --git a/src/filesystem/implementations/local.h b/src/filesystem/implementations/local.h index fc26368a9..d89d56ecb 100644 --- a/src/filesystem/implementations/local.h +++ b/src/filesystem/implementations/local.h @@ -57,7 +57,9 @@ class LocalFileSystem : public FileSystem { Status WriteBinaryFile( const std::string& path, const char* contents, const size_t content_len) override; - Status MakeDirectory(const std::string& dir, const bool recursive) override; + Status MakeDirectory( + const std::string& dir, const bool recursive, + const bool allow_dir_exist) override; Status MakeTemporaryDirectory(std::string* temp_dir) override; Status DeletePath(const std::string& path) override; }; @@ -248,7 +250,8 @@ LocalFileSystem::WriteBinaryFile( } Status -LocalFileSystem::MakeDirectory(const std::string& dir, const bool recursive) +LocalFileSystem::MakeDirectory( + const std::string& dir, const bool recursive, const bool allow_dir_exist) { #ifdef _WIN32 if (mkdir(dir.c_str()) == -1) @@ -256,14 +259,14 @@ LocalFileSystem::MakeDirectory(const std::string& dir, const bool recursive) if (mkdir(dir.c_str(), S_IRWXU) == -1) #endif { - // Return success if directory already exists - if (errno == EEXIST) { + // Return success if directory already exists and it is permitted + if (allow_dir_exist && errno == EEXIST) { return Status::Success; } // In all other cases only allow the error due to parent directory // does not exist, if 'recursive' is requested if ((errno == ENOENT) && (!dir.empty()) && recursive) { - RETURN_IF_ERROR(MakeDirectory(DirName(dir), recursive)); + RETURN_IF_ERROR(MakeDirectory(DirName(dir), recursive, allow_dir_exist)); // Retry the creation #ifdef _WIN32 if (mkdir(dir.c_str()) == -1) diff --git a/src/filesystem/implementations/s3.h b/src/filesystem/implementations/s3.h index b460bd2f2..67862aa0c 100644 --- a/src/filesystem/implementations/s3.h +++ b/src/filesystem/implementations/s3.h @@ -159,7 +159,9 @@ class S3FileSystem : public FileSystem { Status WriteBinaryFile( const std::string& path, const char* contents, const size_t content_len) override; - Status MakeDirectory(const std::string& dir, const bool recursive) override; + Status MakeDirectory( + const std::string& dir, const bool recursive, + const bool allow_dir_exist) override; Status MakeTemporaryDirectory(std::string* temp_dir) override; Status DeletePath(const std::string& path) override; @@ -668,7 +670,8 @@ S3FileSystem::LocalizePath( tmp_folder = mount_dir.empty() ? std::string(env_mount_dir) : mount_dir; tmp_folder = JoinPath({tmp_folder, path.substr(path.find_last_of('/') + 1)}); - RETURN_IF_ERROR(triton::core::MakeDirectory(tmp_folder, true)); + RETURN_IF_ERROR(triton::core::MakeDirectory( + tmp_folder, true /*recursive*/, true /*allow_dir_exist*/)); } // Specify contents to be downloaded @@ -780,7 +783,8 @@ S3FileSystem::WriteBinaryFile( } Status -S3FileSystem::MakeDirectory(const std::string& dir, const bool recursive) +S3FileSystem::MakeDirectory( + const std::string& dir, const bool recursive, const bool allow_dir_exist) { return Status( Status::Code::UNSUPPORTED, From 2bd9a250a1418d44f68b1e2e2597e11174ecd7be Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Tue, 12 Sep 2023 16:08:35 -0700 Subject: [PATCH 5/8] Apply similar changes to gcs --- src/filesystem/implementations/gcs.h | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/filesystem/implementations/gcs.h b/src/filesystem/implementations/gcs.h index 88754c29b..c46157b00 100644 --- a/src/filesystem/implementations/gcs.h +++ b/src/filesystem/implementations/gcs.h @@ -384,9 +384,23 @@ GCSFileSystem::LocalizePath( "GCS file localization not yet implemented " + path); } + // Create a local directory for s3 model store. + // If `mount_dir` or ENV variable are not set, + // creates a temporary directory under `/tmp` with the format: "folderXXXXXX". + // Otherwise, will create a folder under specified directory with the name + // indicated in path (i.e. everything after the last encounter of `/`). + const char* env_mount_dir = std::getenv("TRITON_GCS_MOUNT_DIRECTORY"); std::string tmp_folder; - RETURN_IF_ERROR( - triton::core::MakeTemporaryDirectory(FileSystemType::LOCAL, &tmp_folder)); + if (mount_dir.empty() && env_mount_dir == nullptr) { + RETURN_IF_ERROR(triton::core::MakeTemporaryDirectory( + FileSystemType::LOCAL, &tmp_folder)); + } else { + tmp_folder = mount_dir.empty() ? std::string(env_mount_dir) : mount_dir; + tmp_folder = + JoinPath({tmp_folder, path.substr(path.find_last_of('/') + 1)}); + RETURN_IF_ERROR(triton::core::MakeDirectory( + tmp_folder, true /*recursive*/, true /*allow_dir_exist*/)); + } localized->reset(new LocalizedPath(path, tmp_folder)); @@ -406,7 +420,7 @@ GCSFileSystem::LocalizePath( std::string local_fpath = JoinPath({(*localized)->Path(), gcs_removed_path}); RETURN_IF_ERROR(IsDirectory(gcs_fpath, &is_subdir)); - if (is_subdir) { + if (recursive && is_subdir) { // Create local mirror of sub-directories #ifdef _WIN32 int status = mkdir(const_cast(local_fpath.c_str())); @@ -429,7 +443,7 @@ GCSFileSystem::LocalizePath( ++itr) { contents.insert(JoinPath({gcs_fpath, *itr})); } - } else { + } else if (!is_subdir) { // Create local copy of file std::string file_bucket, file_object; RETURN_IF_ERROR(ParsePath(gcs_fpath, &file_bucket, &file_object)); From 6903e1c2ecedc4c01664e7d6cb3155f5fe2d3d29 Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Tue, 12 Sep 2023 17:02:50 -0700 Subject: [PATCH 6/8] Apply similar changes to AZURE --- src/filesystem/implementations/as.h | 56 +++++++++++++++++------------ 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/src/filesystem/implementations/as.h b/src/filesystem/implementations/as.h index cf86f3952..100ae51ed 100644 --- a/src/filesystem/implementations/as.h +++ b/src/filesystem/implementations/as.h @@ -116,7 +116,7 @@ class ASFileSystem : public FileSystem { Status DownloadFolder( const std::string& container, const std::string& path, - const std::string& dest); + const std::string& dest, const bool recursive); std::shared_ptr client_; re2::RE2 as_regex_; @@ -392,7 +392,7 @@ ASFileSystem::FileExists(const std::string& path, bool* exists) Status ASFileSystem::DownloadFolder( const std::string& container, const std::string& path, - const std::string& dest) + const std::string& dest, const bool recursive) { auto container_client = client_->GetBlobContainerClient(container); auto func = [&](const std::vector& blobs, @@ -408,17 +408,20 @@ ASFileSystem::DownloadFolder( "Failed to download file at " + blob_item.Name + ":" + ex.what()); } } - for (const auto& directory_item : blob_prefixes) { - const auto& local_path = JoinPath({dest, BaseName(directory_item)}); - int status = mkdir( - const_cast(local_path.c_str()), S_IRUSR | S_IWUSR | S_IXUSR); - if (status == -1) { - return Status( - Status::Code::INTERNAL, - "Failed to create local folder: " + local_path + - ", errno:" + strerror(errno)); + if (recursive) { + for (const auto& directory_item : blob_prefixes) { + const auto& local_path = JoinPath({dest, BaseName(directory_item)}); + int status = mkdir( + const_cast(local_path.c_str()), S_IRUSR | S_IWUSR | S_IXUSR); + if (status == -1 && errno != EEXIST) { + return Status( + Status::Code::INTERNAL, + "Failed to create local folder: " + local_path + + ", errno:" + strerror(errno)); + } + RETURN_IF_ERROR( + DownloadFolder(container, directory_item, local_path, recursive)); } - RETURN_IF_ERROR(DownloadFolder(container, directory_item, local_path)); } return Status::Success; }; @@ -445,21 +448,30 @@ ASFileSystem::LocalizePath( "AS file localization not yet implemented " + path); } - std::string folder_template = "/tmp/folderXXXXXX"; - char* tmp_folder = mkdtemp(const_cast(folder_template.c_str())); - if (tmp_folder == nullptr) { - return Status( - Status::Code::INTERNAL, - "Failed to create local temp folder: " + folder_template + - ", errno:" + strerror(errno)); + // Create a local directory for s3 model store. + // If `mount_dir` or ENV variable are not set, + // creates a temporary directory under `/tmp` with the format: "folderXXXXXX". + // Otherwise, will create a folder under specified directory with the name + // indicated in path (i.e. everything after the last encounter of `/`). + const char* env_mount_dir = std::getenv("TRITON_AZURE_MOUNT_DIRECTORY"); + std::string tmp_folder; + if (mount_dir.empty() && env_mount_dir == nullptr) { + RETURN_IF_ERROR(triton::core::MakeTemporaryDirectory( + FileSystemType::LOCAL, &tmp_folder)); + } else { + tmp_folder = mount_dir.empty() ? std::string(env_mount_dir) : mount_dir; + tmp_folder = + JoinPath({tmp_folder, path.substr(path.find_last_of('/') + 1)}); + RETURN_IF_ERROR(triton::core::MakeDirectory( + tmp_folder, true /*recursive*/, true /*allow_dir_exist*/)); } - localized->reset(new LocalizedPath(path, tmp_folder)); - std::string dest(folder_template); + localized->reset(new LocalizedPath(path, tmp_folder)); + std::string dest(tmp_folder); std::string container, blob; RETURN_IF_ERROR(ParsePath(path, &container, &blob)); - return DownloadFolder(container, blob, dest); + return DownloadFolder(container, blob, dest, recursive); } Status From c5ded7e9a5541b33d4bc84a051de0e2e07bb0d81 Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Tue, 12 Sep 2023 20:52:45 -0700 Subject: [PATCH 7/8] Add allow_dir_exist to AS::DownloadFolder --- src/filesystem/implementations/as.h | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/filesystem/implementations/as.h b/src/filesystem/implementations/as.h index 100ae51ed..64d5d38c2 100644 --- a/src/filesystem/implementations/as.h +++ b/src/filesystem/implementations/as.h @@ -116,7 +116,8 @@ class ASFileSystem : public FileSystem { Status DownloadFolder( const std::string& container, const std::string& path, - const std::string& dest, const bool recursive); + const std::string& dest, const bool recursive, + const bool allow_dir_exist); std::shared_ptr client_; re2::RE2 as_regex_; @@ -392,7 +393,7 @@ ASFileSystem::FileExists(const std::string& path, bool* exists) Status ASFileSystem::DownloadFolder( const std::string& container, const std::string& path, - const std::string& dest, const bool recursive) + const std::string& dest, const bool recursive, const bool allow_dir_exist) { auto container_client = client_->GetBlobContainerClient(container); auto func = [&](const std::vector& blobs, @@ -413,14 +414,15 @@ ASFileSystem::DownloadFolder( const auto& local_path = JoinPath({dest, BaseName(directory_item)}); int status = mkdir( const_cast(local_path.c_str()), S_IRUSR | S_IWUSR | S_IXUSR); - if (status == -1 && errno != EEXIST) { + if (status == -1 && !(allow_dir_exist && errno == EEXIST)) { return Status( Status::Code::INTERNAL, "Failed to create local folder: " + local_path + ", errno:" + strerror(errno)); } - RETURN_IF_ERROR( - DownloadFolder(container, directory_item, local_path, recursive)); + RETURN_IF_ERROR(DownloadFolder( + container, directory_item, local_path, recursive, + true /*allow_dir_exist*/)); } } return Status::Success; @@ -471,7 +473,8 @@ ASFileSystem::LocalizePath( std::string dest(tmp_folder); std::string container, blob; RETURN_IF_ERROR(ParsePath(path, &container, &blob)); - return DownloadFolder(container, blob, dest, recursive); + return DownloadFolder( + container, blob, dest, recursive, true /*allow_dir_exist*/); } Status From d35a79409da5b9a80d16e9e649353ee71b5b11dd Mon Sep 17 00:00:00 2001 From: oandreeva-nv Date: Wed, 13 Sep 2023 17:09:46 -0700 Subject: [PATCH 8/8] Adjusted azure mkdir for Win --- src/filesystem/implementations/as.h | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/filesystem/implementations/as.h b/src/filesystem/implementations/as.h index 64d5d38c2..ac055c14d 100644 --- a/src/filesystem/implementations/as.h +++ b/src/filesystem/implementations/as.h @@ -412,17 +412,10 @@ ASFileSystem::DownloadFolder( if (recursive) { for (const auto& directory_item : blob_prefixes) { const auto& local_path = JoinPath({dest, BaseName(directory_item)}); - int status = mkdir( - const_cast(local_path.c_str()), S_IRUSR | S_IWUSR | S_IXUSR); - if (status == -1 && !(allow_dir_exist && errno == EEXIST)) { - return Status( - Status::Code::INTERNAL, - "Failed to create local folder: " + local_path + - ", errno:" + strerror(errno)); - } + RETURN_IF_ERROR(triton::core::MakeDirectory( + local_path, recursive, allow_dir_exist)); RETURN_IF_ERROR(DownloadFolder( - container, directory_item, local_path, recursive, - true /*allow_dir_exist*/)); + container, directory_item, local_path, recursive, allow_dir_exist)); } } return Status::Success; @@ -450,7 +443,7 @@ ASFileSystem::LocalizePath( "AS file localization not yet implemented " + path); } - // Create a local directory for s3 model store. + // Create a local directory for azure model store. // If `mount_dir` or ENV variable are not set, // creates a temporary directory under `/tmp` with the format: "folderXXXXXX". // Otherwise, will create a folder under specified directory with the name