[2/N] Use std::filesystem #152586

cyyever · 2025-05-01T04:10:01Z

Use std::filesystem in most inductor code. This is follow-up of #152288 .
The check of std::filesystem::create_directories has been fixed because it may return false when the directory to create already exists.
cc @H-Huang @awgu @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k

pytorch-bot · 2025-05-01T04:10:04Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/152586

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (2 Unrelated Failures)

As of commit d2f2e86 with merge base 61aa77e ():

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

inductor / cuda12.6-py3.10-gcc9-sm86 / test (inductor_torchbench, 1, 2, ephemeral.linux.g5.4xlarge.nvidia.gpu) (gh) (trunk failure)
hf_BigBird

UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:

inductor / unit-test / cuda12.6-py3.10-gcc9-sm86 / test (inductor_cpp_wrapper, 1, 2, ephemeral.linux.g5.4xlarge.nvidia.gpu) (gh) (#152916)
[ FAILED ] AotInductorTest.BasicTestCuda

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Skylion007 · 2025-05-02T13:06:53Z

torch/csrc/distributed/c10d/UCCTracing.cpp

@@ -42,7 +42,8 @@ void ProcessGroupUCCLogger::flushComms(int rank, int world_size) {
  std::filesystem::path trace_filename =
      fullpath / fmt::format("rank{}.json", rank);
  std::error_code ec{};
-  if (!std::filesystem::create_directories(fullpath, ec)) {
+  std::filesystem::create_directories(fullpath, ec);


What about if create_directory fails because there is a currently a file a the location. Nothing is printed then

The below if should be relying on the error code, right?

Use std::filesystem::is_directory is more robust.

Skylion007 · 2025-05-02T13:07:57Z

torch/csrc/inductor/aoti_torch/shim_common.cpp

-    if (!create_directories(tmp_folder)) {
+    std::error_code ec{};
+    std::filesystem::create_directories(tmp_folder, ec);
+    if (!std::filesystem::is_directory(tmp_folder)) {


Same problem here

cyyever · 2025-05-02T16:07:03Z

@Skylion007 create_directories may return false if the directory exists. In such cases, we can't rely on std::error_code having a failure value !=0 since the error code is platform dependent (C++ doesn't define a successful value and we have to consider non-UNIX platforms). The most reliable way is to check by std::filesystem::is_directory. If that fails, then std::filesystem:: create_directories should have failed (except for rare race-conditions that some action was applied to the dir externally between the two calls), then by API conversion, std::error_code should record the reason. This line of reasoning leads to the code changes.

In summary, there is not platform independent system call values of success and failure... And it is unlikely that the current implementations are all based on POSIX APIs.

torch/csrc/inductor/aoti_runner/model_container_runner.cpp

torch/csrc/inductor/aoti_torch/shim_common.cpp

Skylion007 · 2025-05-04T15:04:15Z

@Skylion007 create_directories may return false if the directory exists. In such cases, we can't rely on std::error_code having a failure value !=0 since the error code is platform dependent (C++ doesn't define a successful value and we have to consider non-UNIX platforms). The most reliable way is to check by std::filesystem::is_directory. If that fails, then std::filesystem:: create_directories should have failed (except for rare race-conditions that some action was applied to the dir externally between the two calls), then by API conversion, std::error_code should record the reason. This line of reasoning leads to the code changes.

In summary, there is not platform independent system call values of success and failure... And it is unlikely that the current implementations are all based on POSIX APIs.

Even if there isn't a platform independent way of checking error, we only really need to check error for POSIX and Windows based systems as the code was doing before.

cyyever · 2025-05-04T16:11:15Z

@Skylion007 On these systems, if the directory didn't exist before creation, std::filesystem::create_directories returns true and std::error_code is the default value. Otherwise std::filesystem::create_directories returns false and std::error_code is an error code. Use std::filesystem::is_directory in the following check is more robust and simple. After all, what we really care is that the directory exists when trying to open file in it.

cyyever · 2025-05-06T13:38:09Z

We shouldn't support GCC<9 in inductor or distributed code...

pytorch-bot bot added ciflow/inductor release notes: inductor (aoti) labels May 1, 2025

cyyever marked this pull request as draft May 1, 2025 04:10

pytorchbot added the open source label May 1, 2025

cyyever force-pushed the fs_path2 branch 2 times, most recently from 4691e9b to 80db20b Compare May 1, 2025 06:00

cyyever changed the title ~~Fs path2~~ [2/N] Use std::filesystem May 1, 2025

cyyever marked this pull request as ready for review May 1, 2025 06:09

cyyever force-pushed the fs_path2 branch 2 times, most recently from 2256eb7 to 9870e67 Compare May 1, 2025 08:43

pytorch-bot bot added the oncall: distributed Add this issue/PR to distributed oncall triage queue label May 1, 2025

cyyever force-pushed the fs_path2 branch 4 times, most recently from 6fc4158 to 340f73e Compare May 1, 2025 09:58

cyyever marked this pull request as draft May 1, 2025 12:49

cyyever force-pushed the fs_path2 branch 3 times, most recently from 64b1f8e to c5b9334 Compare May 1, 2025 13:08

cyyever marked this pull request as ready for review May 1, 2025 14:16

cyyever requested review from malfet, swolchok and desertfire May 1, 2025 14:23

HDCharles requested review from Skylion007 and janeyx99 May 2, 2025 03:55

HDCharles added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 2, 2025

Skylion007 requested changes May 2, 2025

View reviewed changes

janeyx99 removed their request for review May 2, 2025 15:43

Skylion007 reviewed May 4, 2025

View reviewed changes

torch/csrc/inductor/aoti_runner/model_container_runner.cpp Show resolved Hide resolved

torch/csrc/inductor/aoti_torch/shim_common.cpp Show resolved Hide resolved

cyyever force-pushed the fs_path2 branch 3 times, most recently from 9b7cabb to ccb2459 Compare May 6, 2025 13:37

cyyever force-pushed the fs_path2 branch 2 times, most recently from f117b1e to b0eab07 Compare May 7, 2025 00:59

cyyever added 4 commits May 7, 2025 12:07

Use std::filesystem

cdd239a

Fix headers

20972d0

Improve fs::create_directories check

15352cc

Fix headers

d2f2e86

cyyever force-pushed the fs_path2 branch from cf1a989 to d2f2e86 Compare May 7, 2025 04:07

cyyever requested a review from Skylion007 May 11, 2025 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2/N] Use std::filesystem #152586

[2/N] Use std::filesystem #152586

[2/N] Use std::filesystem #152586

Are you sure you want to change the base?

[2/N] Use std::filesystem #152586

Conversation

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/152586

✅ You can merge normally! (2 Unrelated Failures)

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