Move loading and merging intermediate Reports to its own file #767
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #767 +/- ##
=======================================
Coverage 98.03% 98.03%
=======================================
Files 440 442 +2
Lines 36566 36613 +47
=======================================
+ Hits 35846 35895 +49
+ Misses 720 718 -2
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 #767 +/- ##
=======================================
Coverage 98.03% 98.03%
=======================================
Files 440 442 +2
Lines 36566 36613 +47
=======================================
+ Hits 35846 35895 +49
+ Misses 720 718 -2
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 #767 +/- ##
=======================================
Coverage 98.03% 98.03%
=======================================
Files 440 442 +2
Lines 36566 36613 +47
=======================================
+ Hits 35846 35895 +49
+ Misses 720 718 -2
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.
Additional details and impacted files@@ Coverage Diff @@
## main #767 +/- ##
=======================================
Coverage 98.03% 98.03%
=======================================
Files 440 442 +2
Lines 36566 36613 +47
=======================================
+ Hits 35846 35895 +49
+ Misses 720 718 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Swatinem
force-pushed
the
swatinem/merging
branch
from
b5a13dd
to
003b21b
Compare
This should make the high level steps of parallel processing a bit more obvious and easier to grasp.
Swatinem
force-pushed
the
swatinem/merging
branch
from
003b21b
to
c100382
Compare
Swatinem
force-pushed
the
swatinem/merging
branch
from
7b0e134
to
5774097
Compare
| upload_ids: list[int], | ||
| ) -> list[IntermediateReport]: | ||
| @sentry_sdk.trace | ||
| def load_report(upload_id: int) -> IntermediateReport: |
There was a problem hiding this comment.
[very nit] I wonder if it would make sense for this to be a method of IntermediateReport class. Maybe a classmethod like load.
I only say this to avoid having 1 of 2 functions inside another function. But that is a personal preference.
There was a problem hiding this comment.
its a good idea in general, though that would require to pass the archive_service and commitsha to that method, as the current fn captures that from the parent scope.
| upload.state = "processed" | ||
| upload.order_number = session_id | ||
|
|
||
| if upload: |
There was a problem hiding this comment.
is this supposed to be inside the for? It seems to only affect the last upload.
From it being a loop it looks like there might be more than 1 upload
There was a problem hiding this comment.
that is indeed confusing, I admit. The Upload is really only used to get the DB session, as well as joining the Upload.report. It is pretty much querying all the sibling uploads for the same report. so any Upload (for the same report, which is true here) will do.
There was a problem hiding this comment.
Ah I see. Thank for the clarification
| upload.state = "processed" | ||
| upload.order_number = session_id | ||
|
|
||
| if upload: |
There was a problem hiding this comment.
Ah I see. Thank for the clarification
This should make the high level steps of parallel processing a bit more obvious and easier to grasp.