-
Notifications
You must be signed in to change notification settings - Fork 0
Revert "PILOT-7878: CLI download of large folder on bd2data resulting in global.unhandled_exception error (Internal Server error)" #239
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -244,77 +244,52 @@ def check_download_preparing_status(self): | |||||
| executor.shutdown(wait=False) | ||||||
|
|
||||||
| @require_valid_token() | ||||||
| def download_file(self, url, local_filename, download_mode='single'): # noqa: C901 | ||||||
| def download_file(self, url, local_filename, download_mode='single'): | ||||||
| logger.info('start downloading...') | ||||||
| filename = local_filename.split('/')[-1] | ||||||
| max_retries = 3 | ||||||
| retry_count = 0 | ||||||
|
|
||||||
| while retry_count <= max_retries: | ||||||
| try: | ||||||
| with httpx.stream('GET', url, timeout=httpx.Timeout(30.0, read=300.0)) as r: | ||||||
| r.raise_for_status() | ||||||
|
|
||||||
| if r.headers.get('Content-Type') == 'application/zip' or download_mode == 'batch': | ||||||
| size = r.headers.get('Content-length') | ||||||
| self.total_size = int(size) if size else self.total_size | ||||||
|
|
||||||
| stream_iter = r.iter_bytes(chunk_size=1024) | ||||||
| if self.total_size: | ||||||
| downloaded_size = 0 | ||||||
| with open(local_filename, 'wb') as file, tqdm( | ||||||
| desc=f'Downloading {filename}', | ||||||
| total=self.total_size, | ||||||
| unit='iB', | ||||||
| unit_scale=True, | ||||||
| unit_divisor=1024, | ||||||
| bar_format='{desc} |{bar:30} {percentage:3.0f}% {remaining}', | ||||||
| ) as bar: | ||||||
| try: | ||||||
| for data in stream_iter: | ||||||
| size = file.write(data) | ||||||
| bar.update(size) | ||||||
| downloaded_size += len(data) | ||||||
| except httpx.ReadTimeout: | ||||||
| # Re-raise so it's caught by the outer exception handler | ||||||
| raise | ||||||
|
|
||||||
| if downloaded_size != self.total_size: | ||||||
| SrvErrorHandler.customized_handle( | ||||||
| ECustomizedError.DOWNLOAD_SIZE_MISMATCH, | ||||||
| if_exit=self.interactive, | ||||||
| value=(self.total_size, downloaded_size), | ||||||
| ) | ||||||
| else: | ||||||
| with open(local_filename, 'wb') as file: | ||||||
| part = 0 | ||||||
| for data in r.iter_bytes(chunk_size=1024): | ||||||
| size = file.write(data) | ||||||
| progress = '.' * part | ||||||
| click.echo(f'Downloading{progress}\r', nl=False) | ||||||
| if part > 5: | ||||||
| part = 0 | ||||||
| else: | ||||||
| part += 1 | ||||||
| logger.info('Download complete') | ||||||
|
|
||||||
| return local_filename | ||||||
|
|
||||||
| except httpx.ReadTimeout: | ||||||
| retry_count += 1 | ||||||
| if retry_count > max_retries: | ||||||
| logger.error(f'Download failed after {max_retries} attempts due to read timeout') | ||||||
| SrvErrorHandler.customized_handle( | ||||||
| ECustomizedError.DOWNLOAD_TIMEOUT, if_exit=self.interactive, value=filename | ||||||
| ) | ||||||
| return None | ||||||
| wait_time = 2**retry_count | ||||||
| logger.warning(f'Read timeout encountered, retrying in {wait_time}s ({retry_count}/{max_retries})') | ||||||
| time.sleep(wait_time) | ||||||
|
|
||||||
| except Exception as e: | ||||||
| logger.error(f'Error downloading: {e}') | ||||||
| return None | ||||||
| try: | ||||||
| with httpx.stream('GET', url) as r: | ||||||
|
||||||
| with httpx.stream('GET', url) as r: | |
| with httpx.stream('GET', url, timeout=10) as r: # 10-second timeout |
Copilot
AI
Jun 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function unconditionally returns the filename even if an exception occurred. Consider returning None or re-raising the exception to signal a failed download.
| return local_filename | |
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The complexity suppression (
# noqa: C901) was removed but the method remains large. Consider refactoring into smaller helper functions for clarity and maintainability.