Clean up UploadXXX tasks
#592
Conversation
Codecov ReportAttention: Patch coverage is
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #592 +/- ##
==========================================
+ Coverage 97.61% 97.65% +0.04%
==========================================
Files 464 464
Lines 37157 37098 -59
==========================================
- Hits 36271 36229 -42
+ Misses 886 869 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes
|
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #592 +/- ##
==========================================
+ Coverage 97.57% 97.61% +0.04%
==========================================
Files 429 429
Lines 35951 35892 -59
==========================================
- Hits 35079 35037 -42
+ Misses 872 855 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #592 +/- ##
==========================================
+ Coverage 97.57% 97.61% +0.04%
==========================================
Files 429 429
Lines 35951 35892 -59
==========================================
- Hits 35079 35037 -42
+ Misses 872 855 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #592 +/- ##
==========================================
+ Coverage 97.57% 97.61% +0.04%
==========================================
Files 429 429
Lines 35951 35892 -59
==========================================
- Hits 35079 35037 -42
+ Misses 872 855 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
tasks/tests/unit/test_upload_task.py
Outdated
| @@ -609,8 +595,7 @@ def test_upload_task_call_multiple_processors( | |||
| ], | |||
| report_code=None, | |||
| in_parallel=False, | |||
| is_final=False, | |||
| ), | |||
| is_final=True, | |||
There was a problem hiding this comment.
This is an interesting driveby fix. The previous code would get this wrong for the last chunk happens to be on a chunk border. I initially replicated the same buggy behavior, but then found a better fix for it that correctly flags the final chunk/task.
tasks/upload.py
Outdated
| ) | ||
| return {"was_setup": was_setup, "was_updated": was_updated} | ||
|
|
||
| def fetch_commit_yaml_and_possibly_store(self, commit, repository_service): | ||
| def fetch_commit_yaml_and_possibly_store(self, commit: Commit, repository_service): |
There was a problem hiding this comment.
If you'd like to type report_service, that's TorngitBaseAdapter 👍
There was a problem hiding this comment.
Ah nice, that is good to know! I found out that PreProcessUpload.fetch_commit_yaml_and_possibly_store is a duplicated version of this function, so I moved it and its associated tests over to services.repository, as it was also not dependent on self at all.
There was a problem hiding this comment.
Thanks for this change Arpad! All for typing + better conventions.
I left some comments although I didn't have as thorough input with the parallel processing part in uploads.py as I'm not as familiar, I'd encourage you to have someone's eyes on that if possible, also being a big change in the system.
And have you given this a test run locally/in staging to ensure it works with these changes?
This removes some unused code, and cleans up the rest: - Adds types and moves to more modern code patterns - Uses a `LoggerAdapter` to automatically add common log extras - Prefers list comprehensions and a chunks generator over manual loops and slicing
Swatinem
force-pushed
the
swatinem/cleanup-upload
branch
from
e519695 to
edbc490
Compare
|
Converting this to a draft. I will be splitting this up into smaller more digestible pieces. |
Various improvements like fixing typos, adding types and using more modern code patterns
For the
Uploadtask in particular:LoggerAdapterto simplify adding more logging contextchunksgenerator together with list comprehension instead of manual loopsis_finalAs this re-indents a bunch of code, I highly recommend to use "ignore whitespace" when reviewing.