[App] Wrap LightningClient methods with retry wrapper by default #16382

akihironitta · 2023-01-16T16:19:39Z

What does this PR do?

Addresses https://github.com/gridai/grid/pull/9033#discussion_r1071372526.

This PR can impact these occurrences

$ git grep "LightningClient(" src/lightning_app | grep -v "import "
src/lightning_app/cli/cmd_apps.py:        self.api_client = LightningClient()
src/lightning_app/cli/cmd_clusters.py:        self.api_client = LightningClient()
src/lightning_app/cli/cmd_ssh_keys.py:        self.api_client = LightningClient()
src/lightning_app/cli/commands/connection.py:                client = LightningClient()
src/lightning_app/cli/commands/logs.py:    client = LightningClient()
src/lightning_app/cli/lightning_cli.py:    client = LightningClient()
src/lightning_app/runners/backends/cloud.py:        self.client = LightningClient(retry=True)
src/lightning_app/testing/testing.py:    client = LightningClient()
src/lightning_app/testing/testing.py:        client = LightningClient()
src/lightning_app/testing/testing.py:    client = LightningClient()
src/lightning_app/utilities/app_helpers.py:    client = LightningClient()
src/lightning_app/utilities/cli_helpers.py:        client = LightningClient()
src/lightning_app/utilities/commands/base.py:                client = LightningClient()
src/lightning_app/utilities/network.py:class LightningClient(GridRestClient):
src/lightning_app/utilities/port.py:    client = LightningClient()
src/lightning_app/utilities/port.py:    client = LightningClient()
src/lightning_app/utilities/port.py:    client = LightningClient()
src/lightning_app/utilities/secrets.py:    lightning_client = LightningClient()

Does your PR introduce any breaking changes? If yes, please list them.

Changes retry=False to retry=True:

- lightning_app.utilities.network.LightningClient(retry=False)
+ lightning_app.utilities.network.LightningClient(retry=True)

Before submitting

Was this discussed/approved 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?
[n/a] Did you make sure to update the documentation with your changes? (if necessary)
[n/a] 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:

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

Did you have fun?

Make sure you had fun coding 🙃

cc @Borda @tchaton

github-actions · 2023-01-16T16:20:11Z

⚡ Required checks status: All passing 🟢

Groups summary

🟢 lightning_app: Tests workflow

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

These checks are required after the changes to src/lightning_app/cli/cmd_apps.py, src/lightning_app/cli/cmd_clusters.py, src/lightning_app/cli/cmd_ssh_keys.py, src/lightning_app/cli/commands/connection.py, src/lightning_app/cli/commands/logs.py, src/lightning_app/cli/lightning_cli.py, src/lightning_app/runners/backends/cloud.py, src/lightning_app/utilities/cli_helpers.py, src/lightning_app/utilities/commands/base.py, src/lightning_app/utilities/network.py, tests/tests_app/utilities/test_network.py.

🟢 lightning_app: Examples

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

These checks are required after the changes to src/lightning_app/cli/cmd_apps.py, src/lightning_app/cli/cmd_clusters.py, src/lightning_app/cli/cmd_ssh_keys.py, src/lightning_app/cli/commands/connection.py, src/lightning_app/cli/commands/logs.py, src/lightning_app/cli/lightning_cli.py, src/lightning_app/runners/backends/cloud.py, src/lightning_app/utilities/cli_helpers.py, src/lightning_app/utilities/commands/base.py, src/lightning_app/utilities/network.py.

🟢 lightning_app: Azure

Check ID	Status
App.cloud-e2e	success	✅

These checks are required after the changes to src/lightning_app/cli/cmd_apps.py, src/lightning_app/cli/cmd_clusters.py, src/lightning_app/cli/cmd_ssh_keys.py, src/lightning_app/cli/commands/connection.py, src/lightning_app/cli/commands/logs.py, src/lightning_app/cli/lightning_cli.py, src/lightning_app/runners/backends/cloud.py, src/lightning_app/utilities/cli_helpers.py, src/lightning_app/utilities/commands/base.py, src/lightning_app/utilities/network.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/cli/cmd_apps.py, src/lightning_app/cli/cmd_clusters.py, src/lightning_app/cli/cmd_ssh_keys.py, src/lightning_app/cli/commands/connection.py, src/lightning_app/cli/commands/logs.py, src/lightning_app/cli/lightning_cli.py, src/lightning_app/runners/backends/cloud.py, src/lightning_app/utilities/cli_helpers.py, src/lightning_app/utilities/commands/base.py, src/lightning_app/utilities/network.py.

🟢 mypy

Check ID	Status
mypy	success	✅

These checks are required after the changes to src/lightning_app/cli/cmd_apps.py, src/lightning_app/cli/cmd_clusters.py, src/lightning_app/cli/cmd_ssh_keys.py, src/lightning_app/cli/commands/connection.py, src/lightning_app/cli/commands/logs.py, src/lightning_app/cli/lightning_cli.py, src/lightning_app/runners/backends/cloud.py, src/lightning_app/utilities/cli_helpers.py, src/lightning_app/utilities/commands/base.py, src/lightning_app/utilities/network.py.

🟢 install

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

These checks are required after the changes to src/lightning_app/cli/cmd_apps.py, src/lightning_app/cli/cmd_clusters.py, src/lightning_app/cli/cmd_ssh_keys.py, src/lightning_app/cli/commands/connection.py, src/lightning_app/cli/commands/logs.py, src/lightning_app/cli/lightning_cli.py, src/lightning_app/runners/backends/cloud.py, src/lightning_app/utilities/cli_helpers.py, src/lightning_app/utilities/commands/base.py, src/lightning_app/utilities/network.py.

🟢 link-check

Check ID	Status
markdown-link-check	success	✅

These checks are required after the changes to src/lightning_app/CHANGELOG.md.

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.

awaelchli · 2023-01-16T16:25:56Z

@akihironitta Please have a look at the motivation from #15412. There are some consequences in the CLI to doing so.

akihironitta · 2023-01-17T04:17:59Z

@tchaton What would you suggest here? Should we still have retry=True as the default value and turn it off for CLI-related places to avoid the issue described in #15412?

tchaton

LGTM !

akihironitta · 2023-01-19T05:21:27Z

Given #16382 (comment), I see two options:

Close this PR
Disable retry in CLI-related codebase and merge this PR

I'll mark this PR as ready for review again once I update this PR (2.).

awaelchli · 2023-01-19T10:45:43Z

@akihironitta 2) sounds good to me. To be sure, I would also check the usages in other places than the CLI and see if the default value make sense or whether it should be flipped too.

tests/tests_app/utilities/test_network.py

…tning-AI/lightning into bugfix/enable_retry_wrapper

changes were made that require re-review

) Co-authored-by: thomas chaton <[email protected]> Co-authored-by: thomas <[email protected]> (cherry picked from commit d6b62da)

enable retry in LightningClient

8c5670b

akihironitta requested review from tchaton, lantiga, awaelchli, hhsecond and ethanwharris as code owners January 16, 2023 16:19

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

akihironitta changed the title ~~Wrap LightningClient methods with retry wrapper by default~~ [App] Wrap LightningClient methods with retry wrapper by default Jan 16, 2023

udpate chglog

71cf4a2

akihironitta self-assigned this Jan 16, 2023

Use default

d9a46b9

Borda approved these changes Jan 16, 2023

View reviewed changes

mergify bot added the has conflicts label Jan 17, 2023

Merge branch 'master' into bugfix/enable_retry_wrapper

1e69eed

mergify bot removed the has conflicts label Jan 17, 2023

tchaton approved these changes Jan 18, 2023

View reviewed changes

mergify bot added the ready PRs ready to be merged label Jan 18, 2023

akihironitta marked this pull request as draft January 19, 2023 05:18

mergify bot removed the ready PRs ready to be merged label Jan 19, 2023

Merge branch 'master' into bugfix/enable_retry_wrapper

4cca896

akihironitta added the priority: 0 High priority task label Jan 20, 2023

akihironitta added 3 commits January 21, 2023 01:10

update test

ba7051f

keep retry=False in some places

fdd9cec

udpate chglog

9f61b2f

akihironitta marked this pull request as ready for review January 20, 2023 16:32

Merge branch 'master' into bugfix/enable_retry_wrapper

80523de

mergify bot added the ready PRs ready to be merged label Jan 20, 2023

akihironitta added the bug Something isn't working label Jan 20, 2023

awaelchli previously approved these changes Jan 20, 2023

View reviewed changes

tests/tests_app/utilities/test_network.py Outdated Show resolved Hide resolved

awaelchli added this to the v1.9.x milestone Jan 20, 2023

mergify bot added has conflicts and removed ready PRs ready to be merged labels Jan 23, 2023

Merge branch 'master' into bugfix/enable_retry_wrapper

17daee8

mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Jan 23, 2023

thomas and others added 2 commits January 23, 2023 16:38

update

993cd14

Merge branch 'master' into bugfix/enable_retry_wrapper

06d7037

tchaton enabled auto-merge (squash) January 23, 2023 16:54

thomas added 2 commits January 23, 2023 16:59

update

b93f817

Merge branch 'bugfix/enable_retry_wrapper' of https://github.com/Ligh…

b4e98c4

…tning-AI/lightning into bugfix/enable_retry_wrapper

awaelchli approved these changes Jan 23, 2023

View reviewed changes

tchaton merged commit d6b62da into master Jan 23, 2023

tchaton deleted the bugfix/enable_retry_wrapper branch January 23, 2023 18:30

awaelchli mentioned this pull request Feb 5, 2023

Add retry to LightningClient when handling headless apps #16629

Closed

8 tasks

Borda pushed a commit that referenced this pull request Feb 9, 2023

[App] Wrap LightningClient methods with retry wrapper by default (#16382

f69e50b

) Co-authored-by: thomas chaton <[email protected]> Co-authored-by: thomas <[email protected]> (cherry picked from commit d6b62da)

lantiga pushed a commit that referenced this pull request Feb 10, 2023

[App] Wrap LightningClient methods with retry wrapper by default (#16382

fc8c48f

) Co-authored-by: thomas chaton <[email protected]> Co-authored-by: thomas <[email protected]> (cherry picked from commit d6b62da)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App] Wrap LightningClient methods with retry wrapper by default #16382

[App] Wrap LightningClient methods with retry wrapper by default #16382

akihironitta commented Jan 16, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 16, 2023 •

edited

Loading

awaelchli commented Jan 16, 2023

akihironitta commented Jan 17, 2023

tchaton left a comment

akihironitta commented Jan 19, 2023

awaelchli commented Jan 19, 2023

[App] Wrap LightningClient methods with retry wrapper by default #16382

[App] Wrap LightningClient methods with retry wrapper by default #16382

Conversation

akihironitta commented Jan 16, 2023 • edited by github-actions bot Loading

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

github-actions bot commented Jan 16, 2023 • edited Loading

⚡ Required checks status: All passing 🟢

Groups summary

awaelchli commented Jan 16, 2023

akihironitta commented Jan 17, 2023

tchaton left a comment

Choose a reason for hiding this comment

akihironitta commented Jan 19, 2023

awaelchli commented Jan 19, 2023

akihironitta commented Jan 16, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Jan 16, 2023 •

edited

Loading