Resolve Lightning App with remote storage #17426

tchaton · 2023-04-21T10:10:57Z

What does this PR do?

Currently, accessing the Drive or Path doesn't work. This PR should address it.

Fixes #<issue_number>

Before submitting

Was this discussed/agreed via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Reviewer checklist

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

cc @Borda @carmocca

github-actions · 2023-04-21T10:11:28Z

⚡ Required checks status: All passing 🟢

Groups summary

🟢 lightning_app: Tests workflow

Check ID	Status
app-pytest (macOS-11, lightning, 3.8, latest)	success	✅
app-pytest (macOS-11, lightning, 3.8, oldest)	success	✅
app-pytest (macOS-11, app, 3.9, latest)	success	✅
app-pytest (ubuntu-20.04, lightning, 3.8, latest)	success	✅
app-pytest (ubuntu-20.04, lightning, 3.8, oldest)	success	✅
app-pytest (ubuntu-20.04, app, 3.9, latest)	success	✅
app-pytest (windows-2022, lightning, 3.8, latest)	success	✅
app-pytest (windows-2022, lightning, 3.8, oldest)	success	✅
app-pytest (windows-2022, app, 3.8, latest)	success	✅

These checks are required after the changes to src/lightning/app/core/constants.py, src/lightning/app/core/queues.py, src/lightning/app/runners/cloud.py, src/lightning/app/storage/path.py, src/lightning/app/testing/testing.py, src/lightning/app/utilities/app_logs.py, src/lightning/app/utilities/packaging/cloud_compute.py, tests/tests_app/core/test_constants.py, tests/tests_app/core/test_lightning_app.py, tests/tests_app/core/test_lightning_flow.py, tests/tests_app/runners/test_cloud.py, tests/tests_app/storage/test_path.py, tests/tests_app/structures/test_structures.py, tests/tests_app/utilities/packaging/test_cloud_compute.py, tests/tests_app/utilities/test_load_app.py.

🟢 lightning_app: Examples

Check ID	Status
app-examples (macOS-11, lightning, 3.9, latest)	success	✅
app-examples (macOS-11, lightning, 3.9, oldest)	success	✅
app-examples (macOS-11, app, 3.9, latest)	success	✅
app-examples (ubuntu-20.04, lightning, 3.9, latest)	success	✅
app-examples (ubuntu-20.04, lightning, 3.9, oldest)	success	✅
app-examples (ubuntu-20.04, app, 3.9, latest)	success	✅
app-examples (windows-2022, lightning, 3.9, latest)	success	✅
app-examples (windows-2022, lightning, 3.9, oldest)	success	✅
app-examples (windows-2022, app, 3.9, latest)	success	✅

These checks are required after the changes to src/lightning/app/core/constants.py, src/lightning/app/core/queues.py, src/lightning/app/runners/cloud.py, src/lightning/app/storage/path.py, src/lightning/app/testing/testing.py, src/lightning/app/utilities/app_logs.py, src/lightning/app/utilities/packaging/cloud_compute.py, tests/integrations_app/public/test_v0_app.py.

🟢 lightning_app: Azure

Check ID	Status
App.cloud-e2e	success	✅

These checks are required after the changes to .azure/app-cloud-e2e.yml, src/lightning/app/core/constants.py, src/lightning/app/core/queues.py, src/lightning/app/runners/cloud.py, src/lightning/app/storage/path.py, src/lightning/app/testing/testing.py, src/lightning/app/utilities/app_logs.py, src/lightning/app/utilities/packaging/cloud_compute.py, tests/integrations_app/public/test_v0_app.py.

🟢 lightning_app: Docs

Check ID	Status
make-doctest (app)	success	✅
make-html (app)	success	✅

These checks are required after the changes to src/lightning/app/core/constants.py, src/lightning/app/core/queues.py, src/lightning/app/runners/cloud.py, src/lightning/app/storage/path.py, src/lightning/app/testing/testing.py, src/lightning/app/utilities/app_logs.py, src/lightning/app/utilities/packaging/cloud_compute.py.

🟢 mypy

Check ID	Status
mypy	success	✅

These checks are required after the changes to src/lightning/app/core/constants.py, src/lightning/app/core/queues.py, src/lightning/app/runners/cloud.py, src/lightning/app/storage/path.py, src/lightning/app/testing/testing.py, src/lightning/app/utilities/app_logs.py, src/lightning/app/utilities/packaging/cloud_compute.py.

🟢 install

Check ID	Status
install-pkg (ubuntu-22.04, app, 3.8)	success	✅
install-pkg (ubuntu-22.04, app, 3.10)	success	✅
install-pkg (ubuntu-22.04, fabric, 3.8)	success	✅
install-pkg (ubuntu-22.04, fabric, 3.10)	success	✅
install-pkg (ubuntu-22.04, pytorch, 3.8)	success	✅
install-pkg (ubuntu-22.04, pytorch, 3.10)	success	✅
install-pkg (ubuntu-22.04, lightning, 3.8)	success	✅
install-pkg (ubuntu-22.04, lightning, 3.10)	success	✅
install-pkg (ubuntu-22.04, notset, 3.8)	success	✅
install-pkg (ubuntu-22.04, notset, 3.10)	success	✅
install-pkg (macOS-12, app, 3.8)	success	✅
install-pkg (macOS-12, app, 3.10)	success	✅
install-pkg (macOS-12, fabric, 3.8)	success	✅
install-pkg (macOS-12, fabric, 3.10)	success	✅
install-pkg (macOS-12, pytorch, 3.8)	success	✅
install-pkg (macOS-12, pytorch, 3.10)	success	✅
install-pkg (macOS-12, lightning, 3.8)	success	✅
install-pkg (macOS-12, lightning, 3.10)	success	✅
install-pkg (macOS-12, notset, 3.8)	success	✅
install-pkg (macOS-12, notset, 3.10)	success	✅
install-pkg (windows-2022, app, 3.8)	success	✅
install-pkg (windows-2022, app, 3.10)	success	✅
install-pkg (windows-2022, fabric, 3.8)	success	✅
install-pkg (windows-2022, fabric, 3.10)	success	✅
install-pkg (windows-2022, pytorch, 3.8)	success	✅
install-pkg (windows-2022, pytorch, 3.10)	success	✅
install-pkg (windows-2022, lightning, 3.8)	success	✅
install-pkg (windows-2022, lightning, 3.10)	success	✅
install-pkg (windows-2022, notset, 3.8)	success	✅
install-pkg (windows-2022, notset, 3.10)	success	✅

These checks are required after the changes to src/lightning/app/core/constants.py, src/lightning/app/core/queues.py, src/lightning/app/runners/cloud.py, src/lightning/app/storage/path.py, src/lightning/app/testing/testing.py, src/lightning/app/utilities/app_logs.py, src/lightning/app/utilities/packaging/cloud_compute.py.

Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

src/lightning/app/core/constants.py

src/lightning/app/storage/path.py

src/lightning/app/testing/testing.py

Borda · 2023-04-23T21:53:56Z

.azure/app-cloud-e2e.yml

@@ -67,57 +67,42 @@ jobs:
        'App: v0_app':
          name: "v0_app"
          dir: "public"
-          queue_type: "redis"


@ethanwharris why do we drop redis?

Yes, this won't be supported going forward.

Borda · 2023-04-23T21:54:18Z

examples/app/commands_and_api/.lightningignore

@@ -0,0 +1 @@
+venv/


is this used?

This is automatically generated when the app runs. Better to have it in the repo.

Borda · 2023-04-23T21:55:12Z

src/lightning/app/core/constants.py

-    if value not in _CLUSTER_DRIVERS:
-        raise ValueError(f"Found {value} cluster driver. The value needs to be in {_CLUSTER_DRIVERS}.")
-    return value
+    return "direct"


why unset LIGHTNING_CLUSTER_DRIVER is not enough?

This isn't used and we can drop it entirely.

For another PR once the fire is being us.

Borda · 2023-04-23T21:56:02Z

src/lightning/app/core/queues.py

@@ -388,19 +388,30 @@ def get(self, timeout: Optional[float] = None) -> Any:
        if timeout is None:
            while True:
                try:
-                    return self._get()
+                    try:


let's use contextmanager.suppress(...)

Borda · 2023-04-23T21:56:42Z

src/lightning/app/core/queues.py

-            return self._get()
+            try:
+                return self._get()
+            except requests.exceptions.HTTPError:


can you pls add a note about why this error happens and it is fine to ignore it?

It can happen if the queue isn't responsive. This would lead for the app to die entirely. This adds some sort of fault tolerancy during CP re-deployment.

Borda · 2023-04-23T21:58:19Z

tests/integrations_app/public/test_v0_app.py

@@ -53,6 +53,7 @@ def check_content(button_name, text_content):
    has_logs = False
    while not has_logs:
        for log in fetch_logs(["flow"]):
+            print(log)


Good catch !

tests/tests_app/core/test_constants.py

the one break was removed

* update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update * update --------- Co-authored-by: thomas <thomas@thomass-MacBook-Pro.local> (cherry picked from commit 3688b64)

--------- Co-authored-by: thomas <thomas@thomass-MacBook-Pro.local>

--------- Co-authored-by: thomas <thomas@thomass-MacBook-Pro.local> (cherry picked from commit 3688b64) # Conflicts: # examples/app_commands_and_api/.lightningignore # tests/tests_app/core/test_constants.py # tests/tests_app/runners/test_cloud.py

update

50b9172

tchaton requested review from lantiga, awaelchli, ethanwharris and Borda as code owners April 21, 2023 10:10

github-actions bot added the app (removed) Generic label for Lightning App package label Apr 21, 2023

update

ef77a84

awaelchli approved these changes Apr 21, 2023

View reviewed changes

src/lightning/app/core/constants.py Show resolved Hide resolved

awaelchli added the bug Something isn't working label Apr 21, 2023

awaelchli added this to the 2.0.x milestone Apr 21, 2023

ethanwharris reviewed Apr 21, 2023

View reviewed changes

src/lightning/app/storage/path.py Show resolved Hide resolved

update

1220b33

ethanwharris approved these changes Apr 21, 2023

View reviewed changes

mergify bot added the ready PRs ready to be merged label Apr 21, 2023

update

1ce64b7

Borda changed the title ~~PO: Resolve Lightning App with remote storage~~ Resolve Lightning App with remote storage Apr 21, 2023

Borda enabled auto-merge (squash) April 21, 2023 10:57

thomas added 2 commits April 21, 2023 12:05

update

525c1d1

update

4680b40

Borda previously requested changes Apr 21, 2023

View reviewed changes

src/lightning/app/testing/testing.py Outdated Show resolved Hide resolved

mergify bot removed the ready PRs ready to be merged label Apr 21, 2023

thomas added 2 commits April 21, 2023 16:00

update

76050cb

update

a3cbdf3

tchaton requested a review from carmocca as a code owner April 21, 2023 15:26

github-actions bot added the ci Continuous Integration label Apr 21, 2023

thomas added 4 commits April 21, 2023 17:10

update

ccf036d

update

ce0d73d

update

7867342

update

e9acf43

update

f58fd07

github-actions bot removed the ci Continuous Integration label Apr 23, 2023

thomas added 8 commits April 23, 2023 16:51

update

d5fdc2f

update

44d8641

update

88875a1

update

a64743e

update

423f9e9

update

dbecd0d

update

dbb3bd1

update

1b04de3

github-actions bot added the ci Continuous Integration label Apr 23, 2023

thomas added 4 commits April 23, 2023 20:31

update

f0832d9

update

b7b41df

update

355264d

update

cd004fa

Borda reviewed Apr 23, 2023

View reviewed changes

Borda self-requested a review April 23, 2023 21:59

mergify bot added the ready PRs ready to be merged label Apr 23, 2023

williamFalcon disabled auto-merge April 23, 2023 22:08

williamFalcon merged commit 3688b64 into master Apr 23, 2023

williamFalcon deleted the debug_drive branch April 23, 2023 22:08

tchaton added a commit that referenced this pull request Apr 24, 2023

Resolve Lightning App with remote storage (#17426)

31745db

--------- Co-authored-by: thomas <thomas@thomass-MacBook-Pro.local>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve Lightning App with remote storage #17426

Resolve Lightning App with remote storage #17426

tchaton commented Apr 21, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading

Borda Apr 23, 2023

tchaton Apr 24, 2023

Borda Apr 23, 2023

tchaton Apr 24, 2023

Borda Apr 23, 2023

tchaton Apr 24, 2023

tchaton Apr 24, 2023

Borda Apr 23, 2023

Borda Apr 23, 2023

tchaton Apr 24, 2023

Borda Apr 23, 2023

tchaton Apr 24, 2023

		@@ -0,0 +1 @@
		venv/

Resolve Lightning App with remote storage #17426

Resolve Lightning App with remote storage #17426

Conversation

tchaton commented Apr 21, 2023 • edited by github-actions bot Loading

What does this PR do?

PR review

github-actions bot commented Apr 21, 2023 • edited Loading

⚡ Required checks status: All passing 🟢

Groups summary

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

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

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

tchaton commented Apr 21, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Apr 21, 2023 •

edited

Loading