From 32eb9fc8dfef5674952b442831d94f6c9da35ca5 Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Wed, 2 Nov 2022 18:03:01 -0400 Subject: [PATCH 01/17] Error when running on multiple clusters --- src/lightning_app/runners/cloud.py | 15 ++++ tests/tests_app/runners/test_cloud.py | 123 ++++++++------------------ 2 files changed, 51 insertions(+), 87 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 919f7548bc09c..8f416d278e144 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -11,6 +11,7 @@ import click from lightning_cloud.openapi import ( + Externalv1LightningappInstance, Body3, Body4, Body7, @@ -285,6 +286,7 @@ def dispatch( elif CLOUD_QUEUE_TYPE == "redis": queue_server_type = V1QueueServerType.REDIS + existing_instance: Optional[Externalv1LightningappInstance] = None if find_instances_resp.lightningapps: existing_instance = find_instances_resp.lightningapps[0] @@ -329,6 +331,19 @@ def dispatch( default=True, ) app_config.cluster_id = None + if existing_instance and existing_instance.spec.cluster_id != app_config.cluster_id: + raise ValueError( + "\n".join([ + f"Can not run app '{app_config.name}' on cluster {app_config.cluster_id} " + f"since it already exists on {existing_instance.spec.cluster_id} " + "(moving apps between clusters is not supported).", + "You can either:", + f"a. rename app to run on {existing_instance.spec.cluster_id} with the --name option", + "lightning run app script.py" + f"--name (new name) --cloud --cluster-id {existing_instance.spec.cluster_id}", + "b. delete the existing app in the UI before running this command." + ]) + ) if app_config.cluster_id is not None: self._ensure_cluster_project_binding(project.project_id, app_config.cluster_id) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index 50be1ea32ccfb..dd2cc74585196 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -92,19 +92,30 @@ def get_cloud_runtime_request_body(**kwargs) -> "Body8": return Body8(**default_request_body) +@pytest.fixture +def cloud_backend(monkeypatch): + cloud_backend = mock.MagicMock() + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + return cloud_backend + +@pytest.fixture +def project_id(): + return "test-project-id" + class TestAppCreationClient: """Testing the calls made using GridRestClient to create the app.""" # TODO: remove this test once there is support for multiple instances - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) - def test_new_instance_on_different_cluster(self, monkeypatch): + def test_new_instance_on_different_cluster(self, monkeypatch, cloud_backend, project_id): app_name = "test-app-name" original_cluster = "cluster-001" new_cluster = "cluster-002" mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( - memberships=[V1Membership(name="Default Project", project_id="default-project-id")] + memberships=[V1Membership(name="Default Project", project_id=project_id)] ) mock_client.lightningapp_v2_service_create_lightningapp_release.return_value = V1LightningappRelease( cluster_id=new_cluster @@ -113,11 +124,7 @@ def test_new_instance_on_different_cluster(self, monkeypatch): [Externalv1Cluster(id=original_cluster), Externalv1Cluster(id=new_cluster)] ) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) app = mock.MagicMock() app.flows = [] @@ -133,38 +140,26 @@ def test_new_instance_on_different_cluster(self, monkeypatch): cloud_runtime = cloud.CloudRuntime(app=app, entrypoint_file="entrypoint.py") cloud_runtime._check_uploaded_folder = mock.MagicMock() - # without requirements file - # setting is_file to False so requirements.txt existence check will return False - monkeypatch.setattr(Path, "is_file", lambda *args, **kwargs: False) - monkeypatch.setattr(cloud, "Path", Path) - # This is the main assertion: # we have an existing instance on `cluster-001` # but we want to run this app on `cluster-002` - cloud_runtime.dispatch(name=app_name, cluster_id=new_cluster) - - body = Body8( - cluster_id=new_cluster, - app_entrypoint_file=mock.ANY, - enable_app_server=True, - flow_servers=[], - image_spec=None, - works=[], - local_source=True, - dependency_cache_key=mock.ANY, - user_requested_flow_compute_config=mock.ANY, - ) - cloud_runtime.backend.client.lightningapp_v2_service_create_lightningapp_release.assert_called_once_with( - project_id="default-project-id", app_id=mock.ANY, body=body - ) - cloud_runtime.backend.client.projects_service_create_project_cluster_binding.assert_called_once_with( - project_id="default-project-id", - body=V1ProjectClusterBinding(cluster_id=new_cluster, project_id="default-project-id"), + with pytest.raises(ValueError) as exception: + cloud_runtime.dispatch(name=app_name, cluster_id=new_cluster) + + assert str(exception.value) == "\n".join([ + f"Can not run app '{app_name}' on cluster {new_cluster} " + f"since it already exists on {original_cluster} " + "(moving apps between clusters is not supported).", + "You can either:", + f"a. rename app to run on {original_cluster} with the --name option", + "lightning run app script.py" + f"--name (new name) --cloud --cluster-id {original_cluster}", + "b. delete the existing app in the UI before running this command." + ] ) @pytest.mark.parametrize("flow_cloud_compute", [None, CloudCompute(name="t2.medium")]) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) - def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_compute): + def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_compute, cloud_backend): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -176,10 +171,7 @@ def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_comp cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) dummy_flow = mock.MagicMock() monkeypatch.setattr(dummy_flow, "run", lambda *args, **kwargs: None) @@ -208,8 +200,7 @@ def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_comp project_id="test-project-id", app_id=mock.ANY, body=body ) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) - def test_run_on_byoc_cluster(self, monkeypatch): + def test_run_on_byoc_cluster(self, monkeypatch, cloud_backend): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="Default Project", project_id="default-project-id")] @@ -223,11 +214,7 @@ def test_run_on_byoc_cluster(self, monkeypatch): mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse( [Externalv1Cluster(id="test1234")] ) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() app.flows = [] app.frontend = {} @@ -258,8 +245,7 @@ def test_run_on_byoc_cluster(self, monkeypatch): body=V1ProjectClusterBinding(cluster_id="test1234", project_id="default-project-id"), ) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) - def test_requirements_file(self, monkeypatch): + def test_requirements_file(self, monkeypatch, cloud_backend): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -271,11 +257,7 @@ def test_requirements_file(self, monkeypatch): cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() app.flows = [] app.frontend = {} @@ -317,8 +299,7 @@ def test_requirements_file(self, monkeypatch): project_id="test-project-id", app_id=mock.ANY, body=body ) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) - def test_no_cache(self, monkeypatch): + def test_no_cache(self, monkeypatch, cloud_backend): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -330,11 +311,7 @@ def test_no_cache(self, monkeypatch): cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) monkeypatch.setattr(cloud, "get_hash", lambda *args, **kwargs: "dummy-hash") app = mock.MagicMock() app.flows = [] @@ -367,9 +344,8 @@ def test_no_cache(self, monkeypatch): body = kwargs["body"] assert body.dependency_cache_key is None - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir): + def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir, cloud_backend): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -391,11 +367,7 @@ def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir): existing_instance = MagicMock() existing_instance.status.phase = V1LightningappInstanceState.STOPPED mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -469,9 +441,8 @@ def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir): project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=mock.ANY ) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, tmpdir): + def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, tmpdir, cloud_backend): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -483,11 +454,7 @@ def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() app.flows = [] app.frontend = {} @@ -529,9 +496,8 @@ def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=body ) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch, tmpdir): + def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch, tmpdir, cloud_backend): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -556,11 +522,7 @@ def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch existing_instance = MagicMock() existing_instance.status.phase = V1LightningappInstanceState.STOPPED mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -662,9 +624,8 @@ def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=mock.ANY ) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, monkeypatch, tmpdir): + def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, monkeypatch, tmpdir, cloud_backend): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -689,11 +650,7 @@ def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, mo existing_instance = MagicMock() existing_instance.status.phase = V1LightningappInstanceState.STOPPED mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -860,9 +817,8 @@ def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, mo project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=mock.ANY ) - @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, monkeypatch, tmpdir): + def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, monkeypatch, tmpdir, cloud_backend): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -888,11 +844,7 @@ def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, mo existing_instance.status.phase = V1LightningappInstanceState.STOPPED existing_instance.spec.cluster_id = None mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) - cloud_backend = mock.MagicMock() cloud_backend.client = mock_client - monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) - monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) - monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -1016,7 +968,6 @@ def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, mo @mock.patch("lightning_app.core.queues.QueuingSystem", MagicMock()) -@mock.patch("lightning_app.runners.backends.cloud.LightningClient", MagicMock()) def test_get_project(monkeypatch): mock_client = mock.MagicMock() monkeypatch.setattr(cloud, "CloudBackend", mock.MagicMock(return_value=mock_client)) @@ -1049,7 +1000,6 @@ def test_get_project(monkeypatch): @mock.patch("lightning_app.core.queues.QueuingSystem", MagicMock()) -@mock.patch("lightning_app.runners.backends.cloud.LightningClient", MagicMock()) def test_check_uploaded_folder(monkeypatch, tmpdir, caplog): monkeypatch.setattr(cloud, "logger", logging.getLogger()) @@ -1072,7 +1022,6 @@ def test_check_uploaded_folder(monkeypatch, tmpdir, caplog): @mock.patch("lightning_app.core.queues.QueuingSystem", MagicMock()) -@mock.patch("lightning_app.runners.backends.cloud.LightningClient", MagicMock()) def test_project_has_sufficient_credits(): app = mock.MagicMock(spec=LightningApp) cloud_runtime = cloud.CloudRuntime(app=app, entrypoint_file="entrypoint.py") From 5a60a9dd245489d0232dccac53c8aed550eb649e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 2 Nov 2022 22:06:13 +0000 Subject: [PATCH 02/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/lightning_app/runners/cloud.py | 18 ++++++++++-------- tests/tests_app/runners/test_cloud.py | 14 ++++++++------ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 8f416d278e144..5dad1c16537d9 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -11,12 +11,12 @@ import click from lightning_cloud.openapi import ( - Externalv1LightningappInstance, Body3, Body4, Body7, Body8, Body9, + Externalv1LightningappInstance, Gridv1ImageSpec, V1BuildSpec, V1DependencyFileInfo, @@ -333,16 +333,18 @@ def dispatch( app_config.cluster_id = None if existing_instance and existing_instance.spec.cluster_id != app_config.cluster_id: raise ValueError( - "\n".join([ - f"Can not run app '{app_config.name}' on cluster {app_config.cluster_id} " + "\n".join( + [ + f"Can not run app '{app_config.name}' on cluster {app_config.cluster_id} " f"since it already exists on {existing_instance.spec.cluster_id} " "(moving apps between clusters is not supported).", - "You can either:", - f"a. rename app to run on {existing_instance.spec.cluster_id} with the --name option", - "lightning run app script.py" + "You can either:", + f"a. rename app to run on {existing_instance.spec.cluster_id} with the --name option", + "lightning run app script.py" f"--name (new name) --cloud --cluster-id {existing_instance.spec.cluster_id}", - "b. delete the existing app in the UI before running this command." - ]) + "b. delete the existing app in the UI before running this command.", + ] + ) ) if app_config.cluster_id is not None: diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index dd2cc74585196..74da935eefd55 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -100,10 +100,12 @@ def cloud_backend(monkeypatch): monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) return cloud_backend + @pytest.fixture def project_id(): return "test-project-id" + class TestAppCreationClient: """Testing the calls made using GridRestClient to create the app.""" @@ -146,15 +148,15 @@ def test_new_instance_on_different_cluster(self, monkeypatch, cloud_backend, pro with pytest.raises(ValueError) as exception: cloud_runtime.dispatch(name=app_name, cluster_id=new_cluster) - assert str(exception.value) == "\n".join([ + assert str(exception.value) == "\n".join( + [ f"Can not run app '{app_name}' on cluster {new_cluster} " - f"since it already exists on {original_cluster} " - "(moving apps between clusters is not supported).", + f"since it already exists on {original_cluster} " + "(moving apps between clusters is not supported).", "You can either:", f"a. rename app to run on {original_cluster} with the --name option", - "lightning run app script.py" - f"--name (new name) --cloud --cluster-id {original_cluster}", - "b. delete the existing app in the UI before running this command." + "lightning run app script.py" f"--name (new name) --cloud --cluster-id {original_cluster}", + "b. delete the existing app in the UI before running this command.", ] ) From d9bc71cbd324a0a08873675c6bee40557c4f02fc Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Sat, 5 Nov 2022 19:26:49 -0400 Subject: [PATCH 03/17] Revert this in separate PR: keep this focused --- tests/tests_app/runners/test_cloud.py | 65 +++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 9 deletions(-) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index 74da935eefd55..f3121391a2ea4 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -161,7 +161,8 @@ def test_new_instance_on_different_cluster(self, monkeypatch, cloud_backend, pro ) @pytest.mark.parametrize("flow_cloud_compute", [None, CloudCompute(name="t2.medium")]) - def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_compute, cloud_backend): + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) + def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_compute): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -173,7 +174,10 @@ def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_comp cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) dummy_flow = mock.MagicMock() monkeypatch.setattr(dummy_flow, "run", lambda *args, **kwargs: None) @@ -202,7 +206,8 @@ def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_comp project_id="test-project-id", app_id=mock.ANY, body=body ) - def test_run_on_byoc_cluster(self, monkeypatch, cloud_backend): + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) + def test_run_on_byoc_cluster(self, monkeypatch): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="Default Project", project_id="default-project-id")] @@ -216,7 +221,11 @@ def test_run_on_byoc_cluster(self, monkeypatch, cloud_backend): mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse( [Externalv1Cluster(id="test1234")] ) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() app.flows = [] app.frontend = {} @@ -247,7 +256,8 @@ def test_run_on_byoc_cluster(self, monkeypatch, cloud_backend): body=V1ProjectClusterBinding(cluster_id="test1234", project_id="default-project-id"), ) - def test_requirements_file(self, monkeypatch, cloud_backend): + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) + def test_requirements_file(self, monkeypatch): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -259,7 +269,11 @@ def test_requirements_file(self, monkeypatch, cloud_backend): cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() app.flows = [] app.frontend = {} @@ -301,7 +315,8 @@ def test_requirements_file(self, monkeypatch, cloud_backend): project_id="test-project-id", app_id=mock.ANY, body=body ) - def test_no_cache(self, monkeypatch, cloud_backend): + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) + def test_no_cache(self, monkeypatch): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -313,7 +328,11 @@ def test_no_cache(self, monkeypatch, cloud_backend): cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) monkeypatch.setattr(cloud, "get_hash", lambda *args, **kwargs: "dummy-hash") app = mock.MagicMock() app.flows = [] @@ -346,8 +365,9 @@ def test_no_cache(self, monkeypatch, cloud_backend): body = kwargs["body"] assert body.dependency_cache_key is None + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir, cloud_backend): + def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -369,7 +389,11 @@ def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir, cloud_back existing_instance = MagicMock() existing_instance.status.phase = V1LightningappInstanceState.STOPPED mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -443,8 +467,9 @@ def test_call_with_work_app(self, lightningapps, monkeypatch, tmpdir, cloud_back project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=mock.ANY ) + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, tmpdir, cloud_backend): + def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, tmpdir): mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( memberships=[V1Membership(name="test-project", project_id="test-project-id")] @@ -456,7 +481,11 @@ def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, cluster_id="test" ) mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([Externalv1Cluster(id="test")]) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() app.flows = [] app.frontend = {} @@ -498,8 +527,9 @@ def test_call_with_queue_server_type_specified(self, lightningapps, monkeypatch, project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=body ) + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch, tmpdir, cloud_backend): + def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch, tmpdir): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -524,7 +554,11 @@ def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch existing_instance = MagicMock() existing_instance.status.phase = V1LightningappInstanceState.STOPPED mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -626,8 +660,9 @@ def test_call_with_work_app_and_attached_drives(self, lightningapps, monkeypatch project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=mock.ANY ) + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, monkeypatch, tmpdir, cloud_backend): + def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, monkeypatch, tmpdir): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -652,7 +687,11 @@ def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, mo existing_instance = MagicMock() existing_instance.status.phase = V1LightningappInstanceState.STOPPED mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -819,8 +858,9 @@ def test_call_with_work_app_and_multiple_attached_drives(self, lightningapps, mo project_id="test-project-id", app_id=mock.ANY, id=mock.ANY, body=mock.ANY ) + @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) @pytest.mark.parametrize("lightningapps", [[], [MagicMock()]]) - def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, monkeypatch, tmpdir, cloud_backend): + def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, monkeypatch, tmpdir): source_code_root_dir = Path(tmpdir / "src").absolute() source_code_root_dir.mkdir() Path(source_code_root_dir / ".lightning").write_text("cluster_id: test\nname: myapp") @@ -846,7 +886,11 @@ def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, mo existing_instance.status.phase = V1LightningappInstanceState.STOPPED existing_instance.spec.cluster_id = None mock_client.lightningapp_service_get_lightningapp = MagicMock(return_value=existing_instance) + cloud_backend = mock.MagicMock() cloud_backend.client = mock_client + monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend)) + monkeypatch.setattr(cloud, "LocalSourceCodeDir", mock.MagicMock()) + monkeypatch.setattr(cloud, "_prepare_lightning_wheels_and_requirements", mock.MagicMock()) app = mock.MagicMock() flow = mock.MagicMock() @@ -970,6 +1014,7 @@ def test_call_with_work_app_and_attached_mount_and_drive(self, lightningapps, mo @mock.patch("lightning_app.core.queues.QueuingSystem", MagicMock()) +@mock.patch("lightning_app.runners.backends.cloud.LightningClient", MagicMock()) def test_get_project(monkeypatch): mock_client = mock.MagicMock() monkeypatch.setattr(cloud, "CloudBackend", mock.MagicMock(return_value=mock_client)) @@ -1002,6 +1047,7 @@ def test_get_project(monkeypatch): @mock.patch("lightning_app.core.queues.QueuingSystem", MagicMock()) +@mock.patch("lightning_app.runners.backends.cloud.LightningClient", MagicMock()) def test_check_uploaded_folder(monkeypatch, tmpdir, caplog): monkeypatch.setattr(cloud, "logger", logging.getLogger()) @@ -1024,6 +1070,7 @@ def test_check_uploaded_folder(monkeypatch, tmpdir, caplog): @mock.patch("lightning_app.core.queues.QueuingSystem", MagicMock()) +@mock.patch("lightning_app.runners.backends.cloud.LightningClient", MagicMock()) def test_project_has_sufficient_credits(): app = mock.MagicMock(spec=LightningApp) cloud_runtime = cloud.CloudRuntime(app=app, entrypoint_file="entrypoint.py") From cad231f11fa2c4750d1dd3563ad38203c77a938e Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Sat, 5 Nov 2022 19:59:02 -0400 Subject: [PATCH 04/17] Improve testing --- src/lightning_app/runners/cloud.py | 24 ++++++------- tests/tests_app/runners/test_cloud.py | 52 ++++++++++++++++++--------- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 5dad1c16537d9..d65271ff6d1cb 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -8,6 +8,7 @@ from dataclasses import dataclass from pathlib import Path from typing import Any, Callable, List, Optional, Union +from textwrap import dedent import click from lightning_cloud.openapi import ( @@ -332,20 +333,15 @@ def dispatch( ) app_config.cluster_id = None if existing_instance and existing_instance.spec.cluster_id != app_config.cluster_id: - raise ValueError( - "\n".join( - [ - f"Can not run app '{app_config.name}' on cluster {app_config.cluster_id} " - f"since it already exists on {existing_instance.spec.cluster_id} " - "(moving apps between clusters is not supported).", - "You can either:", - f"a. rename app to run on {existing_instance.spec.cluster_id} with the --name option", - "lightning run app script.py" - f"--name (new name) --cloud --cluster-id {existing_instance.spec.cluster_id}", - "b. delete the existing app in the UI before running this command.", - ] - ) - ) + raise ValueError(dedent(f"""\ + Can not run app '{app_config.name}' on cluster {app_config.cluster_id} since it already exists on {existing_instance.spec.cluster_id} + (moving apps between clusters is not supported). + + You can either: + a. rename app to run on {app_config.cluster_id} with the --name option + lightning run app script.py --name (new name) --cloud --cluster-id {app_config.cluster_id} + b. delete the app running on {existing_instance.spec.cluster_id} in the UI before running this command. + """)) if app_config.cluster_id is not None: self._ensure_cluster_project_binding(project.project_id, app_config.cluster_id) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index f3121391a2ea4..bd35b86831a03 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -1,9 +1,11 @@ import logging import os +from contextlib import contextmanager from copy import copy from pathlib import Path from unittest import mock from unittest.mock import MagicMock +from textwrap import dedent import pytest from lightning_cloud.openapi import ( @@ -105,15 +107,25 @@ def cloud_backend(monkeypatch): def project_id(): return "test-project-id" +@contextmanager +def does_not_raise(): + yield + +DEFAULT_CLUSTER = "litng-ai-03" class TestAppCreationClient: """Testing the calls made using GridRestClient to create the app.""" # TODO: remove this test once there is support for multiple instances - def test_new_instance_on_different_cluster(self, monkeypatch, cloud_backend, project_id): + @pytest.mark.parametrize("old_cluster,new_cluster,expected_raise", [ + ("test", "other", pytest.raises(ValueError)), + ("test", "test", does_not_raise()), + (None, None, does_not_raise()), + (None, "litng-ai-03", does_not_raise()), + ("litng-ai-03", None, does_not_raise()), + ]) + def test_new_instance_on_different_cluster(self, cloud_backend, project_id, old_cluster, new_cluster, expected_raise): app_name = "test-app-name" - original_cluster = "cluster-001" - new_cluster = "cluster-002" mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( @@ -122,8 +134,13 @@ def test_new_instance_on_different_cluster(self, monkeypatch, cloud_backend, pro mock_client.lightningapp_v2_service_create_lightningapp_release.return_value = V1LightningappRelease( cluster_id=new_cluster ) + + # backend converts "None" cluster to "litng-ai-03" mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse( - [Externalv1Cluster(id=original_cluster), Externalv1Cluster(id=new_cluster)] + [ + Externalv1Cluster(id=old_cluster or DEFAULT_CLUSTER), + Externalv1Cluster(id=new_cluster or DEFAULT_CLUSTER), + ] ) cloud_backend.client = mock_client @@ -134,7 +151,7 @@ def test_new_instance_on_different_cluster(self, monkeypatch, cloud_backend, pro existing_instance = MagicMock() existing_instance.status.phase = V1LightningappInstanceState.STOPPED - existing_instance.spec.cluster_id = original_cluster + existing_instance.spec.cluster_id = old_cluster or DEFAULT_CLUSTER mock_client.lightningapp_instance_service_list_lightningapp_instances.return_value = ( V1ListLightningappInstancesResponse(lightningapps=[existing_instance]) ) @@ -145,20 +162,21 @@ def test_new_instance_on_different_cluster(self, monkeypatch, cloud_backend, pro # This is the main assertion: # we have an existing instance on `cluster-001` # but we want to run this app on `cluster-002` - with pytest.raises(ValueError) as exception: + with expected_raise as exception: cloud_runtime.dispatch(name=app_name, cluster_id=new_cluster) - assert str(exception.value) == "\n".join( - [ - f"Can not run app '{app_name}' on cluster {new_cluster} " - f"since it already exists on {original_cluster} " - "(moving apps between clusters is not supported).", - "You can either:", - f"a. rename app to run on {original_cluster} with the --name option", - "lightning run app script.py" f"--name (new name) --cloud --cluster-id {original_cluster}", - "b. delete the existing app in the UI before running this command.", - ] - ) + if exception is not None: + old_cluster = old_cluster or DEFAULT_CLUSTER + new_cluster = new_cluster or DEFAULT_CLUSTER + assert str(exception.value) == dedent(f"""\ + Can not run app '{app_name}' on cluster {new_cluster} since it already exists on {old_cluster} + (moving apps between clusters is not supported). + + You can either: + a. rename app to run on {new_cluster} with the --name option + lightning run app script.py --name (new name) --cloud --cluster-id {new_cluster} + b. delete the app running on {old_cluster} in the UI before running this command. + """) @pytest.mark.parametrize("flow_cloud_compute", [None, CloudCompute(name="t2.medium")]) @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) From 0aa51d86eac577153d314457921b78e13a7c5252 Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Sat, 5 Nov 2022 20:00:28 -0400 Subject: [PATCH 05/17] fixup! Improve testing --- src/lightning_app/runners/cloud.py | 2 +- tests/tests_app/runners/test_cloud.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index d65271ff6d1cb..169b1d2986ca7 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -338,7 +338,7 @@ def dispatch( (moving apps between clusters is not supported). You can either: - a. rename app to run on {app_config.cluster_id} with the --name option + a. rename the app to run on {app_config.cluster_id} with the --name option lightning run app script.py --name (new name) --cloud --cluster-id {app_config.cluster_id} b. delete the app running on {existing_instance.spec.cluster_id} in the UI before running this command. """)) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index bd35b86831a03..a0d4b0d7de8c0 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -173,7 +173,7 @@ def test_new_instance_on_different_cluster(self, cloud_backend, project_id, old_ (moving apps between clusters is not supported). You can either: - a. rename app to run on {new_cluster} with the --name option + a. rename the app to run on {new_cluster} with the --name option lightning run app script.py --name (new name) --cloud --cluster-id {new_cluster} b. delete the app running on {old_cluster} in the UI before running this command. """) From 9de897ed011384b1a26ec5e60a4b17271b6af460 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 6 Nov 2022 00:00:30 +0000 Subject: [PATCH 06/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/lightning_app/runners/cloud.py | 10 ++++++--- tests/tests_app/runners/test_cloud.py | 32 ++++++++++++++++++--------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 169b1d2986ca7..7886259503587 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -7,8 +7,8 @@ import traceback from dataclasses import dataclass from pathlib import Path -from typing import Any, Callable, List, Optional, Union from textwrap import dedent +from typing import Any, Callable, List, Optional, Union import click from lightning_cloud.openapi import ( @@ -333,7 +333,9 @@ def dispatch( ) app_config.cluster_id = None if existing_instance and existing_instance.spec.cluster_id != app_config.cluster_id: - raise ValueError(dedent(f"""\ + raise ValueError( + dedent( + f"""\ Can not run app '{app_config.name}' on cluster {app_config.cluster_id} since it already exists on {existing_instance.spec.cluster_id} (moving apps between clusters is not supported). @@ -341,7 +343,9 @@ def dispatch( a. rename the app to run on {app_config.cluster_id} with the --name option lightning run app script.py --name (new name) --cloud --cluster-id {app_config.cluster_id} b. delete the app running on {existing_instance.spec.cluster_id} in the UI before running this command. - """)) + """ + ) + ) if app_config.cluster_id is not None: self._ensure_cluster_project_binding(project.project_id, app_config.cluster_id) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index a0d4b0d7de8c0..8fb3551b29a00 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -3,9 +3,9 @@ from contextlib import contextmanager from copy import copy from pathlib import Path +from textwrap import dedent from unittest import mock from unittest.mock import MagicMock -from textwrap import dedent import pytest from lightning_cloud.openapi import ( @@ -107,24 +107,32 @@ def cloud_backend(monkeypatch): def project_id(): return "test-project-id" + @contextmanager def does_not_raise(): yield + DEFAULT_CLUSTER = "litng-ai-03" + class TestAppCreationClient: """Testing the calls made using GridRestClient to create the app.""" # TODO: remove this test once there is support for multiple instances - @pytest.mark.parametrize("old_cluster,new_cluster,expected_raise", [ - ("test", "other", pytest.raises(ValueError)), - ("test", "test", does_not_raise()), - (None, None, does_not_raise()), - (None, "litng-ai-03", does_not_raise()), - ("litng-ai-03", None, does_not_raise()), - ]) - def test_new_instance_on_different_cluster(self, cloud_backend, project_id, old_cluster, new_cluster, expected_raise): + @pytest.mark.parametrize( + "old_cluster,new_cluster,expected_raise", + [ + ("test", "other", pytest.raises(ValueError)), + ("test", "test", does_not_raise()), + (None, None, does_not_raise()), + (None, "litng-ai-03", does_not_raise()), + ("litng-ai-03", None, does_not_raise()), + ], + ) + def test_new_instance_on_different_cluster( + self, cloud_backend, project_id, old_cluster, new_cluster, expected_raise + ): app_name = "test-app-name" mock_client = mock.MagicMock() @@ -168,7 +176,8 @@ def test_new_instance_on_different_cluster(self, cloud_backend, project_id, old_ if exception is not None: old_cluster = old_cluster or DEFAULT_CLUSTER new_cluster = new_cluster or DEFAULT_CLUSTER - assert str(exception.value) == dedent(f"""\ + assert str(exception.value) == dedent( + f"""\ Can not run app '{app_name}' on cluster {new_cluster} since it already exists on {old_cluster} (moving apps between clusters is not supported). @@ -176,7 +185,8 @@ def test_new_instance_on_different_cluster(self, cloud_backend, project_id, old_ a. rename the app to run on {new_cluster} with the --name option lightning run app script.py --name (new name) --cloud --cluster-id {new_cluster} b. delete the app running on {old_cluster} in the UI before running this command. - """) + """ + ) @pytest.mark.parametrize("flow_cloud_compute", [None, CloudCompute(name="t2.medium")]) @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) From ac9ccab216c10c4660f26e32d0f1c4e1ad14c0f5 Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Mon, 7 Nov 2022 10:27:16 -0500 Subject: [PATCH 07/17] pass flake8 --- src/lightning_app/runners/cloud.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 7886259503587..427c218c33a9c 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -336,14 +336,14 @@ def dispatch( raise ValueError( dedent( f"""\ - Can not run app '{app_config.name}' on cluster {app_config.cluster_id} since it already exists on {existing_instance.spec.cluster_id} - (moving apps between clusters is not supported). - - You can either: - a. rename the app to run on {app_config.cluster_id} with the --name option - lightning run app script.py --name (new name) --cloud --cluster-id {app_config.cluster_id} - b. delete the app running on {existing_instance.spec.cluster_id} in the UI before running this command. - """ + Can not run app '{app_config.name}' on cluster {app_config.cluster_id} since it already exists on {existing_instance.spec.cluster_id} + (moving apps between clusters is not supported). + + You can either: + a. rename the app to run on {app_config.cluster_id} with the --name option + lightning run app script.py --name (new name) --cloud --cluster-id {app_config.cluster_id} + b. delete the app running on {existing_instance.spec.cluster_id} in the UI before running this command. + """ # noqa: E501 ) ) From 97eb3921a1670f3e7aefcd64a5c7faae4d6b6e0b Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Mon, 7 Nov 2022 10:38:22 -0500 Subject: [PATCH 08/17] Update changelog --- src/lightning_app/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/lightning_app/CHANGELOG.md b/src/lightning_app/CHANGELOG.md index cf6302f6dd3de..872225d20df31 100644 --- a/src/lightning_app/CHANGELOG.md +++ b/src/lightning_app/CHANGELOG.md @@ -53,6 +53,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Fixed bug with Multi Node Component and add some examples ([#15557](https://github.com/Lightning-AI/lightning/pull/15557)) +- Fixed bug when launching apps on multiple clusters ([#15484](https://github.com/Lightning-AI/lightning/pull/15484)) + ## [1.8.0] - 2022-11-01 From 17af0f827b90eeaf8b997d9e7f3140d91a75ea98 Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Wed, 9 Nov 2022 12:13:00 -0500 Subject: [PATCH 09/17] Address PR feedback --- src/lightning_app/runners/cloud.py | 2 +- tests/tests_app/runners/test_cloud.py | 32 ++++++++------------------- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 427c218c33a9c..2b2da94eaf3a5 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -336,7 +336,7 @@ def dispatch( raise ValueError( dedent( f"""\ - Can not run app '{app_config.name}' on cluster {app_config.cluster_id} since it already exists on {existing_instance.spec.cluster_id} + Can not run app {app_config.name} on cluster {app_config.cluster_id} since it already exists on {existing_instance.spec.cluster_id} (moving apps between clusters is not supported). You can either: diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index 8fb3551b29a00..e19d704dd7edc 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -1,6 +1,6 @@ import logging import os -from contextlib import contextmanager +from contextlib import nullcontext as does_not_raise from copy import copy from pathlib import Path from textwrap import dedent @@ -108,11 +108,6 @@ def project_id(): return "test-project-id" -@contextmanager -def does_not_raise(): - yield - - DEFAULT_CLUSTER = "litng-ai-03" @@ -123,7 +118,10 @@ class TestAppCreationClient: @pytest.mark.parametrize( "old_cluster,new_cluster,expected_raise", [ - ("test", "other", pytest.raises(ValueError)), + ("test", "other", pytest.raises( + ValueError, + match="Can not run app test-app on cluster other since it already exists on test", + )), ("test", "test", does_not_raise()), (None, None, does_not_raise()), (None, "litng-ai-03", does_not_raise()), @@ -133,7 +131,7 @@ class TestAppCreationClient: def test_new_instance_on_different_cluster( self, cloud_backend, project_id, old_cluster, new_cluster, expected_raise ): - app_name = "test-app-name" + app_name = "test-app" mock_client = mock.MagicMock() mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( @@ -143,7 +141,9 @@ def test_new_instance_on_different_cluster( cluster_id=new_cluster ) + # Note: # backend converts "None" cluster to "litng-ai-03" + # dispatch should receive None, but API calls should return "litng-ai-03" mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse( [ Externalv1Cluster(id=old_cluster or DEFAULT_CLUSTER), @@ -170,23 +170,9 @@ def test_new_instance_on_different_cluster( # This is the main assertion: # we have an existing instance on `cluster-001` # but we want to run this app on `cluster-002` - with expected_raise as exception: + with expected_raise: cloud_runtime.dispatch(name=app_name, cluster_id=new_cluster) - if exception is not None: - old_cluster = old_cluster or DEFAULT_CLUSTER - new_cluster = new_cluster or DEFAULT_CLUSTER - assert str(exception.value) == dedent( - f"""\ - Can not run app '{app_name}' on cluster {new_cluster} since it already exists on {old_cluster} - (moving apps between clusters is not supported). - - You can either: - a. rename the app to run on {new_cluster} with the --name option - lightning run app script.py --name (new name) --cloud --cluster-id {new_cluster} - b. delete the app running on {old_cluster} in the UI before running this command. - """ - ) @pytest.mark.parametrize("flow_cloud_compute", [None, CloudCompute(name="t2.medium")]) @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) From be6b74508bdcfbce34d599d7c828bb3a5405132c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 9 Nov 2022 17:15:58 +0000 Subject: [PATCH 10/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/tests_app/runners/test_cloud.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index e19d704dd7edc..9760058c7d44d 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -118,10 +118,14 @@ class TestAppCreationClient: @pytest.mark.parametrize( "old_cluster,new_cluster,expected_raise", [ - ("test", "other", pytest.raises( - ValueError, - match="Can not run app test-app on cluster other since it already exists on test", - )), + ( + "test", + "other", + pytest.raises( + ValueError, + match="Can not run app test-app on cluster other since it already exists on test", + ), + ), ("test", "test", does_not_raise()), (None, None, does_not_raise()), (None, "litng-ai-03", does_not_raise()), @@ -173,7 +177,6 @@ def test_new_instance_on_different_cluster( with expected_raise: cloud_runtime.dispatch(name=app_name, cluster_id=new_cluster) - @pytest.mark.parametrize("flow_cloud_compute", [None, CloudCompute(name="t2.medium")]) @mock.patch("lightning_app.runners.backends.cloud.LightningClient", mock.MagicMock()) def test_run_with_default_flow_compute_config(self, monkeypatch, flow_cloud_compute): From 58c733fe662e8573dceedc3decd9642b7b7574d9 Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Wed, 9 Nov 2022 17:23:10 -0500 Subject: [PATCH 11/17] remove unused import --- tests/tests_app/runners/test_cloud.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index c8b739c39e91a..61acadfa0013c 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -3,7 +3,6 @@ from contextlib import nullcontext as does_not_raise from copy import copy from pathlib import Path -from textwrap import dedent from unittest import mock from unittest.mock import MagicMock From 7a093a8374ad2a3fc51e06d04877e669cb75e94d Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Mon, 21 Nov 2022 10:07:42 -0500 Subject: [PATCH 12/17] Reword error message --- src/lightning_app/runners/cloud.py | 7 +++---- tests/tests_app/runners/test_cloud.py | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 3e6153310db74..023643bb66a44 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -346,12 +346,11 @@ def dispatch( raise ValueError( dedent( f"""\ - Can not run app {app_config.name} on cluster {app_config.cluster_id} since it already exists on {existing_instance.spec.cluster_id} - (moving apps between clusters is not supported). + An app names {app_config.name} is already running on cluster {existing_instance.spec.cluster_id}, and you requested it to run on cluster {app_config.cluster_id}. - You can either: + In order to proceed, please either: a. rename the app to run on {app_config.cluster_id} with the --name option - lightning run app script.py --name (new name) --cloud --cluster-id {app_config.cluster_id} + lightning run app {app_entrypoint_file} --name (new name) --cloud --cluster-id {app_config.cluster_id} b. delete the app running on {existing_instance.spec.cluster_id} in the UI before running this command. """ # noqa: E501 ) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index fa8d7e143977a..0e71e114f3305 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -126,7 +126,7 @@ class TestAppCreationClient: "other", pytest.raises( ValueError, - match="Can not run app test-app on cluster other since it already exists on test", + match="already running on cluster", ), ), ("test", "test", does_not_raise()), From 3474c5be039b5ee3b34e6f58dc6b0959a9d0bd10 Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Mon, 21 Nov 2022 10:14:42 -0500 Subject: [PATCH 13/17] Error if running on cluster that doesn't exist --- src/lightning_app/runners/cloud.py | 7 +------ tests/tests_app/runners/test_cloud.py | 29 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 023643bb66a44..3038007ce5934 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -336,12 +336,7 @@ def dispatch( f"Your app last ran on cluster {app_config.cluster_id}, but that cluster " "doesn't exist anymore." ) - click.confirm( - f"{msg} Do you want to run on Lightning Cloud instead?", - abort=True, - default=True, - ) - app_config.cluster_id = None + raise ValueError(msg) if existing_instance and existing_instance.spec.cluster_id != app_config.cluster_id: raise ValueError( dedent( diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index 0e71e114f3305..b01106a161e3f 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -117,6 +117,35 @@ def project_id(): class TestAppCreationClient: """Testing the calls made using GridRestClient to create the app.""" + def test_run_on_deleted_cluster(self): + app_name = "test-app" + + mock_client = mock.MagicMock() + mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse( + memberships=[V1Membership(name="Default Project", project_id=project_id)] + ) + + mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([DEFAULT_CLUSTER]) + cloud_backend.client = mock_client + + app = mock.MagicMock() + app.flows = [] + app.frontend = {} + + existing_instance = MagicMock() + existing_instance.status.phase = V1LightningappInstanceState.STOPPED + existing_instance.spec.cluster_id = DEFAULT_CLUSTER + mock_client.lightningapp_instance_service_list_lightningapp_instances.return_value = ( + V1ListLightningappInstancesResponse(lightningapps=[existing_instance]) + ) + + cloud_runtime = cloud.CloudRuntime(app=app, entrypoint_file="entrypoint.py") + cloud_runtime._check_uploaded_folder = mock.MagicMock() + + with pytest.raises(ValueError, match="that cluster doesn't exist"): + cloud_runtime.dispatch(name=app_name, cluster_id="unknown-cluster") + + # TODO: remove this test once there is support for multiple instances @pytest.mark.parametrize( "old_cluster,new_cluster,expected_raise", From 9cd61e507c1b7b0b3009a00edfcf8c30fde15d1f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 21 Nov 2022 15:16:09 +0000 Subject: [PATCH 14/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/tests_app/runners/test_cloud.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index b01106a161e3f..05026b0e0cf38 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -145,7 +145,6 @@ def test_run_on_deleted_cluster(self): with pytest.raises(ValueError, match="that cluster doesn't exist"): cloud_runtime.dispatch(name=app_name, cluster_id="unknown-cluster") - # TODO: remove this test once there is support for multiple instances @pytest.mark.parametrize( "old_cluster,new_cluster,expected_raise", From 0701419b7385e82837a59176d3e9b35345a9ca0a Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Mon, 21 Nov 2022 10:18:06 -0500 Subject: [PATCH 15/17] fixup! Error if running on cluster that doesn't exist --- tests/tests_app/runners/test_cloud.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index 05026b0e0cf38..fff3666027897 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -117,7 +117,7 @@ def project_id(): class TestAppCreationClient: """Testing the calls made using GridRestClient to create the app.""" - def test_run_on_deleted_cluster(self): + def test_run_on_deleted_cluster(self, cloud_backend): app_name = "test-app" mock_client = mock.MagicMock() @@ -125,7 +125,9 @@ def test_run_on_deleted_cluster(self): memberships=[V1Membership(name="Default Project", project_id=project_id)] ) - mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([DEFAULT_CLUSTER]) + mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([ + Externalv1Cluster(id=DEFAULT_CLUSTER), + ]) cloud_backend.client = mock_client app = mock.MagicMock() From 165d4ba1d857943b9bb788d65bf860f5513453f4 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 21 Nov 2022 15:19:54 +0000 Subject: [PATCH 16/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/tests_app/runners/test_cloud.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/tests_app/runners/test_cloud.py b/tests/tests_app/runners/test_cloud.py index fff3666027897..a23429cc07134 100644 --- a/tests/tests_app/runners/test_cloud.py +++ b/tests/tests_app/runners/test_cloud.py @@ -125,9 +125,11 @@ def test_run_on_deleted_cluster(self, cloud_backend): memberships=[V1Membership(name="Default Project", project_id=project_id)] ) - mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse([ - Externalv1Cluster(id=DEFAULT_CLUSTER), - ]) + mock_client.cluster_service_list_clusters.return_value = V1ListClustersResponse( + [ + Externalv1Cluster(id=DEFAULT_CLUSTER), + ] + ) cloud_backend.client = mock_client app = mock.MagicMock() From cc6f4e9f2dd24a478aff16f37c0dd1e66cb156b3 Mon Sep 17 00:00:00 2001 From: Luca Furst Date: Tue, 29 Nov 2022 09:56:12 -0500 Subject: [PATCH 17/17] Remove unsued import --- src/lightning_app/runners/cloud.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lightning_app/runners/cloud.py b/src/lightning_app/runners/cloud.py index 6a359f24efc58..d3613975f6076 100644 --- a/src/lightning_app/runners/cloud.py +++ b/src/lightning_app/runners/cloud.py @@ -9,7 +9,6 @@ from textwrap import dedent from typing import Any, Callable, List, Optional, Union -import click from lightning_cloud.openapi import ( Body3, Body4,