Skip to content
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.

Sign up for GitHub

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

[WIP] Fixing extra download for filesystem's localization #254

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

[WIP] Fixing extra download for filesystem's localization #254

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e5a40b7
Adding ability to specify subdirectory to download for `LocalizePath`
oandreeva-nv Sep 6, 2023
8474fa0
Generalized implementation of : added recursive flag and mount_dir
oandreeva-nv Sep 12, 2023
8511943
Added comments
oandreeva-nv Sep 12, 2023
af94ad7
Added allow_dir_exist to MakeDirectory + clarified comments
oandreeva-nv Sep 12, 2023
2bd9a25
Apply similar changes to gcs
oandreeva-nv Sep 12, 2023
6903e1c
Apply similar changes to AZURE
oandreeva-nv Sep 13, 2023
c5ded7e
Add allow_dir_exist to AS::DownloadFolder
oandreeva-nv Sep 13, 2023
d35a794
Adjusted azure mkdir for Win
oandreeva-nv Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/backend_model.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,19 @@ 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'. This model holds a handle to
// the localized content so that it persists as long as the model is loaded.
std::shared_ptr<LocalizedPath> localized_model_dir;
RETURN_IF_ERROR(LocalizePath(model_path, &localized_model_dir));
RETURN_IF_ERROR(
LocalizePath(model_path, false /*recursive*/, &localized_model_dir));

const auto version_path = JoinPath({model_path, std::to_string(version)});
std::shared_ptr<LocalizedPath> 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)
Expand Down Expand Up @@ -110,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<std::string> 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;
Expand Down
21 changes: 17 additions & 4 deletions src/filesystem/api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,11 +558,23 @@ ReadTextProto(const std::string& path, google::protobuf::Message* msg)
}

Status
LocalizePath(const std::string& path, std::shared_ptr<LocalizedPath>* localized)
LocalizePath(
const std::string& path, const bool recursive,
std::shared_ptr<LocalizedPath>* localized)
{
std::shared_ptr<FileSystem> fs;
RETURN_IF_ERROR(fsm_.GetFileSystem(path, fs));
return fs->LocalizePath(path, localized);
return fs->LocalizePath(path, recursive, "", localized);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this (L565-L567) can potentially be shortened to

retrurn LocalizePath(path, recursive, "", localized)

}

Status
LocalizePath(
const std::string& path, const bool recursive, const std::string& mount_dir,
std::shared_ptr<LocalizedPath>* localized)
{
std::shared_ptr<FileSystem> fs;
RETURN_IF_ERROR(fsm_.GetFileSystem(path, fs));
return fs->LocalizePath(path, recursive, mount_dir, localized);
}

Status
Expand Down Expand Up @@ -607,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<FileSystem> fs;
RETURN_IF_ERROR(fsm_.GetFileSystem(dir, fs));
return fs->MakeDirectory(dir, recursive);
return fs->MakeDirectory(dir, recursive, allow_dir_exist);
}

Status
Expand Down
26 changes: 24 additions & 2 deletions src/filesystem/api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,27 @@ 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 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, std::shared_ptr<LocalizedPath>* localized);
const std::string& path, const bool recursive,
std::shared_ptr<LocalizedPath>* 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 not empty, 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<LocalizedPath>* localized);

/// Write a string to a file.
/// \param path The path of the file.
Expand Down Expand Up @@ -189,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.
Expand Down
72 changes: 46 additions & 26 deletions src/filesystem/implementations/as.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,17 @@ class ASFileSystem : public FileSystem {
const std::string& path, std::set<std::string>* files) override;
Status ReadTextFile(const std::string& path, std::string* contents) override;
Status LocalizePath(
const std::string& path,
const std::string& path, const bool recursive,
const std::string& mount_dir,
std::shared_ptr<LocalizedPath>* localized) override;
Status WriteTextFile(
const std::string& path, const std::string& contents) override;
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;

Expand All @@ -113,7 +116,8 @@ 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,
const bool allow_dir_exist);

std::shared_ptr<asb::BlobServiceClient> client_;
re2::RE2 as_regex_;
Expand Down Expand Up @@ -389,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 std::string& dest, const bool recursive, const bool allow_dir_exist)
{
auto container_client = client_->GetBlobContainerClient(container);
auto func = [&](const std::vector<asb::Models::BlobItem>& blobs,
Expand All @@ -405,17 +409,21 @@ 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<char*>(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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the FileSystem wrapped API be used here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, for Windows. good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

const_cast<char*>(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(DownloadFolder(
container, directory_item, local_path, recursive,
true /*allow_dir_exist*/));
}
RETURN_IF_ERROR(DownloadFolder(container, directory_item, local_path));
}
return Status::Success;
};
Expand All @@ -424,7 +432,8 @@ ASFileSystem::DownloadFolder(

Status
ASFileSystem::LocalizePath(
const std::string& path, std::shared_ptr<LocalizedPath>* localized)
const std::string& path, const bool recursive, const std::string& mount_dir,
std::shared_ptr<LocalizedPath>* localized)
{
bool exists;
RETURN_IF_ERROR(FileExists(path, &exists));
Expand All @@ -441,21 +450,31 @@ ASFileSystem::LocalizePath(
"AS file localization not yet implemented " + path);
}

std::string folder_template = "/tmp/folderXXXXXX";
char* tmp_folder = mkdtemp(const_cast<char*>(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) {
Comment on lines +451 to +453
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's already a follow-up ticket, I'd like to see use of some helper function like this one that safely checks if the env var is set or is nullptr, and just returns a std::string. This helper could probably just take an env var name as an argument to condense the usage.

Currently this if/else is OK, but if the conditions were to be tweaked in the future, there is some risk for std::string(env_mount_dir) when it is a 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*/));
Comment on lines +457 to +461
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oandreeva-nv just checking my understanding, I see your comment saying:

Finally, current requirements for mount directory:

  1. Should exist before

But in the code I'm seeing calls to MakeDirectory(), which I'm inferring would create the directory if it doesn't exist.

So to clarify, is the mount directory required to exist before server startup? Or will Triton create it on demand, similar to tmp folder workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will clarify this in a follow-up commit. This PR would need more de-bugging and testing

}
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, true /*allow_dir_exist*/);
}

Status
Expand Down Expand Up @@ -487,7 +506,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,
Expand Down
7 changes: 5 additions & 2 deletions src/filesystem/implementations/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,17 @@ class FileSystem {
virtual Status ReadTextFile(
const std::string& path, std::string* contents) = 0;
virtual Status LocalizePath(
const std::string& path, std::shared_ptr<LocalizedPath>* localized) = 0;
const std::string& path, const bool recursive,
const std::string& mount_dir,
std::shared_ptr<LocalizedPath>* localized) = 0;
virtual Status WriteTextFile(
const std::string& path, const std::string& contents) = 0;
virtual Status WriteBinaryFile(
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;
};
Expand Down
35 changes: 27 additions & 8 deletions src/filesystem/implementations/gcs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,17 @@ class GCSFileSystem : public FileSystem {
const std::string& path, std::set<std::string>* files) override;
Status ReadTextFile(const std::string& path, std::string* contents) override;
Status LocalizePath(
const std::string& path,
const std::string& path, const bool recursive,
const std::string& mount_dir,
std::shared_ptr<LocalizedPath>* localized) override;
Status WriteTextFile(
const std::string& path, const std::string& contents) override;
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;

Expand Down Expand Up @@ -363,7 +366,8 @@ GCSFileSystem::ReadTextFile(const std::string& path, std::string* contents)

Status
GCSFileSystem::LocalizePath(
const std::string& path, std::shared_ptr<LocalizedPath>* localized)
const std::string& path, const bool recursive, const std::string& mount_dir,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you going to implement the new API in other cloud file system in this PR as well? If not, should return error if !mount_dir.empty()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wanted to first get an agreement on the implementation for s3. I'll populate it to GCS and AS in the next commit for easier review

std::shared_ptr<LocalizedPath>* localized)
{
bool exists;
RETURN_IF_ERROR(FileExists(path, &exists));
Expand All @@ -380,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));

Expand All @@ -402,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<char*>(local_fpath.c_str()));
Expand All @@ -425,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));
Expand Down Expand Up @@ -472,7 +490,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,
Expand Down
Loading