-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add async friendly upload #626
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #626 +/- ##
==========================================
- Coverage 82.28% 81.34% -0.94%
==========================================
Files 78 78
Lines 6124 6139 +15
==========================================
- Hits 5039 4994 -45
- Misses 1085 1145 +60
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
This is a great first attempt, and I like the general structure of the change. I would be very interested in seeing implementations and tests for the cloud blob stores, especially GCS. |
Thanks! I've tried to be as noninvasive as possible. However, besides passing the tests, we don't know yet if it's solving the originating issue.
I'm also curious about how it'd look like. Should we add the changes to activate testing against a GCS blob store in a separate PR? |
Yes, let's put that in a separate PR. |
@ivergara, can you rebase with |
…uetz into async_friendly_upload
@ivergara, can you take care of the lint? then this should be good to go. 👍 |
The tests against GCS did pass 🚀! Thus, at least, behavior wise it's working. Will fix the linting issue and then we're ready to merge and do a manual stress test. EDIT: However, the issue in the linting showed me that perhaps it was a false positive. Hope it's not the case. |
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.
awesome!
Still a work in progress, but should be working in the local package store.
the
add_package
was not directly tested, only theadd_files
, but the upload doesn't use theadd_files
. Thus I added a very minimal test foradd_package
(with a bonus refactoring) and then went to turn it into an asynchronous function.aiofiles
was already part of the dependencies but was never used until now, which provides a facade of synchronicity for FS local operations.We can discuss some decisions, but first I wanted to make it work. Instead of saving the uploaded file with a random name and then renaming it, I'm writing it directly with the correct name. I tried the other way but didn't manage. Could give it another try if it's really critical.
Anyhow, would appreciate early feedback.
Edit:
I did end up keeping the original
add_package
because it's used in other areas and don't want to pollute the codebase with async/await keywords. Thus, added a separate async version foradd_package
.