[train+tune] Make path joining OS-agnostic by using `Path.as_posix` over `os.path.join` #42037

n30111 · 2023-12-20T16:19:22Z

Why are these changes needed?

os.path.join uses an OS-dependent separator (\ on windows and / on posix systems), which causes some issues when os.path.join is used in conjunction with Path. The combination can result in a mix of / and \ when running Ray Train/Tune on windows. This runs into issues when passing these paths into pyarrow.fs, leading to issues such as FileNotFound.

This PR fixes the following issues:

While running training on Windows OS with S3 as storage system, some files are not placed in correct locations.
For example the file checkpoint_00000/.metadata.json ends up as checkpoint_00000\.metadata.json.
Also while restoring a tuner run, it fails, as tuner.pkl is not located correctly due to path issue.

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

justinvyu

Thanks for the contribution! I think we should generally use Path.as_posix everywhere we need to do path joins to be OS agnostic, and pyarrow (which we use under the hood) does not like the mix of posix and windows separators.

I'm wondering if we want to actually go ahead and change ALL of them with this PR? I did a global search on the repo and have found a bunch more instances of os.path.join in our Ray Train/Tune code.

Another note: we basically don't have comprehensive windows tests, so support for windows for at least Ray Train/Tune is very experimental. If you are not tied down to local development on a windows machine, running on a devbox or a linux subsystem may be a good thing to consider.

python/ray/train/_checkpoint.py

Signed-off-by: n3011 <mrinal.haloi11@gmail.com>

Co-authored-by: Justin Yu <justinvyu@anyscale.com> Signed-off-by: Ishant Mrinal <33053278+n30111@users.noreply.github.com> Signed-off-by: n3011 <mrinal.haloi11@gmail.com>

Signed-off-by: n3011 <mrinal.haloi11@gmail.com>

n30111 · 2023-12-21T02:49:28Z

@justinvyu thanks, we are dependent on both linux and windows, so this changes will be useful.

n30111 · 2023-12-28T03:52:50Z

@justinvyu I have updated the PR, It would be great if we can include these changes in the next release, thanks.

justinvyu · 2024-01-08T23:42:35Z

Hey @n30111 sorry for the delay, I'll do another review on this PR, so that we can get it in for Ray 2.10.

jbedorf · 2024-01-11T11:21:51Z

@justinvyu Friendly ping on this PR.

…path_fix

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

…path_fix

justinvyu · 2024-01-17T01:24:52Z

@n3011 @jbedorf Is it possible to "give maintainers permission to edit" the PR (should be some setting in the PR)? I've replaced os.path.join in many other place sin the code.

n30111 · 2024-01-17T08:54:56Z

@justinvyu Hi Justin, that particular option does not work for forks hosted by organizations (see: https://github.com/orgs/community/discussions/5634 ), but we just added you to the fork/repository to enable you to push the changes you made.

…path_fix

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

justinvyu · 2024-01-17T20:56:43Z

@n3011 Is it possible to add a small test to confirm that this PR fixes the failure scenario you're running into? Thanks!

https://github.com/ray-project/ray/blob/master/python/ray/train/tests/test_windows.py

matthewdeng · 2024-01-17T20:39:35Z

python/ray/train/_checkpoint.py

@@ -129,7 +130,7 @@ def get_metadata(self) -> Dict[str, Any]:

        If no metadata is stored, an empty dict is returned.
        """
-        metadata_path = os.path.join(self.path, _METADATA_FILE_NAME)
+        metadata_path = Path(self.path, _METADATA_FILE_NAME).as_posix()


Should we create a common utility that wraps this Path/as_posix logic?

I was thinking about it but I prefer just seeing this spelled out. I think the longer term goal is to replace everything with Path objects and never pass strings around.

matthewdeng · 2024-01-17T21:12:02Z

python/ray/tune/logger/json.py

@@ -41,8 +41,8 @@ class JsonLogger(Logger):

    def _init(self):
        self.update_config(self.config)
-        local_file = os.path.join(self.logdir, EXPR_RESULT_FILE)
-        self.local_out = open(local_file, "a")
+        local_file = Path(self.logdir, EXPR_RESULT_FILE)


Should this include as_posix()?

Same question for other instances below.

I just used Path.open instead of open(str) below.

n30111 · 2024-01-18T03:02:16Z

@n3011 Is it possible to add a small test to confirm that this PR fixes the failure scenario you're running into? Thanks!

https://github.com/ray-project/ray/blob/master/python/ray/train/tests/test_windows.py

This test covers the scenario https://github.com/ray-project/ray/blob/master/python/ray/train/tests/test_trainer_restore.py#L189, is this run on windows system also?

…path_fix Signed-off-by: Justin Yu <justinvyu@anyscale.com>

justinvyu · 2024-01-31T22:35:10Z

Thanks for this contribution @n30111! 🚀 Let me know if you get a chance to try it out on nightly.

…ver `os.path.join` (ray-project#42037) `os.path.join` uses an OS-dependent separator (`\` on windows and `/` on posix systems), which causes some issues when `os.path.join` is used in conjunction with `Path`. The combination can result in a mix of `/` and `\` when running Ray Train/Tune on windows. This runs into issues when passing these paths into `pyarrow.fs`, leading to issues such as `FileNotFound`. --------- Signed-off-by: n3011 <mrinal.haloi11@gmail.com> Signed-off-by: Ishant Mrinal <33053278+n30111@users.noreply.github.com> Signed-off-by: Justin Yu <justinvyu@anyscale.com> Co-authored-by: n3011 <mrinal.haloi11@gmail.com> Co-authored-by: Justin Yu <justinvyu@anyscale.com>

…ver `os.path.join` (ray-project#42037) `os.path.join` uses an OS-dependent separator (`\` on windows and `/` on posix systems), which causes some issues when `os.path.join` is used in conjunction with `Path`. The combination can result in a mix of `/` and `\` when running Ray Train/Tune on windows. This runs into issues when passing these paths into `pyarrow.fs`, leading to issues such as `FileNotFound`. --------- Signed-off-by: n3011 <mrinal.haloi11@gmail.com> Signed-off-by: Ishant Mrinal <33053278+n30111@users.noreply.github.com> Signed-off-by: Justin Yu <justinvyu@anyscale.com> Co-authored-by: n3011 <mrinal.haloi11@gmail.com> Co-authored-by: Justin Yu <justinvyu@anyscale.com> Signed-off-by: tterrysun <terry@anyscale.com>

matthewdeng requested a review from justinvyu December 20, 2023 16:29

justinvyu reviewed Dec 20, 2023

View reviewed changes

python/ray/train/_checkpoint.py Outdated Show resolved Hide resolved

n3011 and others added 3 commits December 21, 2023 02:44

use posix path for windows with s3 compatibility

d419277

Signed-off-by: n3011 <mrinal.haloi11@gmail.com>

Apply suggestions from code review

75f2c38

Co-authored-by: Justin Yu <justinvyu@anyscale.com> Signed-off-by: Ishant Mrinal <33053278+n30111@users.noreply.github.com> Signed-off-by: n3011 <mrinal.haloi11@gmail.com>

update path syntax

d979f76

Signed-off-by: n3011 <mrinal.haloi11@gmail.com>

n30111 force-pushed the win_path_fix branch from 3235d6f to d979f76 Compare December 21, 2023 02:44

Merge branch 'master' into win_path_fix

2829cb8

justinvyu added 12 commits January 16, 2024 11:28

Merge branch 'master' of https://github.com/ray-project/ray into win_…

57ed08e

…path_fix

easy replacements:

80e723c

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

update exclude pattern logic to always check for posix style

6eaf362

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

finish easy train replacements

195ac65

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

update exp analysis paths

55b4c0c

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

make callback can_restore more efficient

cf216d4

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

tune_controller, trial, progress reporter

ad0ca97

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

replace for csv logger

11f77a2

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

finish fixing loggers

8640a58

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

basic variant

8ef4ca3

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

fix trainable

17aa499

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

Merge branch 'master' of https://github.com/ray-project/ray into win_…

5e63aa0

…path_fix

justinvyu added 2 commits January 17, 2024 11:22

Merge branch 'master' of https://github.com/ray-project/ray into win_…

a07521a

…path_fix

fix lint

de7dc72

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

justinvyu requested a review from matthewdeng January 17, 2024 19:23

justinvyu assigned matthewdeng Jan 17, 2024

matthewdeng approved these changes Jan 17, 2024

View reviewed changes

justinvyu self-assigned this Jan 29, 2024

Merge branch 'master' of https://github.com/ray-project/ray into win_…

14fbb18

…path_fix Signed-off-by: Justin Yu <justinvyu@anyscale.com>

justinvyu changed the title ~~[tune/train] Fix issues of files misplacement for Windows with S3 file system~~ [train+tune] Make path joining OS-agnostic by using Path.as_posix over os.path.join Jan 31, 2024

justinvyu merged commit 30653f1 into ray-project:master Jan 31, 2024
9 checks passed

justinvyu deleted the win_path_fix branch January 31, 2024 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[train+tune] Make path joining OS-agnostic by using `Path.as_posix` over `os.path.join` #42037

[train+tune] Make path joining OS-agnostic by using `Path.as_posix` over `os.path.join` #42037

n30111 commented Dec 20, 2023 •

edited by justinvyu

justinvyu left a comment

n30111 commented Dec 21, 2023

n30111 commented Dec 28, 2023

justinvyu commented Jan 8, 2024 •

edited

jbedorf commented Jan 11, 2024

justinvyu commented Jan 17, 2024

n30111 commented Jan 17, 2024

justinvyu commented Jan 17, 2024

matthewdeng Jan 17, 2024

justinvyu Jan 17, 2024

matthewdeng Jan 17, 2024

matthewdeng Jan 17, 2024

justinvyu Jan 17, 2024

n30111 commented Jan 18, 2024 •

edited

justinvyu commented Jan 31, 2024

[train+tune] Make path joining OS-agnostic by using Path.as_posix over os.path.join #42037

[train+tune] Make path joining OS-agnostic by using Path.as_posix over os.path.join #42037

Conversation

n30111 commented Dec 20, 2023 • edited by justinvyu

Why are these changes needed?

Related issue number

Checks

justinvyu left a comment

Choose a reason for hiding this comment

n30111 commented Dec 21, 2023

n30111 commented Dec 28, 2023

justinvyu commented Jan 8, 2024 • edited

jbedorf commented Jan 11, 2024

justinvyu commented Jan 17, 2024

n30111 commented Jan 17, 2024

justinvyu commented Jan 17, 2024

matthewdeng Jan 17, 2024

Choose a reason for hiding this comment

justinvyu Jan 17, 2024

Choose a reason for hiding this comment

matthewdeng Jan 17, 2024

Choose a reason for hiding this comment

matthewdeng Jan 17, 2024

Choose a reason for hiding this comment

justinvyu Jan 17, 2024

Choose a reason for hiding this comment

n30111 commented Jan 18, 2024 • edited

justinvyu commented Jan 31, 2024

[train+tune] Make path joining OS-agnostic by using `Path.as_posix` over `os.path.join` #42037

[train+tune] Make path joining OS-agnostic by using `Path.as_posix` over `os.path.join` #42037

n30111 commented Dec 20, 2023 •

edited by justinvyu

justinvyu commented Jan 8, 2024 •

edited

n30111 commented Jan 18, 2024 •

edited