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

oandreeva-nv · 2023-09-07T21:33:52Z

To avoid unnecessary copy during localization, I added fetch_subdir parameter to LocalizePath: If specified, will only download provided sub directory, otherwise all subdirectories will be downloaded. Does not affect files individual files, located under path.

[WIP] tag due to the following. When we agree on the proposed solution, I will extend it to GCS and AS.

What was seen before:

root@oandreeva-dt:/opt/tritonserver# tree /tmp
/tmp
|-- folderDkrpXO
|   |-- 1
|   |   `-- model.pt
|   |-- 2
|   |   `-- model.pt
|   |-- 3
|   |   `-- model.pt
|   |-- config.pbtxt
|   `-- output0_labels.txt
|-- folderVuS3x0
|   |-- 1
|   |   `-- model.graphdef
|   |-- 2
|   |   `-- model.graphdef
|   |-- 3
|   |   `-- model.graphdef
|   |-- config.pbtxt
|   `-- output0_labels.txt
|-- folderfQ1yf3
|   |-- 1
|   |   `-- model.pt
|   |-- 2
|   |   `-- model.pt
|   |-- 3
|   |   `-- model.pt
|   |-- config.pbtxt
|   `-- output0_labels.txt
`-- folderwxZ6hr
    |-- 1
    |   `-- model.graphdef
    |-- 2
    |   `-- model.graphdef
    |-- 3
    |   `-- model.graphdef
    |-- config.pbtxt
    `-- output0_labels.txt

With this fix:

    root@oandreeva-dt:/opt/tritonserver# tree /tmp
/tmp
|-- folder9sfzV0
|   |-- 1
|   |   `-- model.pt
|   |-- config.pbtxt
|   `-- output0_labels.txt
|-- folderHBt0pT
|   |-- 3
|   |   `-- model.pt
|   |-- config.pbtxt
|   `-- output0_labels.txt
|-- folderZbfFyR
|   |-- 3
|   |   `-- model.graphdef
|   |-- config.pbtxt
|   `-- output0_labels.txt
|-- folderryK9pq
|   |-- 1
|   |   `-- model.graphdef
|   |-- config.pbtxt
|   `-- output0_labels.txt

kthui · 2023-09-07T23:39:54Z

src/filesystem/api.h

 /// 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<LocalizedPath>* localized);
+    const std::string& path, const std::string& fetch_subdir,
+    std::shared_ptr<LocalizedPath>* localized);


I think the interface is insufficiently generic for functions at the level of LocalizePath(), because LocalizePath() should be available to any callers who want to localize any path on the cloud, but fetch_subdir imposes a limitation on the caller to either fetch all sub-directories within the path or only the sub-directory specified. The caller may not "choose" which sub-directories to fetch or not fetch. let alone choosing sub-directories within a sub-directory.

I think there are many different generic design choices. For example:

Use bool recursive instead of string fetch_subdir. The caller can choose not fetching any sub-directories with recursive = false or all sub-directories with recursive = true. Then, the problem can be accomplished by first fetching the given model path with recursive = false (i.e. /repo/model) and then the model path with version with recursive = true (i.e. /repo/model/1). However, this will require the LocalizedPath object to be able to join the model path and model version path localization objects into one localization object.

Allow the caller to specify a list of sub-paths to localize (or not localize). The "complicated" structure of sub-paths allows for fetching different combinations of sub-directories in a single call, which eliminates the need for joining model path and model version path localization objects, but the "complicated" structure is less intuitive for the caller to use and may not be easier to implement than the first design choice.

However, the interface accomplishes exactly what we want to solve at this time. I will defer to @GuanLuo and @nnshah1 to decide.

However, this will require the LocalizedPath object to be able to join the model path and model version path localization objects into one localization object.

Do we? If I understand correctly, versioned path is set here:

core/src/backend_model.cc

Lines 113 to 114 in a23786c

const auto version_path =

JoinPath({localized_model_path, std::to_string(version)});

so instead of

const auto version_path = JoinPath({localized_model_path, std::to_string(version)});

we would do something like:

std::shared_ptr<LocalizedPath> version_path; RETURN_IF_ERROR(LocalizePath(path/to/version, recursive=false, &version_path));

or am I missing something?

I am thinking something like:

std::string model_path = "/repo/model_dir"; std::string model_version_path = "/repo/model_dir/1"; std::shared_ptr<LocalizedPath> model_localized; std::shared_ptr<LocalizedPath> model_version_localized; LocalizePath(model_path, recursive=false, &model_localized); LocalizePath(model_version_path, recursive=true, &model_version_localized); model_localized->Include(model_version_localized);

The idea behind LocalizedPath::Include() is to join

model_tmp | `--- config.pbtxt

and

model_version_tmp | `--- model.py

to

model_tmp | `--- config.pbtxt | `--- 1 | `--- model.py

on the local disk.

I actually am missing something. Even though I like option 1 suggestion, localization in this case will happen to another directory. Meaning, at current stage every call to LocalizePath creates a new temporary directory. Thus if we separately will try to load model and its version, they will be copied in 2 different directories. So unless we want to change that, option 1 is not the best option.

I think it would be ok to only implement the base case above, and throw an error if the caller is trying to join/include anything more complicated.

Based on offline discussion, with @kthui, we can add one more parameter location to control where to localize provided path, i.e. something like

LocalizePath(model_version_path, recursive-true/false, location=<string>, &model_version_localized);

Then we can control where version directory will be localized , and if location is empty, we create a new, temporary directory.

This will also help with the follow up ticket.

Meaning, at current stage every call to LocalizePath creates a new temporary directory. Thus if we separately will try to load model and its version, they will be copied in 2 different directories. So unless we want to change that, option 1 is not the best option.

I am thinking on the same direction as LocalizedPath::Include() that if we want to have a generic way to localizing directories (with customization on fetching subset of contents / recurrsiveness), we want to make LocalizedPath a "filler" class: as on calling fs::LocalizePath, it returns an fully localized object (current) or empty object (with local root path assigned). Then if user want to customize the localization, they can call:

LocalizedPath::Include("sub/path/content", recursive);

Although I think it then the process is convoluted that there is a iterative calls of GetDirectorySubdirs/Files and LocalizedPath::Include

Note that this is really an internal API (unless we want to expose file system API in the future) so we are free to modify the interface as frequent as we want and don't need to worry about being generic. If localizing model directory is somewhat specific, we may just introduce a LocalizeModelDirectory method to fit the specific need.

Because when you look at what to be localized for a model version, it is actually "everything in the directory except for the unwanted version subdirectories (integer-named directories)", that is tricky to express it with some generic language.

oandreeva-nv · 2023-09-12T03:03:06Z

@kthui, @GuanLuo and @nnshah1. It is ready for re-review.

I didn't implement LocalizedPath::Include, since it can be done as a separate follow-up PR with rename for Linux and MoveFile for Windows with concerns regarding

convoluted that there is a iterative calls of GetDirectorySubdirs/Files and LocalizedPath::Include

properly considered. As of now, I think specifying mount_dir of a model's localized path when calling LocalizePath( version_path, true /*recursive*/, localized_model_dir->Path(), &localized_version_dir) should be sufficient. To make sure that localized content for version persists, I add localized_version_dir to localized_model_dir->other_localized_path:
localized_model_dir->other_localized_path.push_back(localized_version_dir);

Change in LocalFileSystem::MakeDirectory : I've added one more allowed errno, i.e. File Exists, since it make sence to use LocalFileSystem::MakeDirectory in LocalizePath, and for multiple versions of the same model, creation of a folder with the same name (i.e. model name) occurs. To clarify, when we use mkdtemp then instead of model names folderXXXX is used, so they are differentiable. With mount directory specified, a folder with model's name will be created on the first call of LocalizePath . Thus, when the next version of this model is being processed, this folder will already exist.

Regarding environmental variable. Current name is TRITON_AWS_MOUNT_DIRECTORY. I think it would make sense to create filesystem type-specific env names for gcs and as as well. Open for discussion.
I thought about adding the ability to specify this location in credentials file, but I think since local mount directory is not really a credential, it is not the best location + code's logic reads contents into S3Credential struct, it would be strange to have mount_dir member in this file, thus it would need some other wrapping structure + more code change. We can do it later upon user request.

Finally, current requirements for mount directory:

Should exist before
Should not contain same elements (folders and files) as remote. Otherwise, upon stopping server, all same contents will be deleted from the local directory. Let me know if this is a reasonable ask.

Current look: bucket is mounted to /tmp/aws:

/tmp
`-- aws
    |-- graphdef_float32_float32_float32
    |   |-- 1
    |   |   `-- model.graphdef
    |   |-- 3
    |   |   `-- model.graphdef
    |   |-- config.pbtxt
    |   `-- output0_labels.txt
    `-- libtorch_float32_float32_float32
        |-- 1
        |   `-- model.pt
        |-- 3
        |   `-- model.pt
        |-- config.pbtxt
        `-- output0_labels.txt

oandreeva-nv · 2023-09-12T18:05:09Z

src/filesystem/api.cc

 {
  std::shared_ptr<FileSystem> fs;
  RETURN_IF_ERROR(fsm_.GetFileSystem(path, fs));
-  return fs->LocalizePath(path, localized);
+  return fs->LocalizePath(path, recursive, "", localized);


this (L565-L567) can potentially be shortened to

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

GuanLuo

Minor comments

GuanLuo · 2023-09-12T18:07:22Z

src/filesystem/implementations/gcs.h

@@ -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,


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()

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

GuanLuo · 2023-09-12T18:12:48Z

src/filesystem/implementations/local.h

-    // Only allow the error due to parent directory does not exist
-    // if 'recursive' is requested
+    // Return success if directory already exists
+    if (errno == EEXIST) {


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

Good idea, I'll add it

kthui · 2023-09-12T18:30:10Z

src/filesystem/api.h

+/// \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


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

I'll clarify, thank you for suggestion

GuanLuo · 2023-09-13T23:36:42Z

src/filesystem/implementations/as.h

+    if (recursive) {
+      for (const auto& directory_item : blob_prefixes) {
+        const auto& local_path = JoinPath({dest, BaseName(directory_item)});
+        int status = mkdir(


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

right, for Windows. good catch

rmccorm4 · 2023-09-18T17:57:37Z

src/filesystem/implementations/as.h

+    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*/));


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

Finally, current requirements for mount directory:

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?

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

rmccorm4 · 2023-09-18T18:00:54Z

src/filesystem/implementations/as.h

+  const char* env_mount_dir = std::getenv("TRITON_AZURE_MOUNT_DIRECTORY");
+  std::string tmp_folder;
+  if (mount_dir.empty() && env_mount_dir == nullptr) {


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.

Adding ability to specify subdirectory to download for LocalizePath

e5a40b7

oandreeva-nv requested review from GuanLuo, kthui and nnshah1 September 7, 2023 21:33

kthui reviewed Sep 7, 2023

View reviewed changes

oandreeva-nv added 2 commits September 11, 2023 17:57

Generalized implementation of : added recursive flag and mount_dir

8474fa0

Added comments

8511943

oandreeva-nv requested a review from kthui September 12, 2023 03:03

oandreeva-nv commented Sep 12, 2023

View reviewed changes

GuanLuo previously approved these changes Sep 12, 2023

View reviewed changes

kthui previously approved these changes Sep 12, 2023

View reviewed changes

oandreeva-nv added 3 commits September 12, 2023 12:28

Added allow_dir_exist to MakeDirectory + clarified comments

af94ad7

Apply similar changes to gcs

2bd9a25

Apply similar changes to AZURE

6903e1c

oandreeva-nv dismissed stale reviews from kthui and GuanLuo via 6903e1c September 13, 2023 02:02

Add allow_dir_exist to AS::DownloadFolder

c5ded7e

oandreeva-nv changed the title ~~[WIP] Adding ability to specify subdirectory to download for LocalizePath~~ Adding ability to specify subdirectory to download for LocalizePath Sep 13, 2023

oandreeva-nv requested review from kthui and GuanLuo September 13, 2023 17:36

oandreeva-nv mentioned this pull request Sep 13, 2023

Tests and docs for ability to specify subdirectory to download for LocalizePath triton-inference-server/server#6308

Merged

GuanLuo reviewed Sep 13, 2023

View reviewed changes

Adjusted azure mkdir for Win

d35a794

oandreeva-nv requested a review from GuanLuo September 14, 2023 00:15

GuanLuo approved these changes Sep 14, 2023

View reviewed changes

kthui approved these changes Sep 14, 2023

View reviewed changes

rmccorm4 reviewed Sep 18, 2023

View reviewed changes

oandreeva-nv changed the title ~~Adding ability to specify subdirectory to download for LocalizePath~~ [WIP] Fixing extra download for filesystem's localization Sep 18, 2023

oandreeva-nv mentioned this pull request Sep 18, 2023

Specify non-random location for filesystem's temporary cache #266

Merged

oandreeva-nv marked this pull request as draft February 12, 2024 23:11

dyastremsky mentioned this pull request May 22, 2024

Allow for explicit folder name when specifying where remote model repository will be downloaded. triton-inference-server/server#6644

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oandreeva-nv commented Sep 7, 2023

kthui Sep 7, 2023

oandreeva-nv Sep 7, 2023 •

edited

Loading

kthui Sep 8, 2023 •

edited

Loading

oandreeva-nv Sep 8, 2023

kthui Sep 8, 2023

oandreeva-nv Sep 8, 2023

GuanLuo Sep 8, 2023

GuanLuo Sep 8, 2023 •

edited

Loading

oandreeva-nv commented Sep 12, 2023 •

edited

Loading

oandreeva-nv Sep 12, 2023

GuanLuo left a comment

GuanLuo Sep 12, 2023

oandreeva-nv Sep 12, 2023

GuanLuo Sep 12, 2023

oandreeva-nv Sep 12, 2023

kthui Sep 12, 2023

oandreeva-nv Sep 12, 2023

GuanLuo Sep 13, 2023

oandreeva-nv Sep 13, 2023

oandreeva-nv Sep 14, 2023

rmccorm4 Sep 18, 2023

oandreeva-nv Sep 18, 2023

rmccorm4 Sep 18, 2023

	const auto version_path =
	JoinPath({localized_model_path, std::to_string(version)});

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

Are you sure you want to change the base?

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

Conversation

oandreeva-nv commented Sep 7, 2023

Choose a reason for hiding this comment

oandreeva-nv Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

kthui Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuanLuo Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

oandreeva-nv commented Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

GuanLuo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandreeva-nv Sep 7, 2023 •

edited

Loading

kthui Sep 8, 2023 •

edited

Loading

GuanLuo Sep 8, 2023 •

edited

Loading

oandreeva-nv commented Sep 12, 2023 •

edited

Loading