Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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
16 changes: 14 additions & 2 deletions src/filesystem/api.cc
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
18 changes: 17 additions & 1 deletion src/filesystem/api.h
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 specified, will use provided local directory
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean "If not empty" rather than "If specified"? I do not think the caller can skip this parameter.

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'll clarify, thank you for suggestion

/// 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
6 changes: 4 additions & 2 deletions src/filesystem/implementations/as.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ 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;
Expand Down Expand Up @@ -424,7 +425,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 Down
4 changes: 3 additions & 1 deletion src/filesystem/implementations/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ 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(
Expand Down
6 changes: 4 additions & 2 deletions src/filesystem/implementations/gcs.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ 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;
Expand Down Expand Up @@ -363,7 +364,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 Down
14 changes: 10 additions & 4 deletions src/filesystem/implementations/local.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ class LocalFileSystem : 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;
Expand Down Expand Up @@ -204,7 +205,8 @@ LocalFileSystem::ReadTextFile(const std::string& path, std::string* contents)

Status
LocalFileSystem::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)
{
// For local file system we don't actually need to download the
// directory or file. We use it in place.
Expand Down Expand Up @@ -254,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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be a function argument on whether file exist is allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I'll add it

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
Expand Down
28 changes: 21 additions & 7 deletions src/filesystem/implementations/s3.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ class S3FileSystem : 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;
Expand Down Expand Up @@ -628,7 +629,8 @@ S3FileSystem::ReadTextFile(const std::string& path, std::string* contents)

Status
S3FileSystem::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)
{
// Check if the directory or file exists
bool exists;
Expand All @@ -652,10 +654,22 @@ S3FileSystem::LocalizePath(
effective_path = path;
}

// Create temporary directory
// 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;
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<std::string> contents;
Expand Down Expand Up @@ -693,7 +707,7 @@ S3FileSystem::LocalizePath(
: JoinPath({(*localized)->Path(), s3_removed_path});
bool is_subdir;
RETURN_IF_ERROR(IsDirectory(s3_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 @@ -716,7 +730,7 @@ S3FileSystem::LocalizePath(
++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));
Expand Down
4 changes: 2 additions & 2 deletions src/model_config_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -918,8 +918,8 @@ LocalizePythonBackendExecutionEnvironmentPath(
model_path_slash) {
// Localize the file
std::shared_ptr<LocalizedPath> localized_exec_env_path;
RETURN_IF_ERROR(
LocalizePath(abs_exec_env_path, &localized_exec_env_path));
RETURN_IF_ERROR(LocalizePath(
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);
Expand Down