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

Downloader support resume from connection reset #9422

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

austinzh
Copy link

@austinzh austinzh commented May 17, 2024

Pull Request Check List

Resolves: #3219

  • Added tests for changed code.
  • Updated documentation for changed code.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

download_file() is used in some more places (HTTPRepository, DirectOrigin), which might not be relevant for installing but only for locking. Maybe, connection resets are not that relevant for locking. On the other side, it feels more consistent to also retry in this case. What do you think?

HTTPRepository already has the config available, so it is easy to use the setting. For DirectOrigin, we had to add another parameter.

@@ -131,6 +131,7 @@ class Config:
"modern-installation": True,
"parallel": True,
"max-workers": None,
"max-retries": 0,
Copy link
Member

Choose a reason for hiding this comment

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

You have to add this setting in two more places:

  • Some lines below in _get_normalizer
  • In ConfigCommand.unique_config_values

Copy link
Author

Choose a reason for hiding this comment

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

Done.


**Environment Variable**: `POETRY_INSTALLER_MAX_RETRIES`

*Introduced in 1.9.0*
Copy link
Member

Choose a reason for hiding this comment

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

Reminder to myself: The next version might be 2.0.0.

docs/configuration.md Outdated Show resolved Hide resolved
@austinzh
Copy link
Author

austinzh commented Jun 12, 2024

download_file() is used in some more places (HTTPRepository, DirectOrigin), which might not be relevant for installing but only for locking. Maybe, connection resets are not that relevant for locking. On the other side, it feels more consistent to also retry in this case. What do you think?

HTTPRepository already has the config available, so it is easy to use the setting. For DirectOrigin, we had to add another parameter.

Sure. I can add parameter to them, It's my first time working in this repo, I agree that even they are not much needed for retry it's better to make it all consistent.

But for the config, should we still use installer.max-retries? I think it's still valid name right? Or we add solver.max-retries ?
@radoering

austinzh and others added 3 commits June 12, 2024 08:52
Co-authored-by: Randy Döring <30527984+radoering@users.noreply.github.com>
@radoering
Copy link
Member

But for the config, should we still use installer.max-retries? I think it's still valid name right? Or we add solver.max-retries ?

Good question. Thinking about it, installer.max-retries might have been a bit ambiguous from the start: We only retry the download not the complete installation. I think we should introduce a new section, maybe call it network or requests? (We already have a POETRY_REQUESTS_TIMEOUT, which is not yet available via config but only via environment variable; anyway, I think that one would fit into the same section.)

@austinzh austinzh requested a review from radoering June 24, 2024 04:06
Copy link
Author

@austinzh austinzh left a comment

Choose a reason for hiding this comment

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

Change config to "requests.max-retries"

@@ -131,6 +131,7 @@ class Config:
"modern-installation": True,
"parallel": True,
"max-workers": None,
"max-retries": 0,
Copy link
Author

Choose a reason for hiding this comment

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

Done.

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.

Frequent intermittent connection failures
2 participants