Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ENG-1524: Change retry mechansim default in LightningClient #15412

Merged
merged 34 commits into from
Nov 9, 2022
Merged

ENG-1524: Change retry mechansim default in LightningClient #15412

merged 34 commits into from
Nov 9, 2022

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Oct 30, 2022 •
edited by github-actions bot
Loading

What does this PR do?

Fixes ENG-1524

Previously, calls to the LightningClient would follow an exponential backoff retry mechanism when calling APIs. This is desired for calls in the cloud, but not for calls in the CLI. This PR changes this default. The cloud backend will still use the retry mechanism, but the LightningClient instances used in the CLI will immediately error out when an API call can't reach the endpoint.

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?
  • 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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read 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?

I made sure I had fun coding 🙃

cc @Borda

@github-actions github-actions bot added the app (removed) Generic label for Lightning App package label Oct 30, 2022
@awaelchli awaelchli added app:cli bug Something isn't working labels Oct 30, 2022
@awaelchli awaelchli modified the milestones: v1.8.x, v1.9 Oct 30, 2022
@awaelchli
Copy link
Contributor Author

The test tests_app/utilities/test_secrets.py::test_names_to_ids is failing here in the CI but not locally. I suspect we set an environment variable somewhere that I don't have set locally, but I don't know where. I can't relate it to the changes in this PR. @tchaton @manskx any idea what it could be?

@awaelchli awaelchli self-assigned this Oct 31, 2022
nmiculinic
nmiculinic approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@nmiculinic nmiculinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

tchaton
tchaton approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@mergify mergify bot added the has conflicts label Nov 8, 2022
@mergify mergify bot removed the ready PRs ready to be merged label Nov 8, 2022
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Nov 8, 2022
@mergify mergify bot added the ready PRs ready to be merged label Nov 8, 2022
@Borda Borda enabled auto-merge (squash) November 8, 2022 17:00
@justusschock
Copy link
Member

@awaelchli mind have a look at failing tests? Could be due to secrets?

@Borda Borda merged commit 7decc50 into master Nov 9, 2022
@Borda Borda deleted the bugfix/disable-retry-for-cli branch November 9, 2022 21:19
Borda pushed a commit that referenced this pull request Nov 10, 2022
@awaelchli @Borda
ENG-1524: Change retry mechansim default in LightningClient (#15412)
ef79369 
* Change retry mechansim default in LightningClient
* add changelog
* fix weird patching mechanism
* remove unused import
* hack the system
* try again, maybe something has changed in the package

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
(cherry picked from commit 7decc50)
lexierule pushed a commit that referenced this pull request Nov 10, 2022
@awaelchli @lexierule
ENG-1524: Change retry mechansim default in LightningClient (#15412)
da721c2 
* Change retry mechansim default in LightningClient
* add changelog
* fix weird patching mechanism
* remove unused import
* hack the system
* try again, maybe something has changed in the package

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
(cherry picked from commit 7decc50)
@nicolai86 nicolai86 mentioned this pull request Dec 7, 2022
Merged
@awaelchli awaelchli mentioned this pull request Jan 16, 2023
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manskx manskx manskx left review comments

@tchaton tchaton tchaton approved these changes

@Borda Borda Borda approved these changes

@nmiculinic nmiculinic nmiculinic approved these changes

@hhsecond hhsecond Awaiting requested review from hhsecond

@lantiga lantiga Awaiting requested review from lantiga lantiga is a code owner

@carmocca carmocca Awaiting requested review from carmocca

@akihironitta akihironitta Awaiting requested review from akihironitta

@otaj otaj Awaiting requested review from otaj

Assignees

@awaelchli awaelchli

Labels
app (removed) Generic label for Lightning App package bug Something isn't working ready PRs ready to be merged
Projects
No open projects
Frameworks Planning
Status: Done
Milestone
v1.8.x
Development

Successfully merging this pull request may close these issues.
6 participants
@awaelchli @justusschock @nmiculinic @Borda @manskx @tchaton