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

Improve download experience #106

Merged
merged 14 commits into from
Dec 16, 2024
Merged

Improve download experience #106

merged 14 commits into from
Dec 16, 2024

Conversation

mephenor
Copy link
Member

@mephenor mephenor commented Dec 12, 2024

This PR includes several changes:

  • Bump version to 1.5.2. Might make more sense to leave it as is for now and bump it to 1.6.0. once the caching changes are included
  • Added overwrite flag to file download and changed default behaviour to skip existing files instead of aborting with an exception
  • Simplified retry handler instantiation. This is now a singleton populated using the CONFIG singleton. No explicit instantiation at the site of use is needed now
  • Merged the two slightly differing await_download_url and get_download_url methods into one and renamed it to fetch_download_url
  • max_wait_time should now always be relative to the last time one or multiple files became available for download
  • Changed logic from potentially initiating staging for all files first before starting any download to always move to downloading files as soon as possible
  • Added more user facing messages during download preparation so progress is now visible and the connector does no longer appear to be hanging
  • Drastically reduced amount of calls to WPS. Now only one call is made for staging another, one when starting the file download and one when getting the file envelope. For the latter two the call to get the envelope could also reuse the earlier work order token, but I'd defer changing that to when we implement caching behaviour. These changes will currently lead to expiry issues with the presigned URLs and need the caching changes on top to function correctly for all file sizes
  • Do not pass exceptions through queue, let them raise in task and properly deal with cancelling remaining tasks. Also added explicit calls to cancel in other places where an exception is raised outside of a task, but should result in cancellation as well
  • Moved progress bar out of drain_queue_to_file to properly exit on outer exceptions

@coveralls
Copy link

coveralls commented Dec 12, 2024

Pull Request Test Coverage Report for Build 12316326061

Details

  • 74 of 92 (80.43%) changed or added relevant lines in 9 files are covered.
  • 4 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.3%) to 71.489%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ghga_connector/cli.py 2 3 66.67%
src/ghga_connector/core/client.py 7 8 87.5%
src/ghga_connector/core/main.py 1 5 20.0%
src/ghga_connector/core/downloading/batch_processing.py 11 16 68.75%
src/ghga_connector/core/downloading/downloader.py 46 53 86.79%
Files with Coverage Reduction New Missed Lines %
src/ghga_connector/core/downloading/progress_bar.py 1 91.3%
src/ghga_connector/cli.py 1 63.57%
src/ghga_connector/core/client.py 1 89.47%
src/ghga_connector/core/downloading/batch_processing.py 1 78.69%
Totals Coverage Status
Change from base Build 11123484136: 0.3%
Covered Lines: 1008
Relevant Lines: 1410

💛 - Coveralls

@mephenor mephenor requested a review from Cito December 13, 2024 09:14
@mephenor mephenor marked this pull request as ready for review December 13, 2024 09:14
Cito
Cito previously approved these changes Dec 13, 2024
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

Looks good, but didn't check all details. Made some minor suggestions.

src/ghga_connector/cli.py Show resolved Hide resolved
src/ghga_connector/core/downloading/batch_processing.py Outdated Show resolved Hide resolved
.pyproject_generation/pyproject_custom.toml Outdated Show resolved Hide resolved
@mephenor mephenor merged commit dc4040e into main Dec 16, 2024
11 checks passed
@mephenor mephenor deleted the fix/improve_donwload_experience branch December 16, 2024 07:29
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.

3 participants