Fix: Network hiccups using Retry logic #306
Conversation
src/fromager/sdist.py
Outdated
| @@ -311,7 +311,7 @@ def download_wheel( | |||
| wheel_filename = output_directory / os.path.basename(urlparse(wheel_url).path) | |||
| if not wheel_filename.exists(): | |||
| logger.info(f"{req.name}: downloading pre-built wheel {wheel_url}") | |||
| wheel_filename = _download_wheel_check(output_directory, wheel_url) | |||
| wheel_filename = _download_wheel_check(output_directory, wheel_url, ctx) | |||
There was a problem hiding this comment.
We have the context as the first argument to functions everywhere else, could you follow that pattern here, too?
src/fromager/sources.py
Outdated
| @@ -165,7 +164,7 @@ def default_download_source( | |||
| ) | |||
|
|
|||
| source_filename = _download_source_check( | |||
| ctx.sdists_downloads, url, destination_filename | |||
| ctx.sdists_downloads, url, ctx, destination_filename | |||
There was a problem hiding this comment.
Same comment about argument ordering.
src/fromager/sources.py
Outdated
| @@ -213,7 +218,7 @@ def download_url( | |||
| return outfile | |||
| # Open the URL first in case that fails, so we don't end up with an empty file. | |||
| logger.debug(f"reading from {url}") | |||
| with requests.get(url, stream=True) as r: | |||
| with ctx.requests.get(url, stream=True) as r: | |||
There was a problem hiding this comment.
How does retrying work with streaming? If it starts over, the caller needs to understand that so it doesn't just write duplicate copies of the data to the same file, right?
There was a problem hiding this comment.
Yeah, need to look at this
There was a problem hiding this comment.
Found this: https://stackoverflow.com/questions/25860105/python-requests-package-lost-connection-while-streaming but not sure whether it will help our cause
There was a problem hiding this comment.
https://gist.github.com/tobiasraabe/58adee67de619ce621464c1a6511d7d9 looks potentially useful. The Range HTTP header tells the server where to start sending data, based on how much has already been downloaded. https://medium.com/@lope.ai/downloading-files-over-http-with-python-requests-e12e6b795e43 shows another example of tracking that.
There was a problem hiding this comment.
Agreed
I have tried to use Retry header in the latest version of this PR
| # Open the URL first in case that fails, so we don't end up with an empty file. | ||
| logger.debug(f"reading from {url}") | ||
| with requests.get(url, stream=True) as r: | ||
| with ctx.requests.get(url, stream=True, headers=headers) as r: |
There was a problem hiding this comment.
I think we're going to need a loop here to handle the retry.
Let's set this aside for now and come back when we have more time to work on it together.
|
Converting to draft based on the last review comment |
Fixes: #222
As per discussion here: #228