From b6f01ab010113668aa24db85a4eabf3fc1ebc2ae Mon Sep 17 00:00:00 2001 From: Rick Izzo Date: Thu, 20 Oct 2022 17:33:35 -0400 Subject: [PATCH] Renamed Mount root_dir Argument to mount_path (#15228) * renamed Mount argument * fix tests * Apply suggestions from code review Co-authored-by: Luca Antiga * updated examples as well Co-authored-by: Luca Antiga --- examples/app_mount/app.py | 2 +- src/lightning_app/runners/cloud.py | 2 +- src/lightning_app/storage/mount.py | 15 ++++++++--- .../utilities/packaging/cloud_compute.py | 6 ++--- tests/tests_app/runners/test_cloud.py | 4 +-- tests/tests_app/storage/test_mount.py | 25 +++++++++++-------- .../utilities/packaging/test_cloud_compute.py | 14 +++++------ 7 files changed, 40 insertions(+), 28 deletions(-) diff --git a/examples/app_mount/app.py b/examples/app_mount/app.py index ed4cb75b4c..9754735f0e 100644 --- a/examples/app_mount/app.py +++ b/examples/app_mount/app.py @@ -23,7 +23,7 @@ class Flow(L.LightningFlow): cloud_compute=CloudCompute( mounts=Mount( source="s3://ryft-public-sample-data/esRedditJson/", - root_dir="/content/esRedditJson/", + mount_path="/content/esRedditJson/", ), ) ) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 4a75678afa..a436a3434b 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -432,5 +432,5 @@ def _create_mount_drive_spec(work_name: str, mount: Mount) -> V1LightningworkDri ), status=V1DriveStatus(), ), - mount_location=str(mount.root_dir), + mount_location=str(mount.mount_path), ) diff --git a/src/lightning_app/storage/mount.py b/src/lightning_app/storage/mount.py index 191684592a..3c520f6ba0 100644 --- a/src/lightning_app/storage/mount.py +++ b/src/lightning_app/storage/mount.py @@ -1,3 +1,4 @@ +import os from dataclasses import dataclass from pathlib import Path from typing import List @@ -16,13 +17,13 @@ class Mount: `s3` style identifier pointing to a bucket and (optionally) prefix to mount. For example: `s3://foo/bar/`. - root_dir: An absolute directory path in the work where external data source should + mount_path: An absolute directory path in the work where external data source should be mounted as a filesystem. This path should not already exist in your codebase. If not included, then the root_dir will be set to `/data/` """ source: str = "" - root_dir: str = "" + mount_path: str = "" def __post_init__(self) -> None: @@ -42,8 +43,14 @@ class Mount: f"Received: '{self.source}'. Mounting a single file is not currently supported." ) - if self.root_dir == "": - self.root_dir = f"/data/{Path(self.source).stem}" + if self.mount_path == "": + self.mount_path = f"/data/{Path(self.source).stem}" + + if not os.path.isabs(self.mount_path): + raise ValueError( + f"mount_path argument must be an absolute path to a " + f"location; received relative path {self.mount_path}" + ) @property def protocol(self) -> str: diff --git a/src/lightning_app/utilities/packaging/cloud_compute.py b/src/lightning_app/utilities/packaging/cloud_compute.py index 09f7830021..c056263f8e 100644 --- a/src/lightning_app/utilities/packaging/cloud_compute.py +++ b/src/lightning_app/utilities/packaging/cloud_compute.py @@ -141,9 +141,9 @@ class CloudCompute: def _verify_mount_root_dirs_are_unique(mounts: Union[None, Mount, List[Mount], Tuple[Mount]]) -> None: if isinstance(mounts, (list, tuple, set)): - root_dirs = [mount.root_dir for mount in mounts] - if len(set(root_dirs)) != len(root_dirs): - raise ValueError("Every Mount attached to a work must have a unique 'root_dir' argument.") + mount_paths = [mount.mount_path for mount in mounts] + if len(set(mount_paths)) != len(mount_paths): + raise ValueError("Every Mount attached to a work must have a unique 'mount_path' argument.") def _maybe_create_cloud_compute(state: Dict) -> Union[CloudCompute, Dict]: diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index 9600fedcef..e8986c28e6 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -665,7 +665,7 @@ class TestAppCreationClient: mocked_mount = MagicMock(spec=Mount) setattr(mocked_mount, "source", "s3://foo/") - setattr(mocked_mount, "root_dir", "/content/") + setattr(mocked_mount, "mount_path", "/content/foo") setattr(mocked_mount, "protocol", "s3://") work = WorkWithSingleDrive() @@ -740,7 +740,7 @@ class TestAppCreationClient: ), status=V1DriveStatus(), ), - mount_location="/content/", + mount_location="/content/foo", ), ], user_requested_compute_config=V1UserRequestedComputeConfig( diff --git a/tests/tests_app/storage/test_mount.py b/tests/tests_app/storage/test_mount.py index 3d7725ecd6..f786cf6054 100644 --- a/tests/tests_app/storage/test_mount.py +++ b/tests/tests_app/storage/test_mount.py @@ -4,34 +4,39 @@ from lightning_app.storage.mount import Mount def test_create_s3_mount_successfully(): - mount = Mount(source="s3://foo/bar/", root_dir="./foo") + mount = Mount(source="s3://foo/bar/", mount_path="/foo") assert mount.source == "s3://foo/bar/" - assert mount.root_dir == "./foo" + assert mount.mount_path == "/foo" assert mount.protocol == "s3://" def test_create_non_s3_mount_fails(): with pytest.raises(ValueError, match="Unknown protocol for the mount 'source' argument"): - Mount(source="foo/bar/", root_dir="./foo") + Mount(source="foo/bar/", mount_path="/foo") with pytest.raises(ValueError, match="Unknown protocol for the mount 'source' argument"): - Mount(source="gcs://foo/bar/", root_dir="./foo") + Mount(source="gcs://foo/bar/", mount_path="/foo") with pytest.raises(ValueError, match="Unknown protocol for the mount 'source' argument"): - Mount(source="3://foo/bar/", root_dir="./foo") + Mount(source="3://foo/bar/", mount_path="/foo") def test_create_s3_mount_without_directory_prefix_fails(): with pytest.raises(ValueError, match="S3 mounts must end in a trailing slash"): - Mount(source="s3://foo/bar", root_dir="./foo") + Mount(source="s3://foo/bar", mount_path="/foo") with pytest.raises(ValueError, match="S3 mounts must end in a trailing slash"): - Mount(source="s3://foo", root_dir="./foo") + Mount(source="s3://foo", mount_path="/foo") -def test_create_mount_without_root_dir_argument(): +def test_create_mount_without_mount_path_argument(): m = Mount(source="s3://foo/") - assert m.root_dir == "/data/foo" + assert m.mount_path == "/data/foo" m = Mount(source="s3://foo/bar/") - assert m.root_dir == "/data/bar" + assert m.mount_path == "/data/bar" + + +def test_create_mount_path_with_relative_path_errors(): + with pytest.raises(ValueError, match="mount_path argument must be an absolute path"): + Mount(source="s3://foo/", mount_path="./doesnotwork") diff --git a/tests/tests_app/utilities/packaging/test_cloud_compute.py b/tests/tests_app/utilities/packaging/test_cloud_compute.py index 294344a29a..48ed3a388b 100644 --- a/tests/tests_app/utilities/packaging/test_cloud_compute.py +++ b/tests/tests_app/utilities/packaging/test_cloud_compute.py @@ -23,8 +23,8 @@ def test_cloud_compute_shared_memory(): def test_cloud_compute_with_mounts(): - mount_1 = Mount(source="s3://foo/", root_dir="./foo") - mount_2 = Mount(source="s3://foo/bar/", root_dir="./bar") + mount_1 = Mount(source="s3://foo/", mount_path="/foo") + mount_2 = Mount(source="s3://foo/bar/", mount_path="/bar") cloud_compute = CloudCompute("gpu", mounts=mount_1) assert cloud_compute.mounts == mount_1 @@ -35,16 +35,16 @@ def test_cloud_compute_with_mounts(): cc_dict = cloud_compute.to_dict() assert "mounts" in cc_dict assert cc_dict["mounts"] == [ - {"root_dir": "./foo", "source": "s3://foo/"}, - {"root_dir": "./bar", "source": "s3://foo/bar/"}, + {"mount_path": "/foo", "source": "s3://foo/"}, + {"mount_path": "/bar", "source": "s3://foo/bar/"}, ] assert CloudCompute.from_dict(cc_dict) == cloud_compute def test_cloud_compute_with_non_unique_mount_root_dirs(): - mount_1 = Mount(source="s3://foo/", root_dir="./foo") - mount_2 = Mount(source="s3://foo/bar/", root_dir="./foo") + mount_1 = Mount(source="s3://foo/", mount_path="/foo") + mount_2 = Mount(source="s3://foo/bar/", mount_path="/foo") - with pytest.raises(ValueError, match="Every Mount attached to a work must have a unique 'root_dir' argument."): + with pytest.raises(ValueError, match="Every Mount attached to a work must have a unique"): CloudCompute("gpu", mounts=[mount_1, mount_2])