Fix setup_outputdir #4734

suzhoum · 2024-12-11T18:19:02Z

Issue #, if available:
Fixes #4733

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

suzhoum · 2024-12-11T18:30:06Z

@LennartPurucker This line was added in #4606. Was it intended for the functionality?

shchur · 2024-12-11T19:46:38Z

common/src/autogluon/common/utils/utils.py

    path = os.path.expanduser(path)  # replace ~ with absolute path if it exists
-    path = os.path.abspath(path)


Maybe we can wrap this block into

if not path.startswith("s3://"): path = os.path.expanduser(path) # replace ~ with absolute path if it exists path = os.path.abspath(path)

I believe there are some edge cases against which the current path = os.path.abspath(path) command protects (though it does introduce another bug with loading from S3).

Yes, this looks like the best solution.

What happens if one calls .expanduser() on an S3 path? I think likely nothing, as S3 paths do not contain/start with ~.

Otherwise, the current suggested if-case would change this behavior, too.

In this case I think what @shchur proposed should accommodate S3 paths. I will update the PR.

LennartPurucker · 2024-12-11T20:38:03Z

Ah, my bad! I did not think of s3 when adding this line.

It fixes a bug in parallel and distributed fitting where the workers/processes use a relative path but should use the absolute path in the (shared) file system.

LennartPurucker

LGTM

Innixma · 2024-12-17T21:34:06Z

common/src/autogluon/common/utils/utils.py

@@ -61,8 +61,9 @@ def setup_outputdir(path, warn_if_exist=True, create_dir=True, path_suffix=None)
            logger.warning(
                f'Warning: path already exists! This predictor may overwrite an existing predictor! path="{path}"'
            )
-    path = os.path.expanduser(path)  # replace ~ with absolute path if it exists
-    path = os.path.abspath(path)
+    if not path.lower().startswith("s3://"):


If we plan to support s3, then we should also edit the code above this when warn_if_exist=True to not call os.makedirs when the path is an s3 path.

Also, if we plan to add support for s3, lets add a short docstring that mentions this

Several things need to be done:

If s3, then os.path.sep needs to instead be / when updating path_suffix.

bool flag near start of function to indicate if s3 path. If flag set, skip elif warn_if_exist.

Why path.lower()? This doesn't seem valid.

Refactored the function to address comments.

Innixma · 2025-01-13T22:43:46Z

common/src/autogluon/common/utils/utils.py

-    path = os.path.expanduser(path)  # replace ~ with absolute path if it exists
-    path = os.path.abspath(path)
+    if not path.lower().startswith("s3://"):
+        path = os.path.expanduser(path)  # replace ~ with absolute path if it exists


Add unit test for s3 path input, since it shouldn't make any folder.

Added unit test.

Innixma · 2025-01-15T20:13:37Z

common/src/autogluon/common/utils/utils.py

+            f"Only str and pathlib.Path types are supported for path, got {path} of type {type(path)}."
+        )
+
+        is_s3_path = str(path).lower().startswith("s3://")


why .lower()?

This is just to incorporate cases when people use capitalized S3 in the path.

Innixma · 2025-01-15T20:19:20Z

common/src/autogluon/common/utils/utils.py

        timestamp = utcnow.strftime("%Y%m%d_%H%M%S")
        path = os.path.join("AutogluonModels", f"ag-{timestamp}{path_suffix}")
        for i in range(1, 1000):
            try:
-                if create_dir:
+                if create_dir and not is_s3_path:


No need to check is_s3_path when path is None.

Innixma · 2025-01-15T20:22:45Z

common/tests/unittests/test_setup_outputdir.py

+        # checks that setup_outputdir handles S3 paths correctly
+        with patch("os.makedirs") as mock_makedirs:  # Mock os.makedirs to ensure no local directory is created
+            path = "s3://test-bucket/test-folder"
+            returned_path = setup_outputdir(path, warn_if_exist=True, create_dir=False, path_suffix=None)


create_dir must be True to properly test this.

Innixma · 2025-01-15T20:23:52Z

common/src/autogluon/common/utils/utils.py

    if path_suffix is None:
        path_suffix = ""
-    if path_suffix and path_suffix[-1] == os.path.sep:
+    if path_suffix and path_suffix[-1] == os.path.sep if not is_s3_path else "/":


would be good to add a unit test for this, it is relevant for Windows OS

Added more unit tests for this.

github-actions · 2025-01-16T03:33:18Z

Job PR-4734-a46aed8 is done.
Docs are uploaded to http://autogluon-staging.s3-website-us-west-2.amazonaws.com/PR-4734/a46aed8/index.html

Innixma

LGTM, thanks for the fix!

suzhoum mentioned this pull request Dec 11, 2024

[BUG]Load predictor from S3 path failed #4733

Closed

3 tasks

suzhoum requested review from LennartPurucker and Innixma December 11, 2024 18:28

shchur reviewed Dec 11, 2024

View reviewed changes

shchur approved these changes Dec 12, 2024

View reviewed changes

LennartPurucker approved these changes Dec 12, 2024

View reviewed changes

Innixma reviewed Dec 17, 2024

View reviewed changes

Innixma requested changes Jan 13, 2025

View reviewed changes

suzhoum force-pushed the fix_setup_dir branch from a9b8b75 to aa94612 Compare January 14, 2025 16:51

suzhoum added 5 commits January 15, 2025 19:58

do not use abspath

0af6094

add condition for s3

9e24646

refactor setup_outputdir

79a2b96

add unit test

61a782c

lint

6e3d99b

Innixma reviewed Jan 15, 2025

View reviewed changes

suzhoum force-pushed the fix_setup_dir branch 2 times, most recently from 11f60c6 to 7f1eaaf Compare January 16, 2025 00:17

add tests

a46aed8

suzhoum force-pushed the fix_setup_dir branch from 7f1eaaf to a46aed8 Compare January 16, 2025 00:27

Innixma approved these changes Jan 17, 2025

View reviewed changes

Innixma merged commit 6c7c3e4 into autogluon:master Jan 17, 2025
27 checks passed

		path = os.path.expanduser(path) # replace ~ with absolute path if it exists
		path = os.path.abspath(path)

Fix setup_outputdir #4734

Fix setup_outputdir #4734

Uh oh!

Conversation

suzhoum commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suzhoum commented Dec 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LennartPurucker commented Dec 11, 2024

Uh oh!

LennartPurucker left a comment

Choose a reason for hiding this comment

Uh oh!

Innixma Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 16, 2025

Uh oh!

Innixma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

suzhoum commented Dec 11, 2024 •

edited

Loading

Innixma Dec 17, 2024 •

edited

Loading