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.

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

fix(cli, studio): don't retry on non request/http errors #907

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

shcheklein
Copy link
Member

@shcheklein shcheklein commented Feb 8, 2025

W/o this fix, if team is not set via env variable DVC_STUDIO_TEAM and / or DataChain config, self.team in Studio remote client raises a DataChainError (which is fine). What is not fine is that _send_request keeps retying with an exponential backoff on those.

Pretty much means that first time someone is trying to run something like datachain ds pull ... there are big chances to hit a long hang w/o any messages at all. Then if you are like to wait you can see that it just expect team to be set.

Also improves the messaging around get_team errors a bit.

TODO:

  • Add tests (probably not needed in this case)
  • Add logger to at least see what is going on with retries in debug mode.

@shcheklein shcheklein self-assigned this Feb 8, 2025
@shcheklein shcheklein requested review from amritghimire and a team February 8, 2025 20:05
Copy link

codecov bot commented Feb 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.69%. Comparing base (03d7440) to head (7acb0f7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #907   +/-   ##
=======================================
  Coverage   87.69%   87.69%           
=======================================
  Files         130      130           
  Lines       11661    11665    +4     
  Branches     1586     1586           
=======================================
+ Hits        10226    10230    +4     
  Misses       1038     1038           
  Partials      397      397           
Flag Coverage Δ
datachain 87.62% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shcheklein shcheklein force-pushed the fix/cli-hangs-on-get-team-exception-retries branch from dede005 to 7acb0f7 Compare February 8, 2025 20:28
Copy link

cloudflare-workers-and-pages bot commented Feb 8, 2025

Deploying datachain-documentation with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7acb0f7
Status: ✅  Deploy successful!
Preview URL: https://16cfc44e.datachain-documentation.pages.dev
Branch Preview URL: https://fix-cli-hangs-on-get-team-ex.datachain-documentation.pages.dev

View logs

def _send_request(
self, route: str, data: dict[str, Any], method: Optional[str] = "POST"
) -> Response[Any]:
"""
Function that communicate Studio API.
It will raise an exception, and try to retry, if 5xx status code is
returned, or if ConnectionError or Timeout exceptions are thrown from
requests lib
returned, or if Timeout exceptions is thrown from the requests lib
Copy link
Member Author

Choose a reason for hiding this comment

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

[C]: removed ConnectionError from the one we retry on. It is usually Proxy, or something SSL - don't think we can recover from those

@shcheklein shcheklein merged commit 1e8bec1 into main Feb 8, 2025
37 checks passed
@shcheklein shcheklein deleted the fix/cli-hangs-on-get-team-exception-retries branch February 8, 2025 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants